Closed Bug 547492 Opened 10 years ago Closed 10 years ago

Use correct resize cursor for collapsed splitters

Categories

(Toolkit :: Themes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: stefanh, Assigned: stefanh)

References

Details

Attachments

(4 files, 4 obsolete files)

If a splitter is collapsed, the cursor should give an indication of what direction the resize can be done - atm it give you the impression that you can resize the widget in both directions.
Attached patch proposed patch (obsolete) — Splinter Review
This should take care of the collapsed state.
Assignee: nobody → stefanh
Status: NEW → ASSIGNED
Attachment #427991 - Flags: review?(dao)
Attached file Testcase
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 427991 [details] [diff] [review]
proposed patch

>+splitter[state="collapsed"][collapse="before"],
>+splitter[state="collapsed"][substate="before"] {

Isn't the first selector redundant?

>+  cursor: e-resize;

Is this correct for rtl?

>+}
>+
>+splitter[state="collapsed"][collapse="after"],
>+splitter[state="collapsed"][substate="after"] {
>+  cursor: w-resize;
>+}
>+
> /* ::::: splitter (horizontal) ::::: */

This seems to mean "splitter (vertical)". Can you fix this while you're at it?
(In reply to comment #3)
> (From update of attachment 427991 [details] [diff] [review])
> >+splitter[state="collapsed"][collapse="before"],
> >+splitter[state="collapsed"][substate="before"] {
> 
> Isn't the first selector redundant?

I was first going to say "for the second line, yes" since the documentation for substate says "On splitters which have state="collapsed" and collapse="both", determines which direction the splitter is actually collapsed in". However, DOMi says something different, so you're 100% right.

> >+  cursor: e-resize;
> 
> Is this correct for rtl?

Hm. Probably not.

> 
> >+}
> >+
> >+splitter[state="collapsed"][collapse="after"],
> >+splitter[state="collapsed"][substate="after"] {
> >+  cursor: w-resize;
> >+}
> >+
> > /* ::::: splitter (horizontal) ::::: */
> 
> This seems to mean "splitter (vertical)". Can you fix this while you're at it?
Sure.
(In reply to comment #4)
> > This seems to mean "splitter (vertical)". Can you fix this while you're at it?
> Sure.

Actually, "splitter (horizontal)" means that the splitter is laid out horizontally (and resize is then up/down). Tell me if you still want me to change this (it's all over the files, also for the grippies).
Attachment #427991 - Flags: review?(dao)
That seems to be an adequate interpretation... just leave it then.
>+splitter[state="collapsed"][collapse="before"],
>+splitter[state="collapsed"][substate="before"] 

OK, so it turns out having collapse="before" will result in substate="before" when collapsed. However, the substate isn't removed when the splitter is uncollapsed.
This one should be rtl-friendly. However, I'm not sure the spliter collapsing is 100% rtl-friendly. I patched browser.xul and played with the sidebar splitter in rtl mode (thanks Ehsan) and the behavior is a bit strange. Basically, setting collapse="both" on the sidebar-splitter and dragging it right/left will always result in the splitter at the right side (and thus wrong cursor for the "after" case).

Ehsan, would you mind helping me out here? I'm not sure if I'm doing something wrong when testing.
Attachment #427991 - Attachment is obsolete: true
Attached patch browser.xul patch (obsolete) — Splinter Review
Comment on attachment 428087 [details] [diff] [review]
browser.xul patch

This is what I used when testing the rtl compat. The pinstripe hunk is just to make the splitter more visible.
Could you push a try server build with the set of required patches?  I'm not sure if I understand comment 8 correctly, and I've recently managed to nuke out my repository, so I can't apply and try the patch locally that quickly.
(In reply to comment #11)
> Could you push a try server build with the set of required patches?

Good point, will do.
(In reply to comment #14)
> tryserver builds available at
> https://build.mozilla.org/tryserver-builds/stefanh@inbox.com-try-f98513294b07/

I tried this build, and the splitter behavior seems pretty broken.  Here is what I did:

1. Created a new profile and installed Force RTL, and switched into RTL mode.
2. Opened the History sidebar.
3. Dragged the splitter to right.

When the splitter reaches the far right side of the window, the cursor's pointing to the wrong direction (right instead of left), and also the splitter actually expands to cover the entire content area.  Screenshot:

http://grab.by/2GKh
(In reply to comment #15)
> When the splitter reaches the far right side of the window, the cursor's
> pointing to the wrong direction (right instead of left), and also the splitter
> actually expands to cover the entire content area.

Yeah, I see that too. The reason why the splitter is pointing eastwards is that the the value of the substate attribute is "after". The key thing here is what attribute value you expect in rtl mode. Wouldn't you expect "before"?

Oddly, if I move the splitter slightly to the left, the content area becomes visible and the splitter direction is the expected.

Dao, I don't think we can get a correct splitter behaviour in rtl mode with css - I think the behaviour is broken from the start :-(. What do you suggest? Skip rtl for now?
JFTR, I'm going to look at this a bit more and decide if it's worth including the rtl changes before the odd behavior is sorted out (in another bug).
Hmm, I also see some problem with the substate attribute. Sometimes it doesn't get set when you collapse the splitter the first time.
I need to investigate comment #18 before I continue here...
Status: ASSIGNED → NEW
What's confusing is that if you read https://developer.mozilla.org/en/XUL/Attribute/substate, the substate attribute should only exists if you have collapse="both". But this doesn't seem to be the case.
OK, so after talking to neil it turns out that we need the before/after collapse attribute values as well - it's only when you have "both" set that the substate attributes are correctly set. This explains my comment #18.

Further, I think I've isolated the rtl problem: it seems that the splitter collapses the correct sibling (after means the left sibling etc) when in rtl mode. However, you need to drag the splitter in the opposite direction.
Here's a test extension that uses a grippy - works fine in rtl mode
And here's a test extension without a grippy - the collapsing is correct, but you have to drag in the "opposite" direction.
Both extension works with Minefield/SeaMonkey trunk builds (I learned by trial/error that you need a locale/en-US dir to make it work), just set intl.uidirection.en-US to "rtl" and you should see what I described above.
Attached patch New version (obsolete) — Splinter Review
I'm unsure if we should skip the rtl styles for now since MDC actually says that you should use a grippy if you set the collapse attribute on a splitter (that will obviously be problematic with "both", though). Regardless, I will of course file a bug for the rtl issue.
Attachment #437126 - Flags: review?(dao)
fwiw, I pushed attachment #437126 [details] [diff] [review] to the tryserver
(In reply to comment #26)
> fwiw, I pushed attachment #437126 [details] [diff] [review] to the tryserver

Tryserver builds available at https://build.mozilla.org/tryserver-builds/stefanh@inbox.com-try-2dbe789cbcbf/
I downloaded the try server build, but I'm not sure how to test it.  I don't get any one-sided cursors, no matter what I try to do.
Sorry for being unclear. The patch (attachment #437126 [details] [diff] [review]) only works with collapsed splitters, so you need to do the following:

1) Launch the tryserver build:
2) Install the 2 extensions in comment #22 and comment #23
3) Go to about:config and set 'intl.uidirection.en-US' to 'rtl'
4) In the Tools menu, select "grippy-test" to test the patch with a grippy splitter. Clicking the grippy should collapse the splitter in the chosen direction. You can choose direction by clicking one of the 2 buttons (they just sets the collapse attribute to either 'before' or 'after' - default is 'before').
5) In the Tools menu, select "splitter-test" to test the patch with a plain, normal splitter without a grippy. This is where I can see a rtl issue with the splitter. You can choose the 'after', 'before' or 'both' values for the collapse attribute, dragging the splitter in one direction should eventually collapse the splitter in the desired direction. The problem I see is that it actually does collapse in the right direction, but in order to make it collapse you have to drag it in the opposite direction.
If you have DOMi installed, you could also try the splitter in there.
(In reply to comment #29)
> Sorry for being unclear. The patch (attachment #437126 [details] [diff] [review]) only works with
> collapsed splitters, so you need to do the following:
> 
> 1) Launch the tryserver build:
> 2) Install the 2 extensions in comment #22 and comment #23
> 3) Go to about:config and set 'intl.uidirection.en-US' to 'rtl'
> 4) In the Tools menu, select "grippy-test" to test the patch with a grippy
> splitter. Clicking the grippy should collapse the splitter in the chosen
> direction. You can choose direction by clicking one of the 2 buttons (they just
> sets the collapse attribute to either 'before' or 'after' - default is
> 'before').

In RTL mode, if I click before, the arrows point to the left, but the splitter collapses to the right.  The reverse thing happens for after.  This seems to be wrong.

> 5) In the Tools menu, select "splitter-test" to test the patch with a plain,
> normal splitter without a grippy. This is where I can see a rtl issue with the
> splitter. You can choose the 'after', 'before' or 'both' values for the
> collapse attribute, dragging the splitter in one direction should eventually
> collapse the splitter in the desired direction. The problem I see is that it
> actually does collapse in the right direction, but in order to make it collapse
> you have to drag it in the opposite direction.

