On some pages, JS scrolling with behavior: "smooth" is ignored during load in Fennec 48

VERIFIED FIXED in Firefox 50

Status

()

defect
P2
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: dave, Assigned: kats)

Tracking

({regression})

48 Branch
Firefox 51
Unspecified
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox50 fixed, firefox51 verified)

Details

Attachments

(1 attachment)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160730000000

Steps to reproduce:

I have an addon: https://addons.mozilla.org/en-US/firefox/addon/mudcat-browser-tools/
Sources: https://addons.mozilla.org/en-US/firefox/files/browse/424090/
It modifies the behaviour of one site - mudcat.org 
One of its functions - 'Scrolls thread to first new post' - has stopped working in Fennec 48

The content script in question - data/mudcat_next_unread.user.js - includes these lines:
nextpostelement.scrollIntoView({block: "start",behavior: "smooth"});
tables[0].children[0].children[1].children[0].children[3].scrollIntoView({block: "start", behavior: "smooth"});
If I remove behavior: "smooth" the script works.

I've tried this on two devices - an Asus Nexus 7 (Android 5.1) and a Samsung Galaxy Tab 10.1 (4.4)
It works fine on Firefox desktop.
Hi Dave,

I tried with the addon(scroll thread to first new post) set to 0 and 1 but I can't see a difference in scrolling or in the website behaviour.

Hey Jorge what is your opinion here? Is this related to addons or we have a FF for Android bug?

Thanks
Component: General → Add-ons
Flags: needinfo?(jorge)
Product: Firefox for Android → Tech Evangelism
Version: 48 Branch → Firefox 48
What you should see on desktop - and doesn't happen in Fennec 48:
- If you click a thread on the main page for the first time - no scrolling on the thread page
- If a post is then added it scrolls to the new post
- If no post has been added it scrolls down a bit - to show you've seen it before
The option is to turn all that off - it doesn't call that content script
It sounds like something changed in Firefox between 47 and 48, but it'd be good to verify that the behavior between those versions really changed, and then try to get a regression window.
Flags: needinfo?(jorge)
Sounds like fallout from APZ.
Component: Add-ons → Graphics, Panning and Zooming
Product: Tech Evangelism → Firefox for Android
Version: Firefox 48 → 48 Branch
I updated the addon v1.2.2 > v1.2.3: removed behavior: "smooth" as in OP to avoid the problem.
Any chance you can make a reduced test case to reproduce the problem? I used the Firefox remote debugger to run your script from mudcat_next_unread.user.js on one of the thread pages from mudcat.org and it seemed to work fine - it did a smooth scroll down to the right place. I also tried a standalone page [1] which does element.scrollIntoView and that seems to work as well. Thus far it seems to indicate the problem is in the addon but I haven't tried the full addon to confirm that.

[1] http://people.mozilla.org/~kgupta/bug/1297419.html
I installed the addon (v1.2.2) and can see how it worked in 47 but doesn't in 48. I suspect it might be because the scroll happens in the middle of page load. Can you try changing the "contentScriptWhen" option for this pagemod module to "end" rather than "ready" and see if that fixes the problem? At least that will tell us if that's the problem or not.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Yes, changing "contentScriptWhen" to "end" makes it work again. Thanks for the diagnosis.
The mudcat servers seem to use rather old software, BTW, in case that's relevant.
Thanks for confirming. This might be related to bug 1247074. It depends on the structure of the page and what all happens between the DOMContentLoaded event and the "load" event.
Depends on: 1247074
Priority: -- → P2
OS: Unspecified → Android
This should be fixed on the 2016-09-02 nightly (which will be out in a few hours) by bug1247074. If you get a chance to test it would be great to get confirmation.
Used Fennec 51a1 2016-09-02 on Samsung GT P7500 Android 4.4.4 and my addon v1.2.2
My initial result is that it does not work.
I'll double-check everything tomorrow.
Tested it again, comparing v1.2.2 of the addon from AMO with v1.2.3 (which omits behavior: "smooth"). v1.2.2 does not scroll in Fennec 51a1 2016-09-02 so not fixed.
Thanks for checking. I was able to repro it locally and chase it down.

When the smooth scroll call happens, the destination gets put into mDestination correctly at [1]. The way it's supposed to work for APZ-driven smooth scrolling is that on the next paint, the FrameMetrics should pick up that destination at [2] and then the APZ will do the smooth scroll. However, at [2] the scroll position is coming back as y=0. The reason for this is that in between [1] and [2] executing, [3] is called, and that resets mDestination to 0.

It looks like the code at [3] is intended to run only while a smooth scroll is not in progress, but the condition used skips the case where the smooth scroll is running in APZ. Updating that condition seems like one way to fix this. Another might be to re-clamp mDestination using the scroll range, rather than just clobbering it with GetScrollPosition(), but that approach seems riskier.

[1] http://searchfox.org/mozilla-central/rev/95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/layout/generic/nsGfxScrollFrame.cpp#2275
[2] http://searchfox.org/mozilla-central/rev/95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/layout/base/nsLayoutUtils.cpp#8877
[3] http://searchfox.org/mozilla-central/rev/95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/layout/generic/nsGfxScrollFrame.cpp#5244
Keywords: regression
Summary: JS scrolling with behavior: "smooth" does not scroll at all in Fennec 48 → On some pages, JS scrolling with behavior: "smooth" is ignored during load in Fennec 48
Comment on attachment 8788581 [details]
Bug 1297419 - Ensure that APZ smooth scrolls don't get clobbered by the main thread as a side-effect of reflow.

https://reviewboard.mozilla.org/r/77020/#review75240
Attachment #8788581 - Flags: review?(tnikkel) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/556e6c27c5ca
Ensure that APZ smooth scrolls don't get clobbered by the main thread as a side-effect of reflow. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/556e6c27c5ca
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
I verified that this works now using the addon v1.2.2 in the latest Nightly. Can you check to make sure as well? If it works I can request uplift to Firefox 50.
Flags: needinfo?(dave)
I tried it earlier today. Yes, it works with Fennec 51a1 2016-09-08.
Thank you all.
Flags: needinfo?(dave)
Comment on attachment 8788581 [details]
Bug 1297419 - Ensure that APZ smooth scrolls don't get clobbered by the main thread as a side-effect of reflow.

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: In some cases a main-thread reflow can cause an APZ smooth scroll to get interrupted or reset
[Describe test coverage new/current, TreeHerder]: hard to write an automated test for it, but verified manually using the provided STR
[Risks and why]: pretty low risk, it's a small change that appears to be a simple oversight in the code
[String/UUID change made/needed]: none
Attachment #8788581 - Flags: approval-mozilla-aurora?
Status: RESOLVED → VERIFIED
Comment on attachment 8788581 [details]
Bug 1297419 - Ensure that APZ smooth scrolls don't get clobbered by the main thread as a side-effect of reflow.

Fix was verified on Nightly, Aurora50+
Attachment #8788581 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.