Andrew Miller Assignee
	Description • 18 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3

The current nsSplitterFrame.cpp only allows collapses in one direction or the other (as specified by the collapse attribute). This limitation means that developers must choose either to allow collapse "before" or collapse "after", even in situations where a collapse in either direction makes more sense (which is in turn confusing for end-users). It would be nice to support a bi-directional collapse, perhaps using something like "collapse=both".

Reproducible: Always

Steps to Reproduce:
See http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsSplitterFrame.cpp
Actual Results:  
Only collapse="before", collapse="after", and collapse="none" are supported.

Expected Results:  
Something like <splitter collapse="both" /> should allow collapses both before and after in response to mouse drags.

	Neil Deakin
	Comment 1 • 18 years ago

Can you give an use case where this would be useful?

	Andrew Miller Assignee
	Comment 2 • 18 years ago

>  ------- Comment #1 From Neil Deakin  2006-05-18 14:47 PDT  [reply] -------
> Can you give an use case where this would be useful?
I think probably most cases where splitter are used to separate the substantial UI into components would benefit from this. Perhaps a good existing example application which uses splitters like this is Sunbird. At the moment, it is possible to re-arrange the splitters so only the calendar view can be seen, but it is not possible, for example, to show only the to-do list (the calendar must sit there at the minimum size) or the event listing.

	Neil Deakin
	Comment 4 • 18 years ago

A testcase would be useful here, and/or a description of what attributes you've added.

	Andrew Miller Assignee
	Comment 5 • 18 years ago

A testcase for splitters.

	Andrew Miller Assignee
	Comment 6 • 18 years ago

Comment on attachment 227027 [details] [diff] [review]
Allow splitters to collapse both ways when collapse="both" set

A new patch is coming shortly, which will infer the substate from the collapse attribute when it is absent (to reduce the risk of breaking existing XUL pages).

	Andrew Miller Assignee
	Comment 7 • 18 years ago

Updates due to this patch:
1) Splitter attribute collapse now supports a new value, "both", allowing collapses to occur in both directions.
2) In addition to the state attribute, which still takes the value "collapsed" when the splitter is collapsed, there is a substate attribute, which indicates "collapsed_before" or "collapsed_after", to determine the direction in which the splitter has collapsed. The use of a new attribute, instead of changing the values state can take, was deliberate, in order to minimise the impact on existing style sheets.
3) If state="collapsed", but no substate is set (or it is invalid), then the code will look at the collapse attribute. If it is after, it will act as if the substate was "collapsed_before". Otherwise, a substate of collapsed_before is inferred. This behaviour should minimise the impact on XUL which wants to continue just setting state="collapsed", and using collapse="before" or collapse="after".

	Neil Deakin
	Comment 9 • 18 years ago

Comment on attachment 227325 [details] [diff] [review]
Attachment 227027 [details] [diff] with an added case to guess at substate when it is absent

OK, I now understand what this patch is adding. It allows a splitter to be dragged and collapse on both sides.

>+GK_ATOM(collapsed_after, "collapsed_after")
>+GK_ATOM(collapsed_before, "collapsed_before")

Just 'before' and 'after' for these attribute values.

>+    case 0:
>+      if (aDirection == Before)
>+        return PR_TRUE;
>+      return PR_FALSE;

return (aDirection == Before)

>+    case 1:
>+      if (aDirection == After)
>+        return PR_TRUE;
>+      return PR_FALSE;

return (aDirection == After)


>+  PRBool collapseBefore = SupportsCollapseDirection(Before);
>+  PRBool collapseAfter = SupportsCollapseDirection(After);
>+  if (collapseBefore || collapseAfter) {

if (SupportsCollapseDirection(Before) || SupportsCollapseDirection(After))

would allow the latter to only be executed when needed.

>     nsIBox* splitterSibling =
>       nsFrameNavigator::GetChildBeforeAfter(mOuter->GetPresContext(), splitter,
>-                                            (direction == Before));
>+                                            (newState == CollapsedBefore ||
>+                                             mState == CollapsedBefore));

This doesn't look right to me. Why does it matter if the old state was 'before'?

>+        if (mState == CollapsedBefore) {
>+          // CollapsedBefore -> Open
>+          // CollapsedBefore -> Dragging
>           sibling->UnsetAttr(kNameSpaceID_None, nsXULAtoms::collapsed,
>                              PR_TRUE);
>+        } else if (mState == CollapsedAfter) {
>+          // CollapsedAfter -> Open
>+          // CollapsedAfter -> Dragging
>+          sibling->UnsetAttr(kNameSpaceID_None, nsXULAtoms::collapsed,
>+                             PR_TRUE);

Both of these could be combined into a single if statement

	Andrew Miller Assignee
	Comment 10 • 18 years ago

Thanks for the review Neil. I have fixed the points you raised, and will be making a new patch shortly. Regarding this comment:
>This doesn't look right to me. Why does it matter if the old state was
>'before'?
>
>>+        if (mState == CollapsedBefore) {
>>+          // CollapsedBefore -> Open
>>+          // CollapsedBefore -> Dragging
>>           sibling->UnsetAttr(kNameSpaceID_None, nsXULAtoms::collapsed,
>>                              PR_TRUE);
>>+        } else if (mState == CollapsedAfter) {
>>+          // CollapsedAfter -> Open
>>+          // CollapsedAfter -> Dragging
>>+          sibling->UnsetAttr(kNameSpaceID_None, nsXULAtoms::collapsed,
>>+                             PR_TRUE);

The same code is used to transition from Collapsed* to Open/Dragging and Open/Dragging to Collapsed*. Therefore, the old state matters because we must know which sibling to transition from a collapsed state to open/dragging.

	Neil Deakin
	Comment 12 • 18 years ago

Comment on attachment 228745 [details] [diff] [review]
Patch against CVS which addresses review comments from enndeakin@sympatico.ca

>+            mOuter->mContent->SetAttr(kNameSpaceID_None, nsXULAtoms::substate,
>+                                      NS_LITERAL_STRING("collapsed_after"),

This should be "after"

Otherwise, OK.

Also, please add a note to the talk page of http://developer.mozilla.org/en/docs/XUL:splitter listing the new feature for 1.9, so it isn't forgotten.

	Neil Deakin
	Comment 14 • 18 years ago

I meant, document on the Talk/Discussion page, not the main page, since this feature isn't available yet.

	Martijn Wargers (dead)
	Comment 15 • 18 years ago

Checking in content/base/src/nsGkAtomList.h;
/cvsroot/mozilla/content/base/src/nsGkAtomList.h,v  <--  nsGkAtomList.h
new revision: 3.28; previous revision: 3.27
done
Checking in layout/xul/base/src/nsSplitterFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsSplitterFrame.cpp,v  <--  nsSplitterFrame
.cpp
new revision: 1.150; previous revision: 1.149
done

Checked into trunk, thanks for the patch Andrew!
Next time, if you want to have a patch checked in, you can also add the [checkin needed] text in the status whiteboard.