In RTL mode, if I click before, the splitter collapses on the left side, and when collapsed, the cursor points to the left.  The reverse thing happens for after.  This is wrong!

If I click both, the splitter collapses on both sides, but the cursor is wrong in both cases.
Comment on attachment 437126 [details] [diff] [review]
New version

r- based on comment 31.
Attachment #437126 - Flags: review-
(In reply to comment #31)
> (In reply to comment #29)
> > Sorry for being unclear. The patch (attachment #437126 [details] [diff] [review] [details]) only works with
> > collapsed splitters, so you need to do the following:
> > 
> > 1) Launch the tryserver build:
> > 2) Install the 2 extensions in comment #22 and comment #23
> > 3) Go to about:config and set 'intl.uidirection.en-US' to 'rtl'
> > 4) In the Tools menu, select "grippy-test" to test the patch with a grippy
> > splitter. Clicking the grippy should collapse the splitter in the chosen
> > direction. You can choose direction by clicking one of the 2 buttons (they just
> > sets the collapse attribute to either 'before' or 'after' - default is
> > 'before').
> 
> In RTL mode, if I click before, the arrows point to the left, but the splitter
> collapses to the right.  The reverse thing happens for after.  This seems to be
> wrong.

I don't understand this - to mee it sounds right. When you click the grippy when the splitter has collapse="before" you'd expect the right side to collapse, don't you (that'd be the "before" in rtl mode)? Then, when the right side is collapsed, there's ony one direction you can drag the splitter... and that's left.
> 
> > 5) In the Tools menu, select "splitter-test" to test the patch with a plain,
> > normal splitter without a grippy. This is where I can see a rtl issue with the
> > splitter. You can choose the 'after', 'before' or 'both' values for the
> > collapse attribute, dragging the splitter in one direction should eventually
> > collapse the splitter in the desired direction. The problem I see is that it
> > actually does collapse in the right direction, but in order to make it collapse
> > you have to drag it in the opposite direction.
> 
> In RTL mode, if I click before, the splitter collapses on the left side, and
> when collapsed, the cursor points to the left.  The reverse thing happens for
> after.  This is wrong!

