337955 - XUL splitter frames can only collapse in one direction

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[Andrew Miller]

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.

Comment 1

[Neil Deakin]

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

Comment 2

[Andrew Miller]

>  ------- 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.

Comment 3

[Andrew Miller]

Attachment: Allow splitters to collapse both ways when collapse="both" set

Comment 4

[Neil Deakin]

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

Comment 5

[Andrew Miller]

Attachment: Testcase: XUL splitters

A testcase for splitters.

Comment 6

[Andrew Miller]

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).

Comment 7

[Andrew Miller]

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".

Comment 8

[Andrew Miller]

Attachment: Attachment 227027 with an added case to guess at substate when it is absent

Comment 9

[Neil Deakin]

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

Comment 10

[Andrew Miller]

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.

Comment 11

[Andrew Miller]

Attachment: Patch against CVS which addresses review comments from [e-mail]

Comment 12

[Neil Deakin]

Comment on attachment 228745 [details] [diff] [review]
Patch against CVS which addresses review comments from [e-mail]

>+            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.

Comment 13

[Andrew Miller]

Attachment: Fix problem found by Neil Deakin in Comment 12

Comment 14

[Neil Deakin]

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

Comment 15

[Martijn Wargers (dead)]

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.