547492 - Use correct resize cursor for collapsed splitters

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Stefan [:stefanh]]

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.

Comment 1

[Stefan [:stefanh]]

Attachment: proposed patch

This should take care of the collapsed state.

Comment 2

[Stefan [:stefanh]]

Attachment: Testcase

Comment 3

[Dão Gottwald [:dao]]

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?

Comment 4

[Stefan [:stefanh]]

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

Comment 5

[Stefan [:stefanh]]

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

Comment 6

[Dão Gottwald [:dao]]

That seems to be an adequate interpretation... just leave it then.

Comment 7

[Stefan [:stefanh]]

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

Comment 8

[Stefan [:stefanh]]

Attachment: New version, should be rtl-friendly

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.

Comment 9

[Stefan [:stefanh]]

Attachment: browser.xul patch

Comment 10

[Stefan [:stefanh]]

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.

Comment 11

[(no longer active)]

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.

Comment 12

[Stefan [:stefanh]]

(In reply to comment #11)
> Could you push a try server build with the set of required patches?

Good point, will do.

Comment 13

[Stefan [:stefanh]]

I just pushed http://hg.mozilla.org/try/rev/f98513294b07

Comment 14

[Stefan [:stefanh]]

tryserver builds available at https://build.mozilla.org/tryserver-builds/[e-mail]-try-f98513294b07/

Comment 15

[(no longer active)]

(In reply to comment #14)
> tryserver builds available at
> https://build.mozilla.org/tryserver-builds/[e-mail]-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

Comment 16

[Stefan [:stefanh]]

(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?

Comment 17

[Stefan [:stefanh]]

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

Comment 18

[Stefan [:stefanh]]

Hmm, I also see some problem with the substate attribute. Sometimes it doesn't get set when you collapse the splitter the first time.

Comment 19

[Stefan [:stefanh]]

I need to investigate comment #18 before I continue here...

Comment 20

[Stefan [:stefanh]]

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.

Comment 21

[Stefan [:stefanh]]

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.

Comment 22

[Stefan [:stefanh]]

Attachment: Test extension with grippy

Here's a test extension that uses a grippy - works fine in rtl mode

Comment 23

[Stefan [:stefanh]]

Attachment: Test extension, plain splitter

And here's a test extension without a grippy - the collapsing is correct, but you have to drag in the "opposite" direction.

Comment 24

[Stefan [:stefanh]]

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.

Comment 25

[Stefan [:stefanh]]

Attachment: New version

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.

Comment 26

[Stefan [:stefanh]]

fwiw, I pushed attachment #437126 [details] [diff] [review] to the tryserver

Comment 27

[Stefan [:stefanh]]

(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/[e-mail]-try-2dbe789cbcbf/

Comment 28

[(no longer active)]

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.

Comment 29

[Stefan [:stefanh]]

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.

Comment 30

[Stefan [:stefanh]]

If you have DOMi installed, you could also try the splitter in there.

Comment 31

[(no longer active)]

(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 32

[(no longer active)]

Comment on attachment 437126 [details] [diff] [review]
New version

r- based on comment 31.

Comment 33

[Stefan [:stefanh]]

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

Comment 34

[(no longer active)]

(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?

Comment 35

[(no longer active)]

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

Comment 36

[Stefan [:stefanh]]

Attachment: Now with rtl-friendly grippy arrows

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.

Comment 37

[Dão Gottwald [:dao]]

(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?

Comment 38

[Stefan [:stefanh]]

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

Comment 39

[Stefan [:stefanh]]

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.

Comment 40

[(no longer active)]

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?  :-)

Comment 41

[Stefan [:stefanh]]

Hmm, I'm wondering if it's better to remove the substate attribute when we're not collapsed?

Comment 42

[(no longer active)]

There's not much point to it, since that attribute is documented to be meaningless when state != "collapsed".

Comment 43

[Stefan [:stefanh]]

Since it's documented to be meaningless unless collapse == "both" and state == "collapsed", is there any reason for having it for the other cases?

Comment 44

[(no longer active)]

I'm not sure, maybe we can remove it, but I don't see the point in that.

Comment 45

[Dão Gottwald [:dao]]

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.

Comment 46

[Stefan [:stefanh]]

Thanks, I'll do the renaming when I land this next week. I'll also file a bug about the grippy-issue.

Comment 47

[Stefan [:stefanh]]

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.