487946 - scrollwheel scrolling in bookmarks menu slowed down

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090410 Minefield/3.6a1pre ID:20090410051121

repro:
open FF
open Bookmarks menu
make sure you have a list of bookmarks higher than your screen
scroll with you scrollwheeel

result:
it takes many revolutions of the scrollwheel to scroll

regression window
works in 20090409 nightly
broken in 20090410 nightly

Comment 1

[Dão Gottwald [:dao]]

Places needs it add smoothscroll="false" to the arrowscrollbox if smooth scrolling isn't wanted.

Comment 2

[Dão Gottwald [:dao]]

... although it might be a good idea to accelerate smooth scrolling if the scroll wheel is used repeatedly.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 372260 [details] [diff] [review]
patch

>       <handler event="DOMMouseScroll"><![CDATA[
[...]
>+        // Accelerate if the scroll wheel is used repeatedly.
>+        if (this._scrollTarget) {
>+          let elements = this._getScrollableElements();
>+          if (this._scrollTarget != elements[0] &&
>+              this._scrollTarget != elements[elements.length - 1])
>+            this.ensureElementIsVisible(this._scrollTarget, false);
>+        }
> 
>         this.scrollByIndex(event.detail);
>         event.stopPropagation();
>       ]]></handler>

This could also be done in scrollByIndex. Doesn't matter in practice, probably.

Comment 5

[Neil Deakin]

(In reply to comment #4)
> (From update of attachment 372260 [details] [diff] [review])
> >       <handler event="DOMMouseScroll"><![CDATA[
> [...]
> >+        // Accelerate if the scroll wheel is used repeatedly.
> >+        if (this._scrollTarget) {
> >+          let elements = this._getScrollableElements();
> >+          if (this._scrollTarget != elements[0] &&
> >+              this._scrollTarget != elements[elements.length - 1])
> >+            this.ensureElementIsVisible(this._scrollTarget, false);
> >+        }

I don't know much about mousescrolling, but can you explain what this is doing?

Comment 6

[Dão Gottwald [:dao]]

Each mouse scroll is expected to scroll event.detail items (on most platforms, this should be 3 by default). So if item x was visible, the mouse scroll should make x+3 visible, and another spin should scroll to x+6. But if the first transition hasn't finished yet, you'll only get to x+3, x+4 or x+5 with the current implementation. The patch completes the pending transition (i.e. scrolls to x+3 immediately) before starting a new one.

Comment 7

[Neil Deakin]

Comment on attachment 372260 [details] [diff] [review]
patch

>-          if (aSmoothScroll != false && this.smoothScroll)
>+          if (aSmoothScroll != false && this.smoothScroll) {
>+            this._scrollTarget = element;
>             this._smoothScrollByPixels(amountToScroll);
>-          else
>+          } else
>             this.scrollByPixels(amountToScroll);

It looks strange here to have the 'if' in braces and not the 'else'.

>+
>+        // Accelerate if the scroll wheel is used repeatedly.
>+        if (this._scrollTarget) {
>+          let elements = this._getScrollableElements();
>+          if (this._scrollTarget != elements[0] &&
>+              this._scrollTarget != elements[elements.length - 1])
>+            this.ensureElementIsVisible(this._scrollTarget, false);
>+        }

OK, this makes sense then. I'd put a clearer comment that indicates that it completes an existing scroll.

Comment 8

[Dão Gottwald [:dao]]

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

Comment 9

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090414 Minefield/3.6a1pre ID:20090414012719

Verified/fixed