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.

Taking.

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

Well, ignore comment 2.

Morphing this a bit to correspond my tree ;)

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

Comment on attachment 227541 [details] [diff] [review] patch r=sspitzer, this is much better and cleaner than how I did it originally.

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

> Would it be possible to make holding down the button scroll continuously see bug #342906

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 on attachment 227541 [details] [diff] [review] patch We need this on 181b1 asap if bug 342906 is still a (181b1) blocker.

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.

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.

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

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

> 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 on attachment 227964 [details] screen shot of the context menu with arrows ignore this screen shot.

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.

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.

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

Comment on attachment 227989 [details] [diff] [review] regression fix r=sspitzer

Comment on attachment 227989 [details] [diff] [review] regression fix mozilla/toolkit/content/widgets/scrollbox.xml 1.10

Thanks. Tested the fix. Works great!

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

Comment on attachment 228039 [details] [diff] [review] Don't let the empty label take space We don't need the same fix for pinstripe?

Let's get this cleaned up

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

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

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

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 on attachment 228434 [details] [diff] [review] Don't override non-native theme for scrollbuttons r=mano.

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

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?

(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 on attachment 228434 [details] [diff] [review] Don't override non-native theme for scrollbuttons Landed on trunk.

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