On Mac: - mousing over the arrows that appears auto scrolls, way too fast to be able to read what's going by On Windows: - mousing over the arrows does nothing, clicking scrolls in increments - holding down the mouse button on the arrow does not continuously scroll What should happen, across platforms: - Clicking the button once scrolls by a little - Clicking and holding the button scrolls continuously - Mousing over does nothing

Ben, the mac problem is bug #342364. (Can you review the simple patch there?) I think all that is missing, once that lands, is that clicking and holding needs to scroll. > Clicking the button once scrolls by a little right now, a single click scrolls a tab at a time. Is that acceptable, or were you thinking something else for "a little"?

morphing ben's bug.

asaf, I've removed handleOnClick() and handleOnCommand() does all the work. I'm using onmousedown, onmouseup and onmouseout to figure out if we're click and holding. The reason for onmouseout is if I click and hold, and then mouse out of the arrow scrollbox, mousing back in should not continue to scroll. also note I'm scrolling by pixels, and not by index (aka by lines) so that behavior scrolling is similar between drag and drop (of tabs) and click and hold, and they both use the hidden toolkit.scrollbox.scrollIncrement hidden pref (which currently defaults to 20 px per interval) note, because it's an autorepeat button, scrolling by index/lines would also cause us to scroll very fast on click and hold.

> The reason for onmouseout is if I click and hold, and then mouse > out of the arrow scrollbox, mousing back in should not continue to scroll. ...which is like scrollbars. when in "clicktoscroll" mode, the arrowscrollbox is very much like a scrollbar, but without the bar or the thumb.

Seth: I would really like to land the clicktoscroll binding rewrok before we fix this bug, so we could just override the methods inside the new binding, and not change the already-shipped binding way too much.

Also, I think the way you want to fix this is by: 1) adding an onmousedown handler on the toolbarbuttons (once the new binding lands). 2) onmousedown, start a TYPE_REPEATING_SLACK timer, and do the actual scrolling in the notify callback (you need to implement nsITimerCallback for this binding), this makes it possible for you to define how much and how often to scroll. 3) Cancel the timer onmouseup and onmousedown You can see a very similar solution for the fade-in timer in preferences.xml. Feel free to ping me on IRC if you need any help.

Comment on attachment 227899 [details] [diff] [review] patch Minusing this for now, mostly because this implementation doesn't allow us to control and tweak the scrolling behavior.

> I would really like to land the clicktoscroll binding rewrok before we fix this bug... OK. Making this bug depend on bug #343097. I'll apply your patch for that bug locally and then work on a new fix. > the way you want to fix this is by... Thanks for the info about TYPE_REPEATING_SLACK timers. I'll investigate and work on a new solution.

comments on my patch: 1) the code in the dtor and the document check in the notify() method follow the timer code in preferences.xml 2) I'm scrolling by index (one at a time, when the timer fires) instead of by pixel. 3) I'm canceling the timer on mouse out for the following reason: click and hold the button and before you release, move the mouse out of the button. release the button when you are outside the button. when you release, the button will not get the onmouseup, so it will continue to scroll. asaf, can you review my patch?

there's a small problem with my patch. I need to scroll by index immediately in startScroll() before firing the timer. otherwise, if you mousedown and mouseup too quick (you click too quickly), you'll cancel the timer and never scroll! new patch coming.

See also bug 331055 which adds a repeat="press" option to the autorepeatbutton.

Comment on attachment 227994 [details] [diff] [review] updated patch, scroll immediately on mouse down. I had to apply this manually ;) IMHO the delay shouldn't be more than 100ms, but this belongs to mconnor/betlzner ui-r. Other than that: >Index: toolkit/content/widgets/scrollbox.xml >=================================================================== >+ <implementation implements="nsITimerCallback"> >+ <constructor><![CDATA[ >+ var pb2 = >+ Components.classes['@mozilla.org/preferences-service;1']. >+ getService(Components.interfaces.nsIPrefBranch2); nit: make this >+ var pb2 = Components.classes['@mozilla.org/preferences-service;1'] >+ .getService(Components.interfaces.nsIPrefBranch2); >+ try { >+ this._scrollDelay = pb2.getIntPref("toolkit.scrollbox.scrollDelay"); >+ } Rename this pref to something like toolkit.scrollbox.clickToScroll.scrollDelay (I don't think we're going to support it in the autorepeat based binding). >+ catch (ex) { >+ this._scrollDelay = 200; Just keep the catch block empty, you've already set the fallback value: >+ <field name="_scrollDelay">200</field> >+ <method name="notify"> >+ <parameter name="aTimer"/> >+ <body> >+ <![CDATA[ >+ if (!document) >+ aTimer.cancel(); >+ This doesn't make sense to me, but as you noted this is also done in the preferences bindings (with "new options dialog" blame comment). Keep this here but please file a bug on this (for both bindings). >+ >+ <method name="startScroll"> ... >+ </method> >+ >+ <method name="stopScroll"> ... >+ </method> >+ Please prefix those with an underscore. Nice work otherwise, r=mano. Please get beltzner/mconnor's ui-r on the default delay value.

Comment on attachment 227994 [details] [diff] [review] updated patch, scroll immediately on mouse down. this should be very safe and we want to get this before Patch Wednesday :)

I'll go log a new bug about the "if (!document)" check in this code and preferences.xml, and I'll log a new bug about the delay (should it be 200, 100, something else?)

fixed on the trunk. > IMHO the delay shouldn't be more than 100ms, but this belongs to > mconnor/betlzner ui-r. see bug #343587 which tracks this issue > This doesn't make sense to me, but as you noted this is also done in the > preferences bindings (with "new options dialog" blame comment). Keep this here > but please file a bug on this (for both bindings). see bug #343586 which tracks this issue

When I just load a bunch of tabs my first impression is that the scroll button including background is visually wider on the left side than on the right side. In real it is only 1 pixel wider on the left side but for some reason it looks more.

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