343097 - Cleaner arrowscrollbox-clicktoscroll binding

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P1)

Description

[Asaf Romano (gone)]

Bug 318168 added handleOnClick and handleOnCommand to the arrowscrollbox binding API (for internal use, called in each anonymouse node oncommand/onclick attributes), we can avoid this by adding XBL event handlers.

Comment 1

[Asaf Romano (gone)]

Taking.

Comment 2

[Asaf Romano (gone)]

What I really don't understand is why do we have both onclick and oncommand handlers. AFAICT the only way to get a command event for these buttons is via a click event (i.e. the click event is the sourceEvent for the command event), this tells me we scroll twice - once by pixels, once by "lines".

Comment 3

[Asaf Romano (gone)]

Well, ignore comment 2.

Comment 4

[Asaf Romano (gone)]

Morphing this a bit to correspond my tree ;)

Comment 5

[Asaf Romano (gone)]

Attachment: patch

Comment 6

[Asaf Romano (gone)]

Note to self: also need to fix the drag handler in tabbrowser.

Comment 7

[Asaf Romano (gone)]

Attachment: patch

Comment 8

[(not reading, please use [e-mail] instead)]

Comment on attachment 227541 [details] [diff] [review]
patch

r=sspitzer, this is much better and cleaner than how I did it originally.

Comment 9

[John Mellor (Jomel)]

Would it be possible to make holding down the button scroll continuously (something like hovering a normal arrowscrollbox does)? Clicking manically on those things is going to give us all RSI :(

Comment 10

[(not reading, please use [e-mail] instead)]

> Would it be possible to make holding down the button scroll continuously

see bug #342906

Comment 11

[Asaf Romano (gone)]

trunk:
mozilla/toolkit/content/xul.css 1.78
mozilla/toolkit/content/widgets/scrollbox.xml 1.9
mozilla/toolkit/content/widgets/tabbrowser.xml 1.161
mozilla/toolkit/themes/pinstripe/global/scrollbox.css  1.4
mozilla/toolkit/themes/winstripe/global/scrollbox.css 1.6

Comment 12

[Asaf Romano (gone)]

Comment on attachment 227541 [details] [diff] [review]
patch

We need this on 181b1 asap if bug 342906 is still a (181b1) blocker.

Comment 13

[(not reading, please use [e-mail] instead)]

asaf, I've updated my trunk and rebuilt since your landing and I see a couple issues.

1)  context menus have scroll up / down buttons.
2)  the horizontal scrollbox buttons are bigger, and have more of a button like appearance and behavior (but this may be desirable, see bug #342909).  

I'll attach a couple screen shots.

Note, I've updated my entire tree, so these might not be regressions due to your change.

Comment 14

[(not reading, please use [e-mail] instead)]

Attachment: screen shot of the context menu with arrows

Comment 15

[(not reading, please use [e-mail] instead)]

Attachment: scrollbox buttons are bigger and button like (the one on the right I am moused over)

Comment 16

[Asaf Romano (gone)]

I don't see how this could be a regression of this change, as i've only removed click-to-scroll related stuff, going to check in my trunk build now.

The button look & feel was intended, I think. If this appears wrong we can tweak the style.

Comment 17

[Asaf Romano (gone)]

FWIW, i wasn't able to reproduce this in my mac trunk build (with cvs up in toolkit and browser only).

Comment 18

[(not reading, please use [e-mail] instead)]

> The button look & feel was intended, I think. If this appears wrong we can
> tweak the style.

I think we should make them look and act like buttons (including disabling them, see bug #342841, if possible) when in "clicktoscroll" mode.

we should center the image in the button, too.  currently the arrow images are on the left within each button.

Comment 19

[(not reading, please use [e-mail] instead)]

> 1)  context menus have scroll up / down buttons.

false alarm.  it may have been cvs-merge weirdness in my local tree.  after checking out toolkit and rebuilding again, things appear fine.

Comment 20

[(not reading, please use [e-mail] instead)]

Comment on attachment 227964 [details]
screen shot of the context menu with arrows

ignore this screen shot.

Comment 21

[Bill Gianopoulos [:WG9s]]

My bookmarks menu is too tall to fit on the screen.  Since this patch landed, I am unable to scroll to see all the bookmarks.

Comment 22

[Asaf Romano (gone)]

Bill Gianopoulos: Did you test this by backing out? A few other scrollbox fixes have landed today (which affect all kinds of scrollboxes), this one hardly touches the normal&vertical binding.

Comment 23

[Asaf Romano (gone)]

Attachment: regression fix

Fix autorepeat issue (see comment 21), thanks for pointing this out Bill.

Comment 24

[(not reading, please use [e-mail] instead)]

Comment on attachment 227989 [details] [diff] [review]
regression fix

r=sspitzer

Comment 25

[Asaf Romano (gone)]

Comment on attachment 227989 [details] [diff] [review]
regression fix

mozilla/toolkit/content/widgets/scrollbox.xml 1.10

Comment 26

[Bill Gianopoulos [:WG9s]]

Thanks.

Tested the fix.  Works great!

Comment 27

[Asaf Romano (gone)]

Attachment: Don't let the empty label take space

I've checked this in on trunk; will get a post-facto review.

Comment 28

[Mike Connor [:mconnor]]

Comment on attachment 228039 [details] [diff] [review]
Don't let the empty label take space

We don't need the same fix for pinstripe?

Comment 29

[Mike Connor [:mconnor]]

Let's get this cleaned up

Comment 30

[Asaf Romano (gone)]

Fixed on 1.8 branch for beta 1 (branch patch is on bug 318168).

Comment 31

[James Ross]

This bug's patch forgot to add the classes to the :hover and :hover:active blocks in scrollbar.css:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/themes/winstripe/global/scrollbox.css&rev=1.8&mark=94-103#80

I believe this is why there is NO hover feedback for the buttons in Windows without themes (e.g. Classic).

Comment 32

[James Ross]

In fact, it's much worse than that. I'm trying to make a patch, though, but it's horrible down here.

Comment 33

[James Ross]

Attachment: Don't override non-native theme for scrollbuttons

This makes sure that the autorepeatbutton visuals do not override the non-native visual appearance of toolbarbuttons that are used for scrollbuttons. In other works, it makes the tab scrolling buttons look right when there is no native theme in effect.

Comment 34

[Asaf Romano (gone)]

Comment on attachment 228434 [details] [diff] [review]
Don't override non-native theme for scrollbuttons

r=mano.

Comment 35

[Asaf Romano (gone)]

Silver, in future, please file separate bugs for regressions. Thanks for fixing this.

Comment 36

[James Ross]

Asaf, this isn't technically a regression, as the buttons didn't used to have borders at all. Also, reopening gets way more attention and I'm not risking this getting lost under the carpet.

I notice you've removed the second-review; does that mean I can land it on trunk?

Comment 37

[Asaf Romano (gone)]

(In reply to comment #36)
> I notice you've removed the second-review; does that mean I can land it on
> trunk?

Please do. 

Comment 38

[James Ross]

Comment on attachment 228434 [details] [diff] [review]
Don't override non-native theme for scrollbuttons

Landed on trunk.

Comment 39

[James Ross]

Comment on attachment 228434 [details] [diff] [review]
Don't override non-native theme for scrollbuttons

Landed on 1.8 branch.