1308775 - scroll-behavior: smooth interacts badly with auto scroll

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[5silentrain]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160922113459

Steps to reproduce:

Activate autoscrolling on the forum: http://nightmarish-dream.ru/forum/


Actual results:

Something strange is happening with the autoscroll on this forum


Expected results:

Autoscroll on this forum should work just like on all other sites

Comment 1

[Tooru Fujisawa [:arai]]

The website has the following style on body:
  scroll-behavior: smooth;

it interacts badly with auto scroll.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: simplified testcase

simplified testcase with |scroll-behavior: smooth| style on body, and long content

Comment 3

[Tooru Fujisawa [:arai]]

the issue is that, setting scrollTop or scrollLeft doesn't actually change the value until it gets scrolled visually.

  console.log(document.documentElement.scrollTop); /// 0
  document.documentElement.scrollTop = 1000;
  console.log(document.documentElement.scrollTop); /// 0

https://dxr.mozilla.org/mozilla-central/rev/7a7ba250bb2f5a7cc7acf4b97145425c5292e894/toolkit/content/browser-content.js#221-222
>       this._scrollable.scrollLeft += actualScrollX;
>       this._scrollable.scrollTop += actualScrollY;

auto scroll is implemented by adding diff to scrollTop and scrollLeft incrementally.
it means, if scrollTop doesn't return the value that is set last time, the value is not updated correctly.

Comment 4

[5silentrain]

(In reply to Tooru Fujisawa [:arai] from comment #2)
> Created attachment 8799228 [details]
> simplified testcase
> 
> simplified testcase with |scroll-behavior: smooth| style on body, and long
> content

Can you confirm this bug?
https://bugzilla.mozilla.org/show_bug.cgi?id=1306925

Comment 5

[Tooru Fujisawa [:arai]]

(In reply to 5silentrain from comment #4)
> Can you confirm this bug?
> https://bugzilla.mozilla.org/show_bug.cgi?id=1306925

I cannot.

Comment 6

[Tooru Fujisawa [:arai]]

on Chrome, setting scrollTop or scrollLeft doesn't start smooth scroll, but just changes the scroll position, when #smooth-scrolling flag is enabled.

kip, which behavior matches to the spec?
If Firefox's behavior matches to the spec, this might be a bug in auto scroll.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

It seems to me CSSOM View spec is mostly written according to Gecko's behavior. Setting scrollTop on Chrome doesn't really works.

But autoscroll should not be affected by scroll-behavior anyway, as it is a scrolling performed by user.

Then it comes to me that it should actually be a bug of toolkit that, toolkit should always use "scrollBy" even for elements, with "behavior" set to "instant" (or "smooth" if it looks better, but should never be "auto" since that would depend on value of scroll-behavior from the page).

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Given comment 7, clear ni? for kip, and move this bug to Toolkit component.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Use Element.scrollBy with behavior: "instant" in auto scroll.

Changed to use Element.scrollBy instead of scrollTop/scrollLeft.
I choose |behavior: "instant"| as |behavior: "smooth"| makes it more like *delayed* instead of smooth, and I cannot stop at the point that I want to stop.

Added somewhat corresponding testcase to browser_bug295977_autoscroll_overflow.js, but the test will be passed even without this patch.
actually it's hard to test this fix, as this bug is about smoothness, and checking smoothness will cause intermittent failure...

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8799289 [details] [diff] [review]
Use Element.scrollBy with behavior: "instant" in auto scroll.

Mike might be a better reviewer here

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8799289 [details] [diff] [review]
Use Element.scrollBy with behavior: "instant" in auto scroll.

Review of attachment 8799289 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to redirect this to felipe who originally reviewed the e10s middle-sclick autoscroll behaviour added by billm in bug 938359.

If felipe is overloaded, I guess you can send it back to me and I can try to swap in how all of this stuff works.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8799289 [details] [diff] [review]
Use Element.scrollBy with behavior: "instant" in auto scroll.

Review of attachment 8799289 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Since this changes behavior, it's hard to predict how it can affect webpages, so let's pay extra attention for possible fall out from this. But the change and the rationale for it are reasonable.

Comment 13

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d11e38b5fcfd0573945b810ceb49e83e0ba5386
Bug 1308775 - Use Element.scrollBy with behavior: "instant" in auto scroll. r=felipe

Comment 14

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3cf8325bf7530f13c331298f1ed1be7ce92ce38
Backed out changeset 9d11e38b5fcf for bc test failure (bug 1308775)

Comment 15

[Tooru Fujisawa [:arai]]

Looks like the testcase wasn't working as expected.
will investigate shortly.

Comment 16

[Tooru Fujisawa [:arai]]

apparently I should set middlemousepastepref to false to make the test does autoscroll on linux, after the other tests.
triggered try run
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=732e0e1d1474

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Use Element.scrollBy with behavior: "instant" in auto scroll. r=felipe

so, the pref for middle click affects the tests, and it should be better testing this before that part.
moved the test before tests with middlemousepastepref.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b63951fa8add
Use Element.scrollBy with behavior: "instant" in auto scroll. r=felipe

Comment 19

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b63951fa8addd9dd1011a1d95319437a364512a2
Bug 1308775 - Use Element.scrollBy with behavior: "instant" in auto scroll. r=felipe

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b63951fa8add