1584035 - Inserting an element in DOM cancel smooth scrolling upwards

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, defect, P1)

Description

[ariasuni]

Attachment: bug-interrupting-scroll.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:70.0) Gecko/20100101 Firefox/70.0

Steps to reproduce:

See attachment: the behavior is correct with auto but not with smooth.

I’ve been able to reproduce the bug with Element.scrollTo() and suppose that it affects all functions doing scrolling.

Actual results:

Firefox stops scrolling if an element is inserted in the container that is currently smooth scrolling upwards.

Behavior tested on Linux:
– Firefox 69 (Release) with and without WebRender
– Firefox 70b9 (Developer Edition) with WebRender
– Firefox 71.0a1 (Nightly) with WebRender

Expected results:

Firefox does not stop scrolling in the middle of the operation. All the examples in the attachment perfectly fine with Chrome.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

To be fair, if the scroll range changes because you insert an element, it kinda makes sense for the animation to stop, as the offset could be out of range now, for example...

We should probably handle it a bit more gracefully, though... Do you know how WebKit / Safari behaves?

Comment 3

[ariasuni]

First, I’d like to point out that the behavior described in the standard MDN is referring to does not state anything like that. According to this document, the only way to cancel a scroll is for the user to trigger a scroll, and the final scroll position doesn’t get updated during the scroll.

The scroll seems to stop a few frames after the element was inserted, I can’t find an obvious and reliable way to know precisely if and when the scroll is interrupted. I have to wait for Firefox to stop scrolling and check if it reached its original target, or wait some arbitrary (magic value) quantity of time that works after inserting an element, to relaunch the scroll. This workaround is prone to all sort of timing bugs, breakage on new versions of Firefox, etc.

Also, I think that the behavior should be simple and predictable so that developer can build more complex behavior on top of it, when it is appropriate (and the current behavior makes it harder to). And having cases where you’re not relaunching a scroll in the case it is not necessary is better than having to relaunch the scroll every time, because it can break the momentum of the scroll.

In my use case, where scrolling is a core part of the Tab Center Reborn experience, this is really annoying.

Comment 4

[ariasuni]

I forgot to add the links I was referring to:

• https://developer.mozilla.org/en-US/docs/Web/CSS/scroll-behavior
• https://developer.mozilla.org/en-US/docs/Web/API/ScrollToOptions/behavior
• https://drafts.csswg.org/cssom-view/

Comment 5

[ariasuni]

Attachment: scroll-into-view.html

OK so I investigated Chrome’s behavior with a new test that better represent my initial problem, since my issue is with scrolling to an element in a list that may be updated during a scroll.

Chrome just keep scrolling to the initial place of the element, just like in the specifications. It looks like their scroll is “relaunched” after an element was inserted, but to the same position where it was initially scrolling.

In my case, if an element is inserted in my list, I wanted to check if the element will be visible at the end of the scroll, and only relaunch a scroll in this case.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 7

[Negritas Sergiu, Desktop QA]

I managed to find a regression for this issue. I have 2 pushlogs https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=27845cbdcac6f18002fd0d46021ba963487c6659&tochange=bcb74daf647371e48fb2d3a88992f2dd17cbf4f1 in this one I clicked a second time on the smooth scrolling button without refreshing the page and the animation stopped. Also in this pushlog https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5d9c5aa981a5b824e6132c923c2f201b3fa49397&tochange=3dafc8f144ed53747b2606bceb8abf17c3ec68e4 I only took good builds with only one click on the smooth scrolling button. Also i would like to mention that not all the builds scrolled smoothly.

Comment 8

[ariasuni]

This is still happening in Firefox 73.0b11.

I think this bug is important, because it can triggers in a lot of place at seemingly random times: Twitter, Mastodon, or basically anything with a timeline which is being updated regularly; my extension Tab Center Reborn; probably; probably a lot of interactive websites; etc.

This is one of many tiny things that makes Firefox feel buggy, rough and unreliable sometimes. And working around this bug is very hard, so people won’t bother for something that’s not that bad (but actually is for the quality image of Firefox) or will bother but won’t like Firefox for giving them more work.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

This is an interaction with scroll anchoring (setting layout.css.scroll-anchoring.enabled=false makes this work).

Can you confirm that it also works on your end that way? I can take a look at it if so.

