347363 - Implement smooth scroll for the tab bar

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P2)

Description

[Michael Ventnor]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060802 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060802 Minefield/3.0a1

I've spent the best part of 9 hours straight working on implementing smooth scrolling in an overflown tab bar. Patch coming soon.

Reproducible: Always

Comment 1

[Michael Ventnor]

Attachment: Patch

Here's the patch. After installing it, activate tab overflow and play around with it. I think you'll like it. If you want to turn it off, set browser.tabs.useLegacyScroll to true.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

dogcow:~/dev/moz-18/mozilla beltzner$ patch -p0 --dry-run <../../patch/smoothScroll.patch 
patching file toolkit/content/widgets/scrollbox.xml
patch: **** malformed patch at line 135:            if (!this._scrollTimer) 

Maybe bitrotted a little? I'd love to try this out!

Comment 3

[Michael Ventnor]

Attachment: Another patch

I don't know how what happened. Here's another patch.

Comment 4

[Michael Ventnor]

Attachment: Patch with a bug fix

I've actually found a bug after further testing, and this patch has a fix for it. Use this patch instead. :)

Comment 5

[Peter Kasting]

Michael, did you read my pri 3 items around smooth scrolling from the tab overflow design doc ( https://bugzilla.mozilla.org/attachment.cgi?id=229310 items 3G - 3I)?  If so, can you say which ones this does and doesn't implement?  If not, could you check them out?

Comment 6

[Michael Ventnor]

Attachment: Patch implementing 3H

"Michael, did you read my pri 3 items around smooth scrolling from the tab
overflow design doc ( https://bugzilla.mozilla.org/attachment.cgi?id=229310
items 3G - 3I)?  If so, can you say which ones this does and doesn't implement?
 If not, could you check them out?"

No, I thought I had a unique idea. :(
3G was my initial intention. All of 3G is implemented. However, there isn't a defined max speed. Instead, it starts at a peak speed and decelerates until it reaches the selected tab.

3H is all implemented EXCEPT for "If it cannot be decelerated in time to be fully onscreen, the scroll direction reverses as needed so that the tab decelerates back onto the screen." I can't figure out a way to do this, and even then the way 3H is handled in this patch this case will be very, very rare. If it does happen 90% of the tab will still be visible in the worst case scenario (yes, I did calculate that :) ).

3I was there and I didn't even do anything. O_O Well, actually I did replace the functions of the scroll buttons but when I did I never thought about this. I guess I was just lucky this time.

Comment 7

[Michael Ventnor]

Attachment: Final patch

OK, now that I've toyed with the code a bit more, I can safely say I have filled all (or maybe almost all?) of the criteria 3G-I. In addition, I've added some more checks and rearranged a few things to make the smooth scrolling mechanism suitable as an API call. I've stress-tested this as much as I could and I reckon this patch is ready for review and trunk-baking.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Michael,

This is great work, thanks much for your time and effort. I'm gonna pick at some nits here, since I think that animation is something we definitely want to do more of, but it's something we want to make sure that we get absolutely right. I'm playing with a Mac build at the moment, so please do let me know if these issues are mac-only (which is entirely possible!):

- The tab closest to the scroll arrow on which the user is clicking should never end up truncated after a scroll animation. Right now it's possible to release the scroll button and end up with a cut off tab right next to it; we should complete the animation and always show that full tab.

- Selecting a tab using accel-1 through accel-9 or by the "All Tabs" drop down should cause the strip to scroll over quickly to that position (this is 3H, isn't it?)

- The acceleration doesn't feel fast enough to me; I think we can safely ramp it up to about 1.5 times where it is right now

- The animation doesn't play when I use the mousewheel to scroll the tabstrip

Again, this is a killer change, and definitely one for the better. I'm liking what I'm seeing so far!

Comment 9

[Michael Ventnor]

Attachment: Patch implementing some of Mike's points

Points 1 & 3 are in this patch here. :)
As for points 2 & 4, it WFM, however remember that the tabbar will only scroll if the desired tab is off screen. Try mouse scrolling to an off screen tab and it should work. Try pressing Accel-9 while the last tab is off screen and it should work. Select any tab that isn't fully on-screen using any method and it should scroll.

Comment 10

[Ian Pottinger]

Just out of curiosity I want to ask, can the tabbar scrolling code in this patch be utilized by the code being developed in Bug 320638 or should the two bugs implement their own scrolling code.

Comment 11

[Peter Kasting]

(In reply to comment #8)
> Michael,
> 
> This is great work, thanks much for your time and effort.

I wanted to echo this, even though I haven't played with the patch.  Thanks a lot for writing this patch :)

I did notice your useLegacyScroll pref in the diff.  Is that for testing purposes or did you intend that to remain in the final version?  I'd rather not leave the old code in with prefs to control it unless we know there's a reason why we might want it.

Comment 12

[Tony Mechelynck [:tonymec]]

(In reply to comment #11)
[...]
> I did notice your useLegacyScroll pref in the diff.  Is that for testing
> purposes or did you intend that to remain in the final version?  I'd rather not
> leave the old code in with prefs to control it unless we know there's a reason
> why we might want it.
> 

When scrolling the page vertically, the user has the choice to use smooth scrolling or not (on Preferences "Advanced" tab). Why not let him decide to smooth-scroll the tab bar, or not to?

Comment 13

[Michael Ventnor]

(In reply to comment #11)
> (In reply to comment #8)
> > Michael,
> > 
> > This is great work, thanks much for your time and effort.
> 
> I wanted to echo this, even though I haven't played with the patch.  Thanks a
> lot for writing this patch :)
> 
> I did notice your useLegacyScroll pref in the diff.  Is that for testing
> purposes or did you intend that to remain in the final version?  I'd rather not
> leave the old code in with prefs to control it unless we know there's a reason
> why we might want it.
> 

While I tried my very best to make this code as efficient as possible, I realise that not everyone has a GHz computer. This option stays here so that Firefox doesn't slow to a crawl on older machines.
While running the task manager and scrolling, my 2GHz computer shows 11% usage on Firefox at the most. I would never bloat Fx with old code like Windows, however I still think its wise to keep in the old scrolling code available at this time. Some people may not want this smooth scroll anyway.

Thanks for your kind words though. Now I just need the all-clear from Mike to get a review done. :)

Comment 14

[Michael Ventnor]

Attachment: Patch with small fixes

Comment 15

[Dão Gottwald [:dao]]

Is this supposed to make Fx2?

Comment 16

[Michael Ventnor]

(In reply to comment #15)
> Is this supposed to make Fx2?
> 

No, it isn't designed to cater for the new theme. I only want this to make trunk, but I want it in trunk as soon as possible.

Comment 17

[Michael Ventnor]

Attachment: Hopefully final patch

I've finished making a patch for this unless I change my mind about something again. Hopefully this will land now.

Comment 18

[Ria Klaassen (not reading all bugmail)]

*** Bug 356195 has been marked as a duplicate of this bug. ***

Comment 19

[Michael Ventnor]

Comment on attachment 239474 [details] [diff] [review]
Hopefully final patch

Since the new theme is on trunk, my patch won't work since scrollBy has changed dramatically, and I don't know how to work around it.

Comment 20

[Dão Gottwald [:dao]]

CCing sspitzer, because he implemented most of the new tab strips infrastructure, AFAIK.

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #19)
> Since the new theme is on trunk, my patch won't work since scrollBy has changed
> dramatically

What exactly doesn't work? I see some bugs, but the basic click-to-scroll functionality works for me.

Comment 22

[Dão Gottwald [:dao]]

Attachment: updated patch

that's basically the same as attachment 239474 [details] [diff] [review] with just two nits fixed: [] instead of new Array and var added.

Comment 23

[Dão Gottwald [:dao]]

Attachment: patch, complete overhaul

Comment 24

[Dão Gottwald [:dao]]

(In reply to comment #19)
> Since the new theme is on trunk, my patch won't work since scrollBy has changed
> dramatically, and I don't know how to work around it. 

Btw, the actual problem was var mTabs = this.parentNode.childNodes;.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Dao, Michael: this is great! I'm noticing one strange thing when I use the mousewheel to scroll the tabstrip: if I don't go all the way to the edge with my scroll action, then the tabstrip seems to slowly crawl along to some arbitrary point before it stops. This is on OSX. Any ideas?

Comment 26

[Dão Gottwald [:dao]]

(In reply to comment #25)
> use the mousewheel to scroll the tabstrip

Does that mean switching tabs or really scrolling? Is that platform-specific?

> then the tabstrip seems to slowly crawl along to some arbitrary point
> before it stops.

I think that's the same what happens here when using ctrl+9 (going to the last tab) and then ctrl+1 (going to the first tab). Sometimes the scrolling works as expected, sometimes it doesn't. There must be a state that I didn't reset properly, or Michael already didn't.

Comment 27

[Dão Gottwald [:dao]]

Attachment: more cleanup

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #26)
> (In reply to comment #25)
> > use the mousewheel to scroll the tabstrip
> 
> Does that mean switching tabs or really scrolling? Is that platform-specific?

It's just scrolling, I think, not selecting. And I can't recreate it. I'll keep trying.

> I think that's the same what happens here when using ctrl+9 (going to the last
> tab) and then ctrl+1 (going to the first tab). Sometimes the scrolling works as
> expected, sometimes it doesn't. There must be a state that I didn't reset
> properly, or Michael already didn't.

Does the latest clean up patch address this?

Comment 29

[Dão Gottwald [:dao]]

Yes, should be fixed, as well as all other bugs that I could notice.
I've also made it possible to click the scroll buttons multiple times while scrolling.

Comment 30

[Dão Gottwald [:dao]]

Attachment: resize fix

the previous patch regressed window resizing behaviour

Comment 31

[Dão Gottwald [:dao]]

Attachment: patch, scroll faster

after using this for some time, I think the scrolling should be faster and more fluid.

with this patch, scrolling starts immediately instead of after 100ms and the effect frame length is 60ms instead of 100ms.

Comment 32

[Michael Ventnor]

Heh, for a while there I thought this bug was dead. Thanks Beltzner for blogging about this to bring it back to everyone's attention and thank you Dao for continuing work on this. :)

> Btw, the actual problem was var mTabs = this.parentNode.childNodes;.
> 

Silly bugger...

> with this patch, scrolling starts immediately instead of after 100ms 

Good catch.

> and the effect frame length is 60ms instead of 100ms.

Hm, 16 redrawings per second. Did you notice a higher CPU usage during the 16fps scroll than the 10fps one? I'm worried that if this animation regresses performance too much this will be touted as unnecessary bloat rather than a usability-enhancing animation. Then again we still do have the pref...

I'll test this myself later. Once again thanks to both of you!

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #32)
> Hm, 16 redrawings per second. Did you notice a higher CPU usage during the
> 16fps scroll than the 10fps one? I'm worried that if this animation regresses
> performance too much this will be touted as unnecessary bloat rather than a
> usability-enhancing animation. Then again we still do have the pref...

My system isn't adequate for testing performance -- too much CPU noise. If it is a significant worse performance, I think we should at least change |scrollAmount = round(amountToScroll / 3)| to |scrollAmount = round(amountToScroll / 2)| or so. scrollToTab (was snapToTab, I think) is too slow compared to "legacy scroll" (e.g. with 50 tabs, press Ctrl+1 and then Ctrl+9), and that could be considered bloat. Also, 10 fps didn't really feel "smooth" to me.

> I'll test this myself later.

Yes, please -- I hope there are no remaining regressions due to my changes.

Comment 34

[Michael Ventnor]

>+            //XXX not sure why this is needed
>+            if (aTab != this.tabContainer.firstChild)
>+              amountToScroll++;

I don't know why either since it wasn't in my original implementation. I deleted this bit without any troubles so why did you put this in the first place?

If you feel it is needed, maybe you could remove the if and just apply it anyway? There is code in the smooth scroll function to deal with distances that go past the end of the tab bar. But I don't think this is needed at all, 1 pixel is insignificant, and the more code we can shave the better.

+          function scrollBy(self, scrollAmount) {
+            if (scrollAmount <= 1 && scrollAmount >= -1)
+              self._isScrolling = 0;
+            self.scrollBoxObject.scrollBy(scrollAmount, 0);
+          }

Maybe make those 2 and -2? If they're too close to 0 then we'll cause the brake effect when we don't want to. If they're too far away though then the brake effect will almost never happen.

> My system isn't adequate for testing performance -- too much CPU noise. If it
> is a significant worse performance, I think we should at least change
> |scrollAmount = round(amountToScroll / 3)| to |scrollAmount =
> round(amountToScroll / 2)| or so. scrollToTab (was snapToTab, I think) is too
> slow compared to "legacy scroll" (e.g. with 50 tabs, press Ctrl+1 and then
> Ctrl+9), and that could be considered bloat. Also, 10 fps didn't really feel
> "smooth" to me.

On the contrary. Since we are starting at a peak speed and slowing down by dividing by a constant, the scrolling time should be the same whether we have 5 tabs or 50 tabs. Or have I just lost my way with maths. :(

BTW, snapToTab was used to make the whole tab visible once the user finished scrolling with the buttons (Comment 8 Point 1). It was a bit of a hack, your implementation is much, much better.

> Yes, please -- I hope there are no remaining regressions due to my changes.
> 

All lights are green here. CPU never went up above 65 when I tried an extreme case (Ctrl-1, Crtl-9, Ctrl-1, Crtl-9, Ctrl-1, Crtl-9, Ctrl-1, Crtl-9... ;) ) on my 2GHz 512MB machine, which is much less CPU than what is needed for the scrollbar smooth scroll in IE7 (and I presume Firefox) so I think we have a pretty efficient algorithm so far.

Comment 35

[Michael Ventnor]

Actually I just found a big problem in your patch:

+      <property name="useLegacyScroll" readonly="true">
+        <getter><![CDATA[
+          if (this._useLegacyScroll == null) {
+            try {
+              this._useLegacyScroll = this.prefBranch.getBoolPref("browser.tabs.useLegacyScroll");
+            }
+            catch (ex) {
+            }
+          }
+          return this._useLegacyScroll;
+        ]]></getter>
+      </property>

This means that flipping the pref won't change between the scroll types because the pref is cached from the start. Maybe you could add a pref observer?

Comment 36

[Dão Gottwald [:dao]]

(In reply to comment #34)
> If you feel it is needed, maybe you could remove the if and just apply it
> anyway? There is code in the smooth scroll function to deal with distances that
> go past the end of the tab bar. But I don't think this is needed at all, 1
> pixel is insignificant, and the more code we can shave the better.

I don't know why, but it was only needed there. And one pixel is significant. If a 1px row of the destination tab is left off-screen, it will remain the scroll destination the next time you hit the button. Even worse, it will scroll zero pixels then, because the pixel that should be scrolled is missing again. I did experience this.

> Maybe make those 2 and -2? If they're too close to 0 then we'll cause the brake
> effect when we don't want to.

Could you elaborate on "brake effect"?

> On the contrary. Since we are starting at a peak speed and slowing down by
> dividing by a constant, the scrolling time should be the same whether we have 5
> tabs or 50 tabs. Or have I just lost my way with maths. :(

Every time the distance grows by 50%, you will need one more frame (assuming scrollAmount = round(amountToScroll / 3)), thus scrolling 50 tabs does in fact take longer. I'd like to avoid that, but my point was another one. I was comparing to the legacy scroll case, i.e.: if you switch from the first to the last tab using a shortcut, the bar scrolls the whole distance immediately; there's no delay at all.

(In reply to comment #35)
> This means that flipping the pref won't change between the scroll types because
> the pref is cached from the start. Maybe you could add a pref observer?

I cached the value because calling getBoolPref() for each access to .useLegacyScroll isn't appropriate. An observer could be added, but I wouldn't do that. It's only a hidden pref and the user isn't expected to toggle it all day long.

Comment 37

[Michael Ventnor]

> I don't know why, but it was only needed there. And one pixel is significant.
> If a 1px row of the destination tab is left off-screen, it will remain the
> scroll destination the next time you hit the button. Even worse, it will scroll
> zero pixels then, because the pixel that should be scrolled is missing again. I
> did experience this.

OK then.

> Could you elaborate on "brake effect"?

https://bugzilla.mozilla.org/attachment.cgi?id=229310 Item 3H
Its what happens when a tab that is currently visible is selected during a scroll; the sudden deceleration in scrollToTab. It will only happen if _isScrolling is not 0. However if the values are too low then we may cause this effect when the tab bar is about to stop anyway. Try continually scrolling the mouse wheel back and forth on the tab bar during a scroll and you may see the tab bar scrolling at a very slow rate, thats many of these "brake effects" happening which we don't want to. I think 2 is a much more reasonable value so this doesn't happen as much.

> Every time the distance grows by 50%, you will need one more frame (assuming
> scrollAmount = round(amountToScroll / 3)), thus scrolling 50 tabs does in fact
> take longer. I'd like to avoid that, but my point was another one. I was
> comparing to the legacy scroll case, i.e.: if you switch from the first to the
> last tab using a shortcut, the bar scrolls the whole distance immediately;
> there's no delay at all.

As long as the content is displayed immediately I'm sure the user won't mind the extra scroll time, providing the scrolling is fluid and reasonably fast like what your last patch made it.

> I cached the value because calling getBoolPref() for each access to
> .useLegacyScroll isn't appropriate. An observer could be added, but I wouldn't
> do that. It's only a hidden pref and the user isn't expected to toggle it all
> day long.
> 

I see. I just hope this value gets documented as needing a restart so we don't get bugs saying the pref doesn't work.

Comment 38

[Dão Gottwald [:dao]]

Attachment: some cleanup ... again

Alright, I've changed quite some stuff, but I'm too tired to explain it en détail.
Most important: scrolling while holding the mouse button down works differently. I like it more.

(In reply to comment #37)
> > I don't know why, but it was only needed there.
> 
> OK then.

I managed to remove it.
Don't ask how, as I still don't understand it.

> Its what happens when a tab that is currently visible is selected during a
> scroll; [...] Try continually scrolling the
> mouse wheel back and forth on the tab bar during a scroll and you may see the
> tab bar scrolling at a very slow rate

I couldn't reproduce that, please test if it's still happening with that patch.

> I see. I just hope this value gets documented as needing a restart so we don't
> get bugs saying the pref doesn't work.

Many hidden prefs need a restart, that's quite common (and mainly undocumented, I think). This is a case where I as a user really wouldn't expect something else.

Comment 39

[Michael Ventnor]

(In reply to comment #38)
> Alright, I've changed quite some stuff, but I'm too tired to explain it en
> détail.
> Most important: scrolling while holding the mouse button down works
> differently. I like it more.

I don't. I think a smooth and consistent flow is a lot better and more pleasing than the scrolling in this patch.
Also, call me crazy, but I think that 2.5 makes it too fast. I liked it more when it was at 3.
Your old patch was much, much better IMO.

Comment 40

[Michael Ventnor]

One more thing:

(In reply to comment #38)
> I couldn't reproduce that, please test if it's still happening with that patch.

Yes it is, but I wouldn't lose my hair over it. I was stress-testing this with an edge case and it is VERY unlikely for a user to do what I was doing.

Comment 41

[Dão Gottwald [:dao]]

(In reply to comment #39)
> I don't. I think a smooth and consistent flow is a lot better and more pleasing
> than the scrolling in this patch.

I found it hard(er) to recognize tabs. That was actually a regression from the legacy case.

> Also, call me crazy, but I think that 2.5 makes it too fast. I liked it more
> when it was at 3.

I set it to 2 first, and that was really too fast. 2.5 I found pleasing, but I'll compare it again to 3.

> (In reply to comment #38)
> Yes it is, but I wouldn't lose my hair over it. I was stress-testing this with
> an edge case and it is VERY unlikely for a user to do what I was doing.

Nevertheless, it speaks for the cleanup. Whatever we do, I think we should do it based on the last patch.

Comment 42

[Michael Ventnor]

(In reply to comment #41)
> I found it hard(er) to recognize tabs. That was actually a regression from the
> legacy case.

I'd consider it a feature ;) In all seriousness, please consider reverting back to the older behaviour, I found it a lot more streamlined and I think the users would prefer this behaviour too. IMO, the current behaviour slows things down enough so that it actually becomes a hindrance. I could still recognise tabs with the old behaviour.

> I set it to 2 first, and that was really too fast. 2.5 I found pleasing, but
> I'll compare it again to 3.

If the animation goes too fast then we'll have some users who think "what the hell just happened". My dear mother is in that camp with almost everything she uses ;) Give the animation enough time for the user's brain to figure it out.

Comment 43

[Dão Gottwald [:dao]]

I went from 3 to 2.5 also because I thought my change made it too slow. Now I don't really understand how the animation can go too fast and how it can slow things down at the same time.

> I'd consider it a feature ;) In all seriousness, please consider reverting back
> to the older behaviour, I found it a lot more streamlined and I think the users
> would prefer this behaviour too.

Let's just disagree here. :)  Beltzner will have to decide, I suppose.
I think the older behaviour's implementation was flawed and should be rewritten *if* we go back.

Comment 44

[Michael Ventnor]

Removing helpwanted keyword since we are back on track.

Comment 45

[Michael Ventnor]

(In reply to comment #43)
> Now I don't really understand how the animation can go too fast and how it can slow
> things down at the same time.

From the last patch I felt it takes too long to scroll through the tab bar using the scroll buttons due to the slight stop at each tab, but a fly-over to a specific tab using Ctrl+9 or the All Tabs menu felt too fast. Maybe I'm just a paranoid perfectionist :) but as Mike Beltzner said we need to make sure we get this animation absolutely 100% perfect the first time.

Comment 46

[Neil Deakin]

Why are you putting tab/tabbrowser related functionality in <arrowscrollbox>? It doesn't belong there.

Comment 47

[Dão Gottwald [:dao]]

(In reply to comment #46)
> tab/tabbrowser related functionality

what exactly?

Comment 48

[Neil Deakin]

For one thing, the 'scrollToTab' method. Second, the property called 'tabContainer'. Third, various functions which refer to tabs and/or assume that the <arrowscrollbox> is being used inside a <tabs>.

Comment 49

[Dão Gottwald [:dao]]

(In reply to comment #48)
> For one thing, the 'scrollToTab' method. Second, the property called
> 'tabContainer'.

You can call scrollToTab scrollToElement, it doesn't matter.
tabContainer can be renamed as well and the implementation can be changed easily to catch the nodes in .scrollbox-inner.

> Third, various functions which refer to tabs and/or assume that
> the <arrowscrollbox> is being used inside a <tabs>.

Again, details please.

Comment 50

[Neil Deakin]

What other details do you want? The patch clearly adds and changes numerous functions with references to tabs. The word 'tab' should not appear anywhere in scrollbox.xml

Comment 51

[Dão Gottwald [:dao]]

If the word "tab" is your only concern, please re-read my previous comment. The UI is yet to be reviewed, and variable names don't really matter for that.

Comment 52

[Dão Gottwald [:dao]]

Attachment: patch, acknowledging that a scrollbox doesn't necessarily contain tabs; no behavioural change

Comment 53

[Dão Gottwald [:dao]]

Comment on attachment 262769 [details] [diff] [review]
patch, acknowledging that a scrollbox doesn't necessarily contain tabs; no behavioural change

Neil, I see that you're a toolkit peer and your queue happens to be empty. You've already commented on the older patch, so will you review this one?

Comment 54

[Dão Gottwald [:dao]]

Btw, I realize that the latest patch could become obsolete, given the pending ui-review. I'm pleased with the current behaviour and hopefully beltzner will agree. However, I'll leave it to Neil or some other peer to decide if reviewing now is worthwhile.

Comment 55

[Neil Deakin]

Comment on attachment 262769 [details] [diff] [review]
patch, acknowledging that a scrollbox doesn't necessarily contain tabs; no behavioural change


+      <field name="_prefBranch"/>
+      <property name="prefBranch" readonly="true">


__prefBranch and _prefBranch here
 
-      <field name="_scrollBoxObject">null</field>
+      <field name="_legacyScroll">null</field>
+      <property name="legacyScroll" readonly="true">
+        <getter><![CDATA[
+          if (this._legacyScroll == null) {
+            try {
+              this._legacyScroll = this.prefBranch.getBoolPref("toolkit.scrollbox.legacyScroll");
+            } catch(e) {
+              this._legacyScroll = false;
+            }
+          }
+          return this._legacyScroll;
+        ]]></getter>
+      </property>

I'd rather see 'smoothScroll' rather than 'legacyScroll'. Otherwise, this implies that the non-smooth scroll method is only there for compatibility.
Also, a setter might be good to allow the setting to be overriden, possibly with a mapped smoothscroll attribute.

+
+      <field name="_scrollBoxObject"/>
       <property name="scrollBoxObject" readonly="true">
         <getter><![CDATA[
           if (!this._scrollBoxObject) {
             this._scrollBoxObject =
               this._scrollbox.boxObject
                              .QueryInterface(Components.interfaces.nsIScrollBoxObject);
           }
           return this._scrollBoxObject;
         ]]></getter>
       </property>
 
+      <field name="_elementContainer"/>
+      <property name="elementContainer" readonly="true">
+        <getter><![CDATA[
+          if (!this._elementContainer)
+            this._elementContainer = document.getBindingParent(this);
+          return this._elementContainer;
+        ]]></getter>
+      </property>
+

I don't think this needs to be cached.
 
       <method name="ensureElementIsVisible">
...
+
+          if (elementStart < containerStart) {
+            var amountToScroll = elementStart - containerStart;
+          } else if (containerEnd < elementEnd)
+            var amountToScroll = elementEnd - containerEnd;
+          else if (this._isScrolling) {
+            // decelerate if a currently-visible element is selected during the scroll
+            var amountToScroll = this._isScrolling * STOP_DISTANCE;

Just declare amountToScroll once, not three times.

+
+            // If the sudden deceleration will cause the element to go over the edge,
+            // scroll in the opposite direction instead.
+            if (elementStart - STOP_DISTANCE < containerStart || elementEnd + STOP_DISTANCE > containerEnd)
+              amountToScroll *= -1;

Not sure what this is supposed to be doing. Wouldn't it be better to just reduce the amountToScroll to the distance remaining?

+          } else
+            return;
+

Use the same brace style consistently. I suggest putting braces around all four blocks.
 
       <method name="scrollByIndex">
         <parameter name="lines"/>

The argument should be changed to 'index' rather than 'lines' as horizontal scrolling is also supported.

         <body><![CDATA[
-          this.scrollBoxObject.scrollByIndex(lines);
+          if (lines == 0)
+            return;
+          var nextElement;
+          var elements = this.elementContainer.childNodes;

This should use this.childNodes if there are any, and use this.elementContainer.childNodes otherwise.

+
+          while (lines < 0 && nextElement) {
+            targetElement = nextElement;
+            nextElement = nextElement.previousSibling;
+            lines++;
+          }
+          while (lines > 0 && nextElement) {
+            targetElement = nextElement;
+            nextElement = nextElement.nextSibling;
+            lines--;
+          }

Not a big deal for now, but it should be iterating in visible order, not dom order.

An actual issue though is whether this code is correct for rtl?

These both also apply to the other methods you added.

+      <field name="_isScrolling">0<!-- 0=idle, 1=now scrolling right, -1=now scrolling left --></field>

Put this comment before the <field>.
 
+      <field name="_allCurrentScrollEffects">[]</field>
+
+      <method name="haltSmoothScroll">
+        <body><![CDATA[
+          // Stop a currently-happening smooth scroll effect.

I'd reword to 'Stop a smooth scroll effect that is currently happening'

+          var howManyToStop = this._allCurrentScrollEffects.length;
+          for (var i = 0; i < howManyToStop; i++) {
+            clearTimeout(this._allCurrentScrollEffects[0]);
+            this._allCurrentScrollEffects.shift();
+          }

Just use clearTimeout(this._allCurrentScrollEffects.shift())

+      <method name="smoothScroll">
...
+
+          function scrollBy(self, scrollAmount) {
+            if (scrollAmount <= 1 && scrollAmount >= -1)
+              self._isScrolling = 0;

Is this supposed to call haltSmoothScroll?

+            self.scrollBoxObject.scrollBy(scrollAmount, 0);
+          }
+
+          // amountToScroll = total distance to scroll
+          // scrollAmount = distance to move during the particular effect frame (60ms)
+          for (var delay = 0; this._isScrolling < 0 && amountToScroll < 0 ||
+                              this._isScrolling > 0 && amountToScroll > 0; delay += 60) {
+            amountToScroll -= (scrollAmount = round(amountToScroll / 2.5));
+            this._allCurrentScrollEffects.push(setTimeout(scrollBy, delay, this, scrollAmount));
+          }

This is an interesting way to do this, but wouldn't it be easier to use setInterval, and have an array of amountToScroll values instead?
 
-      <field name="_scrollTimer">null</field>
       <field name="_scrollLines">0</field>
       <field name="_scrollDelay">150</field>
+      <field name="_scrollTimer"/>

Why this change?
 

General comments:

 - the code assumes that the arrowscrollbox is going to be horizontally arranged. The arrowscrollbox used in popups, for instance, is vertical.
 - unless you expect added properties/methods to be part of the public api, they should be preceeded by an underscore:
     prefBranch, elementContainer, haltSmoothScroll, smoothScroll
    If the latter are intended to be public apis, scrollByPixelsSmooth and stopScroll are better names.
 - there are several places you access 'scrollBoxObject' when 'boxObject' is sufficient.

Comment 56

[Dão Gottwald [:dao]]

Attachment: patch x+1, tested with rtl

(In reply to comment #55)
>  - there are several places you access 'scrollBoxObject' when 'boxObject' is
> sufficient.

It seems that boxObject contains the scroll buttons while scrollBoxObject doesn't, which means that its position and dimensions are different.

Other than that, I think I've addressed all your comments.

Comment 57

[Neil Deakin]

(In reply to comment #56)
> 
> It seems that boxObject contains the scroll buttons while scrollBoxObject
> doesn't, which means that its position and dimensions are different.

Can't be, scrollBoxObject inherits from boxObject and doesn't override any of the properties. The only effect the latter should have is to have an extra QI that isn't necessary.

Comment 58

[Dão Gottwald [:dao]]

Hm, yes, that's what I thought too ... yet it didn't work with boxObject. That is, it did scroll, but not far enough. I can try it again in two days or so, but for now I'm off.

Comment 59

[Michael Ventnor]

(In reply to comment #55)
>  - unless you expect added properties/methods to be part of the public api,
> they should be preceeded by an underscore:
>      prefBranch, elementContainer, haltSmoothScroll, smoothScroll
>     If the latter are intended to be public apis, scrollByPixelsSmooth and
> stopScroll are better names.

Yes, that was my original intention when I wrote the first implementation, so extensions (and maybe future Moz patches) can smooth-scroll to precisely wherever they want.

Comment 60

[Michael Ventnor]

Attachment: New patch

Using plain boxObject WFM so heres a patch which uses it. It also gives the public API clearer names (_stopScroll is already in scrollBox so to prevent confusion I used stopSmoothScroll).

Comment 61

[Michael Ventnor]

Attachment: New patch 1.1

Oops, forgot one _really_ important thing.

Comment 62

[Dão Gottwald [:dao]]

Attachment: patch x+2

ah, I think I was using this.boxObject :)
thanks for that update, Michael.

Your patch breaks rtl though. Also, I'm not fond of making "startScrollSmooth" and "stopSmoothScroll" part of the public API. I don't see a certain use case for that. And in case of doubt, the API should be kept lean and sleek. Adding these methods later is always an option, removing them not so.

Comment 63

[Dão Gottwald [:dao]]

(In reply to comment #62)
> Also, I'm not fond of making "startScrollSmooth"
> and "stopSmoothScroll" part of the public API.

Err, I meant "scrollByPixelsSmooth", not "startScrollSmooth".

Comment 64

[Michael Ventnor]

Comment on attachment 262988 [details] [diff] [review]
patch x+2

>+            amountToScroll -= (scrollAmount = round(amountToScroll / 2.5));

Can you change this to 2.6? I don't know why, but I found the scrolling a bit more pleasing at that value. At 2.5, I found the scrolling stopping a little bit too suddenly after decelerating, or maybe its just an illusion to me.

Comment 65

[Michael Ventnor]

OK, ignore my previous comment. I bumped it back to 2.5 in my private build and it seemed OK.

Comment 66

[Neil Deakin]

Comment on attachment 262988 [details] [diff] [review]
patch x+2

 
+      <property name="smoothScroll">
+        <getter><![CDATA[
+          if (!this.hasAttribute("smoothscroll")) {
+            try {
+              this.setAttribute("smoothscroll", this._prefBranch.getBoolPref("toolkit.scrollbox.smoothScroll"));
+            } catch(e) {
+              this.setAttribute("smoothscroll", false);
+            }

Break up the long line. Don't change the attribute if it isn't used, just return the right value. Also, the exception block should return true as smooth scrolling is the default.

+            const STOP_DISTANCE = 20;
+            if (this._isScrolling == -1 && elementStart - STOP_DISTANCE < containerStart)
+              amountToScroll = elementStart - containerStart;
+            else if (this._isScrolling == 1 && containerEnd + STOP_DISTANCE < elementEnd)
+              amountToScroll = elementEnd - containerEnd;

The second condition should subtract STOP_DISTANCE as well, no?

+          if (amountToScroll < 0) {
+            this._isScrolling = -1;
+            var round = Math.floor;
+          } else {
+            this._isScrolling = 1;
+            var round = Math.ceil;
+          }
+          var scrollAmount;

Use: var round, scrollAmount; before the conditions
 
       <method name="scrollByIndex">
-        <parameter name="lines"/>
+        <parameter name="index"/>
         <body><![CDATA[
-          this.scrollBoxObject.scrollByIndex(lines);
+          if (index == 0)
+            return;
+          if (this.getAttribute("orient") == "vertical") {
+            this.scrollBoxObject.scrollByIndex(index);
+            return;
+          }
+          if (!this._isLTRScrollbox)
+            index *= -1;
+          var nextElement;
+          var elements = this.childNodes.length ?
+                         this.childNodes :
+                         document.getBindingParent(this).childNodes;

Use this.hasChildNodes()

+            case "resize":
+              var width = this.mTabstrip.boxObject.width;
+              if (width != this.mTabstripWidth) {
+                this.adjustTabstrip();
+                if (this.mTabstrip.smoothScroll)
+                  this.mTabstrip.scrollByPixels(1);
+                else
+                  this.mTabstrip.scrollByIndex(1);

I didn't ask this before, but this looks the wrong way round to me.

Comment 67

[Dão Gottwald [:dao]]

(In reply to comment #66)
> +            const STOP_DISTANCE = 20;
> +            if (this._isScrolling == -1 && elementStart - STOP_DISTANCE <
> containerStart)
> +              amountToScroll = elementStart - containerStart;
> +            else if (this._isScrolling == 1 && containerEnd + STOP_DISTANCE <
> elementEnd)
> +              amountToScroll = elementEnd - containerEnd;
> The second condition should subtract STOP_DISTANCE as well, no?

Yes, I think so.

> +            case "resize":
> +              var width = this.mTabstrip.boxObject.width;
> +              if (width != this.mTabstripWidth) {
> +                this.adjustTabstrip();
> +                if (this.mTabstrip.smoothScroll)
> +                  this.mTabstrip.scrollByPixels(1);
> +                else
> +                  this.mTabstrip.scrollByIndex(1);
> I didn't ask this before, but this looks the wrong way round to me.

Is there a reason against using scrollByPixels in both cases? As far as I remember, it's just a hack anyway. scrollByIndex (which was used before) does feel a little bit odd no matter if it's smooth or not.

Comment 68

[Michael Ventnor]

Attachment: Patch x+3

Comment 69

[Dão Gottwald [:dao]]

Comment on attachment 263064 [details] [diff] [review]
Patch x+3

thanks again, Michael. but I have yet another (so far unmentioned) change in mind, thus removing review request. x+4 should follow shortly.

Comment 70

[Michael Ventnor]

Comment on attachment 263064 [details] [diff] [review]
Patch x+3

Fine by me. What's the record for most obsolete patches on any one bug? I wonder if this bug has beaten it :-)

Comment 71

[Dão Gottwald [:dao]]

Attachment: patch x+4

added _autorepeatSmoothScroll, which makes scrolling with the buttons slightly faster while theoretically requiring less CPU cycles.

btw, there are bugs where I contributed significantly more obsoletes ;)

Comment 72

[Dão Gottwald [:dao]]

Attachment: patch x+5

changed the construction of the scrollAmounts array.

e.g. for scrolling 100 px:
  was: [40,24,15, 9, 5, 3, 2, 1, 1]
   is: [20,20,20,20,10, 5, 3, 1, 1]

Comment 73

[Neil Deakin]

Comment on attachment 263160 [details] [diff] [review]
patch x+5

+      <property name="smoothScroll">
+        <getter><![CDATA[
+          if (this._smoothScroll === null) {
+            if (this.hasAttribute("smoothscroll")) {
+              this._smoothScroll = (this.getAttribute("smoothscroll") == "true");

Use:
  this._smoothScroll = (this.getAttribute("smoothscroll") != "false");
so that invalid values get treated as true.

+          var scrollAmount, scrollAmounts = [];
+          if (amountToScroll > 2 || amountToScroll < -2) {
+            scrollAmount = round(amountToScroll * 0.2);
+            scrollAmounts.push (scrollAmount, scrollAmount, scrollAmount);

No space after 'push' here

Comment 74

[Michael Ventnor]

(Dao, I tried emailing you back but it wouldn't work)
 
I have tested the latest patch and I like it. I don't feel strongly about the flow issue anymore since I'm not a heavy tab user, but tab-wise flow seems fine to me now. I was just concerned about those who claim to always have hundreds of tabs open (I still don't know why they'd need so many) but the all-tabs menu sort-of solves that problem and it will be even better once your double-click and triple-click implementations are landed (bug 357951).
 
Regards,
Michael

>Hi Michael,

>I'm curious: how's your current position regarding consistent vs.
>tab-wise flow? I assume you've tested the latest patch -- did you come
>in terms with it?

>Regards,
>Dao

Comment 75

[Mike Beltzner [:beltzner, not reading bugmail]]

Per PRD, not blocking, but definitely wanted. I owe you a UI-R here like you wouldn't believe, next week I'm out of the office which will give me more time to focus on that sort of stuff.

Comment 76

[Jay Patel [:jay]]

beltzner:  I'm waiting for this to land so I can start QA test planning.  Any chance this is going to be reviewed and landed soon?  

Comment 77

[Alex Faaborg [:faaborg] (Firefox UX)]

beltzner: I got a request on my blog to ping you about the ui-r.

Comment 78

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 263160 [details] [diff] [review]
patch x+5

OMG, I totally thought I approved this weeks ago. My session login must have expired and I didn't notice or something.

Comment 79

[Michael Ventnor]

Attachment: Update to tip

Lets finally finish this.

Comment 80

[Ryan Flint [:rflint] (ping via IRC for reviews)]

mozilla/toolkit/content/widgets/scrollbox.xml 	1.24 	mozilla/toolkit/content/widgets/tabbrowser.xml 	1.236 	mozilla/modules/libpref/src/init/all.js 	3.687

Comment 81

[Justin Kerk]

Been using this in the nightlies for the last few days, it looks nice when changing to a new tab using the all tabs drop-down (because the desired page is already shown instantly, the animation is just a visual flourish). But when clicking the arrows on the tab bar to move left or right by one tab at a time (which I do frequently when I know the desired tab is only one or two over), the tab bar now does this animation instead of moving over instantly, which makes it *much* more sluggish, particularly when scrolling more than once. IMO this effect should not be used for the left and right arrow buttons.

Comment 82

[Michael Ventnor]

(In reply to comment #81)
> But when
> clicking the arrows on the tab bar to move left or right by one tab at a time
> (which I do frequently when I know the desired tab is only one or two over),
> the tab bar now does this animation instead of moving over instantly, which
> makes it *much* more sluggish, particularly when scrolling more than once. IMO
> this effect should not be used for the left and right arrow buttons.

Bug 357951 proposes a solution, it just needs someone with CVS checkin rights to check in the patch on that bug.

Comment 83

[Alex Faaborg [:faaborg] (Firefox UX)]

Hey Michael, I'll be covering a lot of the stuff Beltzner was involved with for the next three weeks while he is on vacation.

The solution proposed in Bug 357951 is a nice additional feature for advanced users, but obviously won't be very discoverable.

The current behavior for single click-release on the left and right arrows works great, but I share the same concerns in comment #81, that clicking and holding is now less efficient. 

What if we changed the single click-hold-release behavior to this:

-on downclick the tab strip begins to scroll smoothly (adding acceleration would be really cool, but a constant velocity would be fine too)

-on upclick, tab strip continues to scroll a little so that the tab nearest the button hit is fully visible.

Comment 84

[Michael Ventnor]

Thats what one of the old patches did before Dao changed it for some reason. I begged for it to be changed back but eventually I stopped caring. See comment 39. I think the patch in comment 31 was the last one that showed that behaviour.

Since Beltzner's on vacation, does that mean you're qualified to give ui-r's for now? I could make a new patch that reinstates that behaviour (and as I've seen in the blogosphere a LOT of people really don't like the current behaviour)

Comment 85

[Alex Faaborg [:faaborg] (Firefox UX)]

>Since Beltzner's on vacation, does that mean you're qualified to give ui-r's for now?

No, only Beltzner and Mike Connor can do ui-r's, but mconnor will be taking my recommendations into account.

(in reply to comment #41)
>I found it hard(er) to recognize tabs. That was actually a regression from the
>legacy case.

I understand what you are getting at, but the overall scroll speed seems to be a more obvious regression than the ability to recognize individual tabs.

While the user is scrolling they are probably going to be looking for a particular favicon as opposed to the shape of the tab itself. And the final adjustment to make sure the nearest tab is visible should help with tab recognition.  The tabs may appear to lock into place when this occurs.

Comment 86

[Michael Ventnor]

Attachment: Use constant scroll

Restore the behaviour as described in comment 83.

Comment 87

[Michael Ventnor]

Comment on attachment 277511 [details] [diff] [review]
Use constant scroll

Actually, I think this could do with a new bug.

Comment 88

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'll echo comment 81 -- it's extremely frustrating to scroll by a few tabs since this change, since you lose track of any clicks after the first.  The animation also stutters a bit as well.  The double-click patch doesn't really fix this; it just adds a new way of scrolling, which is fine, but doesn't address this issue.

Comment 89

[Michael Ventnor]

Bug 393006 is for addressing this issue, first of all it needs a ui-review from mconnor. Then, Dao requested that the patch in bug 357081 land first because the codepaths collide.