No, if you click "before" and drag the splitter to the left - it will collapse to the right (look at the colors and you'll see what I mean). What's wrong here is not that the right side of the splitter collapses (which is imo correct since the right side is "before" in rtl mode), what's wrong is instead that in order to make the splitter collapse, you have to drag it to the leftmost side. What should happen is that the splitter should collapse the right side if you drag it to the right - but that has nothing to do with this patch.
(In reply to comment #33)
> (In reply to comment #31)
> > (In reply to comment #29)
> > > Sorry for being unclear. The patch (attachment #437126 [details] [diff] [review] [details] [details]) only works with
> > > collapsed splitters, so you need to do the following:
> > > 
> > > 1) Launch the tryserver build:
> > > 2) Install the 2 extensions in comment #22 and comment #23
> > > 3) Go to about:config and set 'intl.uidirection.en-US' to 'rtl'
> > > 4) In the Tools menu, select "grippy-test" to test the patch with a grippy
> > > splitter. Clicking the grippy should collapse the splitter in the chosen
> > > direction. You can choose direction by clicking one of the 2 buttons (they just
> > > sets the collapse attribute to either 'before' or 'after' - default is
> > > 'before').
> > 
> > In RTL mode, if I click before, the arrows point to the left, but the splitter
> > collapses to the right.  The reverse thing happens for after.  This seems to be
> > wrong.
> 
> I don't understand this - to mee it sounds right. When you click the grippy
> when the splitter has collapse="before" you'd expect the right side to
> collapse, don't you (that'd be the "before" in rtl mode)? Then, when the right
> side is collapsed, there's ony one direction you can drag the splitter... and
> that's left.

With splitter=before, when the grippy is not collapsed, it shows arrows pointing to left.  When you click it, it collapses to right.  The arrows on the grippy need to be corrected to actually point to where it will be collapsed.

Once the grippy is collapsed, its arrows point to the correct direction (because they show how you can expand it.

> > > 5) In the Tools menu, select "splitter-test" to test the patch with a plain,
> > > normal splitter without a grippy. This is where I can see a rtl issue with the
> > > splitter. You can choose the 'after', 'before' or 'both' values for the
> > > collapse attribute, dragging the splitter in one direction should eventually
> > > collapse the splitter in the desired direction. The problem I see is that it
> > > actually does collapse in the right direction, but in order to make it collapse
> > > you have to drag it in the opposite direction.
> > 
> > In RTL mode, if I click before, the splitter collapses on the left side, and
> > when collapsed, the cursor points to the left.  The reverse thing happens for
> > after.  This is wrong!
> 
> No, if you click "before" and drag the splitter to the left - it will collapse
> to the right (look at the colors and you'll see what I mean).

Do you mean that when you're dragging it to the left, as soon as it reaches to the left side, it will suddenly collapse to the right?

> What's wrong here
> is not that the right side of the splitter collapses (which is imo correct
> since the right side is "before" in rtl mode), what's wrong is instead that in
> order to make the splitter collapse, you have to drag it to the leftmost side.

Hmm, if I understand what you're saying here correctly, I agree with you.  (Note that this wrong behavior is the exact wrong behavior that I'm talking about in response to your previous sentence.)

