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

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

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.

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

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.

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

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

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

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

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

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.

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 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 (enndeakin@gmail.com) for a formal review.

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; }

Clearing checkin-needed until comment 16 is addressed.

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

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