Comment 10

[ariasuni]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

This is an interaction with scroll anchoring (setting layout.css.scroll-anchoring.enabled=false makes this work).

Can you confirm that it also works on your end that way? I can take a look at it if so.

Yes, it works when I change the setting you mention, and doesn’t work again when putting it back to true. Thanks for your investigation!

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1584035 - Don't apply scroll anchoring adjustments if we have an ongoing APZ smooth scroll. r=dholbert

Same case as the other smooth-scrolling thingies, scroll anchoring is less
prioritary (and disturbing) in that case.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Botond, do you know how can I add a test for this that's not racy? Is there a way I can await an smooth scroll to ensure it completes to end?

Comment 13

[Botond Ballo [:botond]]

There's an APZ:TransformEnd observer notification that APZ sends at the end of a scroll or zoom, perhaps that would work (example).

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Alright, will try to use that and send you the test for review :-)

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1584035 - Add a test for this bug. r=botond

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1503388fd12
Don't apply scroll anchoring adjustments if we have an ongoing APZ smooth scroll. r=dholbert

Comment 17

[Razvan Maries]

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

Comment 18

[Julien Cristau [:jcristau]]

Is this safe/worth uplifting to beta (after the test lands, anyway)?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

(In reply to Julien Cristau [:jcristau] from comment #18)

Is this safe/worth uplifting to beta (after the test lands, anyway)?

Yeah should be.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9125085 [details]
Bug 1584035 - Don't apply scroll anchoring adjustments if we have an ongoing APZ smooth scroll. r=dholbert

Beta/Release Uplift Approval Request

• User impact if declined: Comment 0
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Disable scroll anchoring while APZ smooth scrolling is going on. Similar to what we do for a bunch of other similar scroll mechanisms.
• String changes made/needed: none

Comment 21

[ariasuni]

I’m not sure how it works, but can I request it to be fixed in ESR too? The fix looks small and it would help me greatly.

If it isn’t going to be, I would have to wait for months (basically the EOL of Firefox ESR 68) to be able to discard my (bad) workaround code (involving an external dependency implementing smooth scroll on its own) for Tab Center Reborn and make the scrolling works smoothly/not jump around.

Anyway, thanks a lot for the fix!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

The patch needs minor tweaking in order apply to ESR68, but it doesn't look scary and we've taken similar patches before (disabling scroll anchoring in situations where it behaves badly), so I wouldn't be opposed to taking it there also.

Comment 23

[Pascal Chevrel:pascalc]

Tracking for 74 as there is an uplift request, I'd like it to be verified by QA on Nightly before uplifting to beta though.

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Comment 25

[Daniel Cicas [:dcicas], Release QA]

Hello,

I can confirm that this issue is fixed in Fx Nightly 75.0a1 (20200214100511)

Comment 26

[Pascal Chevrel:pascalc]

Comment on attachment 9125085 [details]
Bug 1584035 - Don't apply scroll anchoring adjustments if we have an ongoing APZ smooth scroll. r=dholbert

Verified by QA on Nightly, uplift approved for 74.0b4, thanks.

Comment 27

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9cf0faa2144b

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9125085 [details]
Bug 1584035 - Don't apply scroll anchoring adjustments if we have an ongoing APZ smooth scroll. r=dholbert

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: UX is bad, and this is an unfortunate bug for devs to have to workaround.
• User impact if declined: comment 0
• Fix Landed on Version: 75
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple scroll-anchoring-disabling for the case of APZ smooth scroll.
• String or UUID changes made by this patch: non

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9125085 [details]
Bug 1584035 - Don't apply scroll anchoring adjustments if we have an ongoing APZ smooth scroll. r=dholbert

Approved for 68.6esr too. Would be nice if we could get the test reviewed and landed, though.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/ab2f635aa70a

Comment 31

[Botond Ballo [:botond]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #29)

Approved for 68.6esr too. Would be nice if we could get the test reviewed and landed, though.

I've been away for a few weeks and I have a long review backlog, but I hope to get to it tomorrow.

Comment 32

[Daniel Cicas [:dcicas], Release QA]

Hello,

Sorry for the late reply, this issue is also fixed on Fx 74.0b6.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f708e1087811
Add a test for this bug. r=botond,kats

Comment 34

[Daniel Varga [:dvarga]]

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