Also, note that when you've dragged it to the left and it has collapsed, in order to expand it, you should again start dragging from the left side.  This is why I said (it collapses to the left).  I didn't really pay attention to the colors, though.  I think some screenshots would have helped here.

> What should happen is that the splitter should collapse the right side if you
> drag it to the right - but that has nothing to do with this patch.

So, what does your patch do?
Depends on: 558598
Comment on attachment 437126 [details] [diff] [review]
New version

r=me based on my discussion with Stefan on IRC.

He also promised to fix the arrows on the grippy.  :-)
Attachment #437126 - Flags: review- → review+
Here's a new version that adds rtl support to the grippy arrows. I didn't used the substate attribute, since I don't think "both" is ment to be used together with a grippy.
Attachment #428086 - Attachment is obsolete: true
Attachment #428087 - Attachment is obsolete: true
Attachment #437126 - Attachment is obsolete: true
Attachment #438360 - Flags: review?(dao)
Attachment #437126 - Flags: review?(dao)
Status: NEW → ASSIGNED
Blocks: 558668
(In reply to comment #7)
> >+splitter[state="collapsed"][collapse="before"],
> >+splitter[state="collapsed"][substate="before"] 
> 
> OK, so it turns out having collapse="before" will result in substate="before"
> when collapsed. However, the substate isn't removed when the splitter is
> uncollapsed.

Given [state="collapsed"], why is that a problem?
(In reply to comment #37)
> (In reply to comment #7)
> > >+splitter[state="collapsed"][collapse="before"],
> > >+splitter[state="collapsed"][substate="before"] 
> > 
> > OK, so it turns out having collapse="before" will result in substate="before"
> > when collapsed. However, the substate isn't removed when the splitter is
> > uncollapsed.
> 
> Given [state="collapsed"], why is that a problem?

I didn't expect it to be that way, but it's not a problem if you also rely on [state="collapse"].
Actually, I have just noticed a problem - It looks like dynamically changing the collapse attribute when you use grippies is a bad idea:

1) With the grippy-test extension (ltr), have the collapse attribute set to "before", then make the splitter collapse by clicking the grippy.

2) Check DOMi and notice that there is no substate attribute set, but since the collapse attribute is set to "before" and we're collapsed, the right cursor appears.

3) Drag the splitter right and then left (so it's collapsed again). Note that the substate attribute is then set to "before".

4) Drag the splitter to the center.

5) Click the "after" button so the collapse attribute changes to "after".

6) Note that the substate attribute is still "before"

7) Click the grippy and note that it collapses in the wrong direction and the cursor points left.
This is because the grippy implementation just sets the state attribute:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/splitter.xml#18

which ends up calling nsSplitterFrameInner::UpdateState:

http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsSplitterFrame.cpp#862

which does not set the substate attribute.

Can you file a bug about this in Core::XUL?  Also, are you willing to write a patch for it?  :-)
Hmm, I'm wondering if it's better to remove the substate attribute when we're not collapsed?
There's not much point to it, since that attribute is documented to be meaningless when state != "collapsed".
Since it's documented to be meaningless unless collapse == "both" and state == "collapsed", is there any reason for having it for the other cases?
I'm not sure, maybe we can remove it, but I don't see the point in that.
Comment on attachment 438360 [details] [diff] [review]
Now with rtl-friendly grippy arrows

Please rename grip-vrt-before.gif to grip-left.gif, grip-vrt-after.gif to grip-right.gif, grip-hrz-before.gif to grip-top.gif and grip-hrz-after.gif to grip-bottom.gif.
Attachment #438360 - Flags: review?(dao) → review+
Thanks, I'll do the renaming when I land this next week. I'll also file a bug about the grippy-issue.
I messed up a bit when I landed this, so I had to do 3 pushes:
http://hg.mozilla.org/mozilla-central/rev/f8455e7935f5
http://hg.mozilla.org/mozilla-central/rev/06d46390acb0
http://hg.mozilla.org/mozilla-central/rev/9585e4ed630e

Filed bug 560310 for the grippy-collapse issue.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
You need to log in before you can comment on or make changes to this bug.