565783 - Eliminate bounce when scrolling tab bar with trackpad

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Alex Limi (:limi) — Firefox UX Team]

(Note: this is filed as part of the “Paper Cut” bugs — we assume that there may be multiple existing bugs on this. Please make them block this bug, and we will de-dupe if they are indeed exactly the same. Thanks!)

To reproduce:
1. Open lots of tabs, so you have to scroll the tab bar to see them all.
2. Scroll to the rightmost end of the collection of tabs, then use the mouse wheel to scroll all the way back to the left.
3. Now, keep scrolling, and notice how the scroll never seems to stop — even when you're at the end, you can invoke more scrolling that just resets some position so there's the notion of motion even though you hit the end long time ago.

This is on Mac OS X using a multitouch trackpad to scroll — I tried doing this on an older Mac with a trackpad, it worked fine there, and with a mouse it doesn't seem to be a problem either. Feel free to ping me in the office to get a demo of this happening. :)

Recommendation:
We should not accept or act on any scroll events to the left when the leftmost tab is all the way visible.

Comment 1

[Frank Yan (:fryn)]

Assigning to self. Please let me know if you or someone else has already started work on this.

Comment 2

[Frank Yan (:fryn)]

It turns out that, while scrolling quickly in one direction with a trackpad or mouse button, DOMMouseScroll events are sometimes fired in the opposite direction (usually within the first 3 events fired during the scroll). Not sure if this is a hardware issue?

One way around this would be to make the response time quicker when you see two DOMMouseScroll events with opposite signs, .e.g if you see a scroll event with direction -2 and start scrolling to the left but then see a scroll event with direction 10, the tab bar should immediately start scrolling to the right rather than finish scrolling to the left.

Comment 3

[Frank Yan (:fryn)]

Just noting that other boxes with scrollbars, e.g. content window and bookmarks sidebar, do not have this problem.

Comment 4

[Frank Yan (:fryn)]

Turns out that it only affects mouse wheels and trackpads that support scrolling in both vertical and horizontal dimensions simultaneously. Case closed. Patch being written.

Comment 5

[Frank Yan (:fryn)]

Attachment: patch v1 (experimenting with sensitivity)

here is the initial algorithm:

- case 1: if the last 5 contiguous scroll events for each dimension average out to the same direction as the current scroll, do the scroll.

- case 2: otherwise, if there have 4 or more previous contiguous scroll events in the current scroll's dimension and, with the current scroll have an average amplitude of greater than twice the average amplitude of the 5 previous contiguous scroll events in the other dimension, do the scroll.

- case 3: otherwise, wait 10 milliseconds in case the user is actually just beginning a scroll. then do case 2.

the reason for these tests is that we allow vertical mouse/trackpad scrolling to scroll the horizontal tab bar (and other horizontal scrollboxes), so in both of the following cases, the tab bar will try to scroll both left and right, wreaking havoc on the user's sanity:

- scrolling downwards and (even slightly) to the left
- scrolling upwards and (even slightly) to the right

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #5)
> - scrolling downwards and (even slightly) to the left
> - scrolling upwards and (even slightly) to the right

I suppose you mean "scrolling left and (even slightly) downwards"?

Is there a certain sequence in which the events typically occur?

Comment 7

[Frank Yan (:fryn)]

