1320609 - tabbar should scroll smoothly even when wheel device doesn't support high resolution scroll

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, <scrollbox>.scrollByPixels doesn't support smooth scroll, but wheel event handler always use it even when wheel event's deltaMode is DOM_DELTA_MODE_LINE or DOM_DELTA_MODE_PAGE. This causes uncomfortable scroll.

Comment 1

[Dão Gottwald [:dao]]

[Tracking Requested - why for this release]: regression from bug 1316505 on devices that don't support high-resolution scrolling (that's a lot of devices)

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sigh. I'm struggling with test_mousescroll.xul since it's not smooth scroll aware and only on macOS, it times out...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c85631bc9ffa800ab17cf6ba04cf3262d13c5ad

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1320609 part.1 wheel event handler of <scrollbox> should use |_smoothScrollByPixels| instead of |scrollByPixels| when the deltaMode is DOM_DELTA_LINE or DOM_DELTA_PAGE

<scrollbox> should scroll its contents smoothly even if wheel device doesn't support high resolution scroll.

This patch makes the wheel event handler use |_smoothScrollByPixels| when deltaMode is DOM_DELTA_LINE or DOM_DELTA_PAGE.  The reason why ignoring DOM_DELTA_PIXEL is, such devices are typically supports high resolution scroll.  Therefore, <scrollbox> doesn't need to animate its scroll by itself.

However, |_smoothScrollByPixels| doesn't work fine without some additional fixes.

First, this patch doesn't stop pending scroll from |_smoothScrollByPixels| because discarding some pending scroll causes slower scroll.  Actually, when you turn mouse wheel faster, scroll amount becomes smaller without this fix.

Next, this patch adds continous scroll mode to |_scrollAnim| object.  If it's requested new animation during processing previous scroll, it keeps previous scroll amount and restart animation with new destination.  Additionally, accumulating the scroll distance as fractional value.  Therefore, it scrolls 3px when 1.5px scroll is requested twice.

Finally, this patch discards pending scrolls when user tries to scroll to reverse direction.  When user tries to scroll to reserse direction, the user must want to scroll the contents from current position rather than actual scroll destination.  Therefore, this patch discards pending scrolls in |_smoothScrollByPixels|.  Although, it might be better to handle this in |_scrollAnim|.  However, |_isScrolling| is not managed by it and changing the behavior in private method is safer than changing utility object (i.e., |_scrollAnim|).

Additionally, this patch fixes timestamp issue at callback of requestAnimationFrame().  Sometimes, the given timestamp is older than start time.  In that case, current code scrolls to reverse diretion.  Therefore, we should treat such old timestamp as same as start time.

Review commit: https://reviewboard.mozilla.org/r/96842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96842/

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1320609 part.2 Rewrite test_mousescroll.xul with generator for async-scroll-aware

test_mousescroll.xul becomes permanent orange due to the animation at handling wheel event.  Therefore, this patch rewrites it with generators.

Note that this removes |delta(X|Y) == 0| cases from testArrowScrollbox() because it doesn't cause wheel events, therefore, it's impossible to test without timer and it's not worthwhile to test the cases that no wheel events are fired.

Additionally, this adds todo to testArrowScrollBox() because |scrollPosition += px| in <scrollbox>.scrollByPixels doesn't work fine only on macOS with this test.  Even when the px is larger than its scrollSize, the result isn't set to scrollSize.  This issue shouldn't be the scope of bug 1320609, therefore, this patch makes it todo. (Of course, I don't see any problem with actual UI such as tabbar and bookmark menus.)

Review commit: https://reviewboard.mozilla.org/r/96844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96844/

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I give up to fix the odd orange on macOS. It's caused by setting scrollPosition which doesn't work well only during the test. For example, in my environment, the arrowscrollbox#hscrollbox's scrollSize is 326. However, setting scrollPosition to any big value nor retrying to set it to same value again sets 266. I don't understand what happens. With Trackpad, it can be scrolled to right-most and then the test restarts.

I don't have much time to investigating it for now and the test works fine on the other platforms. So, using todo_is() should be enough for now.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8816059 [details]
Bug 1320609 part.1 wheel event handler of <scrollbox> should use |_smoothScrollByPixels| instead of |scrollByPixels| when the deltaMode is DOM_DELTA_LINE or DOM_DELTA_PAGE

Calling _smoothScrollByPixels like this means that you'll ignore the smoothscroll attribute and toolkit.scrollbox.smoothScroll pref.

nit: rename toScroll to scrollDirection

The code you added in _scrollAnim could use a few blank lines for better structuring and readability.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8816059 [details]
Bug 1320609 part.1 wheel event handler of <scrollbox> should use |_smoothScrollByPixels| instead of |scrollByPixels| when the deltaMode is DOM_DELTA_LINE or DOM_DELTA_PAGE

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96842/diff/1-2/

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8816060 [details]
Bug 1320609 part.2 Rewrite test_mousescroll.xul with generator for async-scroll-aware

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96844/diff/1-2/

Comment 9

[Marcia Knous [:marcia]]

