Last Comment Bug 337955 - XUL splitter frames can only collapse in one direction
: XUL splitter frames can only collapse in one direction
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Andrew Miller
:
: Neil Deakin
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-14 20:22 PDT by Andrew Miller
Modified: 2008-07-31 03:21 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Allow splitters to collapse both ways when collapse="both" set (11.17 KB, patch)
2006-06-25 19:11 PDT, Andrew Miller
no flags Details | Diff | Splinter Review
Testcase: XUL splitters (1.76 KB, application/vnd.mozilla.xul+xml)
2006-06-27 15:00 PDT, Andrew Miller
no flags Details
Attachment 227027 with an added case to guess at substate when it is absent (11.30 KB, patch)
2006-06-27 16:54 PDT, Andrew Miller
enndeakin: review-
Details | Diff | Splinter Review
Patch against CVS which addresses review comments from enndeakin@sympatico.ca (10.37 KB, patch)
2006-07-10 16:43 PDT, Andrew Miller
enndeakin: review+
Details | Diff | Splinter Review
Fix problem found by Neil Deakin in Comment 12 (10.36 KB, patch)
2006-07-17 16:57 PDT, Andrew Miller
neil: superreview+
Details | Diff | Splinter Review

Description Andrew Miller 2006-05-14 20:22:32 PDT
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 2006-05-18 14:47:54 PDT
Can you give an use case where this would be useful?
Comment 2 Andrew Miller 2006-05-18 14:57:02 PDT
>  ------- 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 2006-06-25 19:11:22 PDT
Created attachment 227027 [details] [diff] [review]
Allow splitters to collapse both ways when collapse="both" set
Comment 4 Neil Deakin 2006-06-27 09:25:13 PDT
A testcase would be useful here, and/or a description of what attributes you've added.
Comment 5 Andrew Miller 2006-06-27 15:00:10 PDT
Created attachment 227301 [details]
Testcase: XUL splitters

A testcase for splitters.
Comment 6 Andrew Miller 2006-06-27 15:20:18 PDT
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 2006-06-27 15:28:13 PDT
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 2006-06-27 16:54:40 PDT
Created attachment 227325 [details] [diff] [review]
Attachment 227027 [details] [diff] with an added case to guess at substate when it is absent
Comment 9 Neil Deakin 2006-07-10 11:19:54 PDT
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 2006-07-10 14:39:07 PDT
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 2006-07-10 16:43:52 PDT
Created attachment 228745 [details] [diff] [review]
Patch against CVS which addresses review comments from enndeakin@sympatico.ca
Comment 12 Neil Deakin 2006-07-17 08:57:18 PDT
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.
Comment 13 Andrew Miller 2006-07-17 16:57:44 PDT
Created attachment 229580 [details] [diff] [review]
Fix problem found by Neil Deakin in Comment 12
Comment 14 Neil Deakin 2006-07-18 06:55:27 PDT
I meant, document on the Talk/Discussion page, not the main page, since this feature isn't available yet.
Comment 15 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-07-19 15:32:48 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.