(In reply to comment #6)
> (In reply to comment #5)
> > - scrolling downwards and (even slightly) to the left
> > - scrolling upwards and (even slightly) to the right
> 
> I suppose you mean "scrolling left and (even slightly) downwards"?
> 
> Is there a certain sequence in which the events typically occur?

No, I mean scrolling with a displacement vector in the first or third quadrant.
The horizontal displacement is the opposite sign of the vertical displacement (since down/right is positive and up/left is negative).

The sequence is quite unpredictable. Here is the beginning of an example log:
-1 horizontal
1 vertical
2 vertical
-1 horizontal
6 vertical
10 vertical
...

it's pretty clear at this point that the user is trying to scroll downward but also has a little bit of leftward displacement. The effect this causes is especially obvious when the tab bar is already scrolled all the way to the end (last tab is visible). The the user will see the tab bar scroll away from the end a couple tabs and then scroll back to the end, sometimes making it seem like there are infinite tabs, since the tab bar never stops scrolling.

Comment 8

[Frank Yan (:fryn)]

Attachment: patch v2 (improved sensitivity algorithm i hope)

now combines both horizontal and vertical scroll events in the same queue so old events in either axis are flushed out.
shortens delay in edge cases.
makes it harder for user to confuse the scrollbox.

Comment 9

[Frank Yan (:fryn)]

Attachment: patch v2.1 (improved sensitivity and performance)

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 448108 [details] [diff] [review]
patch v2.1 (improved sensitivity and performance)

>--- a/toolkit/content/widgets/scrollbox.xml
...
>+      <field name="_scrolls">[]</field>

It would probably be good to have a slightly more descriptive name for this... _previousScrolls, maybe?

>+      <method name="_mouseScroll">
...
>+            setTimeout(self._mouseScroll, 0, axis, index, self);

Hmm. I'm not sure I understand what this function's for. Also seems risky to delay and rehandle events, because other events could arrive in the meantime.


>       <handler event="DOMMouseScroll"><![CDATA[
>-        // prevent horizontal scrolling from scrolling a vertical scrollbox
>-        if (event.axis == event.HORIZONTAL_AXIS &&
>-            this.orient == "vertical")
>-          return;
>+        if (this.orient == "vertical") {
>+          // prevent horizontal scrolling from scrolling a vertical scrollbox
>+          if (event.axis == event.HORIZONTAL_AXIS)
>+            return;
>+          else
>+            this.scrollByIndex(event.detail);

Nit: Preferred style is to avoid else-after-return... Just do:

  if (foo)
      return;
  bar();


>+        else {

Add a comment here noting what this is trying to do and why.

>+          let axis = event.axis == event.VERTICAL_AXIS ? "v" : "h";

How about just a boolean |isVertical| instead of |axis|?

>+          this._scrolls.push({
>+            axis : axis,
>+            index : event.detail
>+          });

I'd just call it |detail| instead of |index|, or perhaps |magnitude|.

>+          if (this._scrolls.length > 10)
>+            this._scrolls.shift();

How did you arrive at 10?

>+          let [vSum, hSum] = this._scrolls.reduce(function(a, b) {
>+            a[b.axis == "v" ? 0 : 1] += b.index;
>+            return a;
>+          }, [0, 0]);

One letter vars generally suck. a --> sum, b --> val?

Or just calculate vSum/hSum each time an event is received (add the new event, remove the shifted _scrolls event)? More efficient, though I suppose it doesn't matter much for 5-10 events.

>+          if (event.detail > 0 && vSum > 0 && hSum > 0 ||
>+              event.detail < 0 && vSum < 0 && hSum < 0) {

Hmm. Shouldn't the sums be using Math.abs()? The direction (sign) of the previous scrolls shouldn't matter, for trying to see if the use has been scrolling vertically or horizontally.

EG:

let [vSum, hSum] = reduce() { ... += Math.abs(b.index) ... }
let wasVertical = (vSum > hSum);
if (isVertical == wasVertical)
  this.scrollByIndex(event.detail);

Comment 11

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

(re summarizing since this effect was never intentional, and doesn't happen if you scroll with a mousewheel)

Comment 12

[Frank Yan (:fryn)]

Attachment: WIP v3

This current patch effectively does the same thing is v2.1, except with code style changes as per Dolske's feedback.
Limi has tried it out and approves of the effect.

The notable change from v2.1 to v3 is the addition of a dump line so you can see the actual DOMMouseScroll events in the console in the form:
v : -1
h : 4
(This will be removed in the final patch, of course.)

Dolske suggested timestamps, but they seem to make it worse. Not sure why.
You can uncomment the timestamp-related lines in the code and try it out.

Dolske also suggested dropping some initial events by not handling them, which would effectively add "resistance" to scrolling.
I disagree with this approach as it might make Firefox's UI feel slower.

I also tried the approach of keeping a running total of the scroll amounts (and then calling the scroll once the total passes a certain threshold or something like that) but haven't been able to tweak it to work better than this current patch.

Comment 13

[Frank Yan (:fryn)]

Attachment: patch v4 (much shorter and much more awesome)

Please ignore the silliness that was patches versions 1 through 3.
Somehow, in the delirium of being stricken with gastroenteritis, I realized that dolske's idea was on the right track.
Here's the new awesome much simpler technique:

* If the scrollbox is horizontal, only scroll when the last two scroll events were on the same axis as the current one!

DONE!

Comment 14

[Justin Dolske [:Dolske]]

Comment on attachment 449448 [details] [diff] [review]
patch v4 (much shorter and much more awesome)

>+        // We do this by scrolling only when the last two scroll events were
>+        // on the same axis as the current scroll event.

One interesting side effect of this is that diagonal scrolls are effectively ignored, because they toggle between h/v such that the last two events are never the same. I was able to fairly easily do that. [This is really only of interest for the down+right and up+left cases, where both axes would scroll in the same direction.]

But I think it's fine, because if you're going diagonally like that it's not really clear what you're trying to do. And it would be odd to have that work differently from the down+left and up+right cases.

>+          this._prevMouseScrolls.push(isVertical);

Nit: it would be good to keep the push/shift bits together at the end, so that there are always 2 (or less) entries.

Looks good to me, bounce over to Enn ([e-mail]) for a formal review.

Comment 15

[Frank Yan (:fryn)]

Attachment: patch v4.1 (fixed nits)

Comment 16

[[e-mail]]

ensureElementIsVisible scrolls backwards in one dimensional case as well


181           if (elementStart < containerStart) {
182             amountToScroll = elementStart - containerStart;
183           } else if (containerEnd < elementEnd) {
184             amountToScroll = elementEnd - containerEnd;
185           } else if (this._isScrolling) {
186             // decelerate if a currently-visible element is selected during the scroll
187             const STOP_DISTANCE = 15;
188             if (this._isScrolling == -1 && elementStart - STOP_DISTANCE < containerStart)
189               amountToScroll = elementStart - containerStart;
190             else if (this._isScrolling == 1 && containerEnd - STOP_DISTANCE < elementEnd)
191               amountToScroll = elementEnd - containerEnd;
192             else
193               amountToScroll = this._isScrolling * STOP_DISTANCE;
194           } else {
195             return;
196           }

if DOMMouseScroll event hapens when _isScrolling!=0 
this code gives amountToScroll in the opposite direction to scrolling
code after line 185 is probably meant to prevent this, but doesn't.

changing this block with following helps
181
switch(this._isScrolling){
   case 0:
   if (elementStart < containerStart) {
    amountToScroll = elementStart - containerStart;
   } else if (containerEnd < elementEnd) {
    amountToScroll = elementEnd - containerEnd;
   } else {
    amountToScroll=0;
   }
  break;
   case -1:
  if (elementStart < containerStart)
    amountToScroll = elementStart - containerStart;
    break;
   case 1:
  if ( containerEnd  < elementEnd)
    amountToScroll = elementEnd - containerEnd;
    break;
}

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Clearing checkin-needed until comment 16 is addressed.

Comment 18

[Frank Yan (:fryn)]

Attachment: patch v.5 (trivial change to update and pass test)

made a trivial change to the mousescroll test to recognize the fixed scrollbox behavior.
carrying forward the r+.

Comment 19

[Frank Yan (:fryn)]

Oops, my bad. I was logged into my old bugzilla account when attaching.
Frank Yan (fyan) === Frank Yan (studio17).

(In reply to comment #16)
That block of code (not written by me) mainly serves the purpose of making sure the tab container scrolls back into a reasonable position. This might need to occur when, say, the window is resized to a smaller width. The code to make sure the tab container does not "jump backwards" already exists (in another block).

Comment 20

[Frank Yan (:fryn)]

Attachment: patch v.6 (trivial change to update and pass test)

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/f572a9ab898e