Tracking 53+ for the reasons in Comment 1.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8816059 [details]
Bug 1320609 part.1 wheel event handler of <scrollbox> should use |_smoothScrollByPixels| instead of |scrollByPixels| when the deltaMode is DOM_DELTA_LINE or DOM_DELTA_PAGE

https://reviewboard.mozilla.org/r/96842/#review99498

This is pretty good! I'm not completely convinced on the "discard destination when scrolling backwards" argument, but I'll leave it up to you to decide whether to leave it in or to drop it.
I also don't really like the fact that we have all three of .startPos, .distance and .destination, where just .startPos and .destination would have sufficed to express the same information. But that's not the end of the world, and I can see how changing it would make the code harder to read in some places.

::: toolkit/content/widgets/scrollbox.xml:312
(Diff revision 2)
> +          // However, if the scroll direction is changed, let's cancel the
> +          // pending scroll because user must want to scroll from current
> +          // position.
> +          if (this._isScrolling && this._isScrolling != scrollDirection)
> -          this._stopSmoothScroll();
> +            this._stopSmoothScroll();

I'm not convinced of this argument. It might make sense, but it also breaks symmetry - if you quickly scroll back and forth, then you'll and up scrolling slightly backwards instead of ending up at your starting position.

::: toolkit/content/widgets/scrollbox.xml:318
(Diff revision 2)
>            if (amountToScroll == 0)
>              return;

I'd put this if at the very start, so that we don't accidentally reset smooth scrolls for zero-delta events. I know that that's what the old code was doing, but I don't think it's a good idea.

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8816060 [details]
Bug 1320609 part.2 Rewrite test_mousescroll.xul with generator for async-scroll-aware

https://reviewboard.mozilla.org/r/96844/#review99510

::: toolkit/content/tests/chrome/test_mousescroll.xul:234
(Diff revision 2)
> +                                if (getPos() == expected) {
> +                                  return true;
> +                                }
> +                                if (oldPos == getPos()) {
> +                                  // If scroll stopped completely, let's continue the test.
> +                                  return ++retry == 5;

Maybe pull the 5 out into a MAX_RETRY_COUNT constant.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Masayuki, we looked at this during regression triage today and thought we'd ping you about landing this fix.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'd like to try to land this tonight, but I'm not sure if it's possible. Sorry for the delay due to some trouble of my machine and business trip.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8816059 [details]
Bug 1320609 part.1 wheel event handler of <scrollbox> should use |_smoothScrollByPixels| instead of |scrollByPixels| when the deltaMode is DOM_DELTA_LINE or DOM_DELTA_PAGE

https://reviewboard.mozilla.org/r/96842/#review101002

::: toolkit/content/widgets/scrollbox.xml:312
(Diff revision 2)
> +          // However, if the scroll direction is changed, let's cancel the
> +          // pending scroll because user must want to scroll from current
> +          // position.
> +          if (this._isScrolling && this._isScrolling != scrollDirection)
> -          this._stopSmoothScroll();
> +            this._stopSmoothScroll();

> but it also breaks symmetry

Yes, I intentionally do it. If the process is busy, delayed scroll is very annoying.

> if you quickly scroll back and forth, then you'll
> and up scrolling slightly backwards instead of
> ending up at your starting position.

Indeed. However, I don't think that users manage scroll position so strictly in their brain. So, if this behavior makes users feel bad, I have another option.

That is, we can skip the animation and restart animation from old destination. But I feel this is odd behavior.

Anyway, I'd like to check feedback from users if this makes users feel bad.

::: toolkit/content/widgets/scrollbox.xml:318
(Diff revision 2)
>            if (amountToScroll == 0)
>              return;

Okay. Basically, 0 delta wheel events shouldn't be fired. See here.
https://dxr.mozilla.org/mozilla-central/rev/10a5e2fc24e4e24fbe115fb190bcc28df4627ee2/widget/WidgetEventImpl.cpp#373,396,400-401

So, moving this check to the start shouldn't change anything actually.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8816059 [details]
Bug 1320609 part.1 wheel event handler of <scrollbox> should use |_smoothScrollByPixels| instead of |scrollByPixels| when the deltaMode is DOM_DELTA_LINE or DOM_DELTA_PAGE

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96842/diff/2-3/

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8816060 [details]
Bug 1320609 part.2 Rewrite test_mousescroll.xul with generator for async-scroll-aware

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96844/diff/2-3/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83bfdbcc5018
part.1 wheel event handler of <scrollbox> should use |_smoothScrollByPixels| instead of |scrollByPixels| when the deltaMode is DOM_DELTA_LINE or DOM_DELTA_PAGE r=mstange
https://hg.mozilla.org/integration/autoland/rev/9fc13b45970a
part.2 Rewrite test_mousescroll.xul with generator for async-scroll-aware r=mstange

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/83bfdbcc5018
https://hg.mozilla.org/mozilla-central/rev/9fc13b45970a

Comment 19

[Andrei Vaida [:avaida]]

Setting qe-verify- since this fix seems to have automated coverage. Masayuki, if you think manual QA should instead be looking at this, feel free to flip the flag or ni? me.