Closed Bug 1247074 Opened 4 years ago Closed 3 years ago

Smooth scroll gets interrupted by frame reconstruction

Categories

(Core :: Panning and Zooming, defect, P2)

44 Branch
x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox44 --- wontfix
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Keywords: correctness, regression, Whiteboard: gfx-noted)

Attachments

(2 files, 1 obsolete file)

Attached file Test case
See attached test - loading the page should result in the inner frame scrolling to 500px. That's what happens in Chrome and Safari. In Firefox it doesn't. I think the reason is the same as what's happening in bug 1235899 - the smooth-scroll gets interrupted because the nsGfxScrollFrame is destroyed and re-created.

That being said, I would have expected my patch in bug 1235899 to fix this (with APZ enabled) but it doesn't seem to. It needs some more investigation.
Component: Layout → Panning and Zooming
Whiteboard: gfx-noted,perf
This is not really perf, more correctness. Also I guess technically it's a regression from when the scroll-behavior stuff was turned on.
Keywords: regression
Whiteboard: gfx-noted,perf → gfx-noted,correctness
Keywords: correctness
Whiteboard: gfx-noted,correctness → gfx-noted
Version: Trunk → 48 Branch
Bumping out of 48
Version: 48 Branch → 44 Branch
Attached patch Patch (obsolete) — Splinter Review
This one turned out to be pretty straightforward: the frame reconstruction basically loses the destination of the smooth-scroll animation. The restored scroll position is whatever the scroll position was at the time the frame was destroyed. In the non-APZ case the AsyncScroll or AsyncSmoothMSDScroll scroll is destroyed during frame reconstruction, halting the animation. In the APZ case a new scroll position update is sent to the APZ which aborts the animation in the compositor and resets the position back to what the main thread thinks it was.

As per discussion with tnikkel last week, it's better to save/restore the destination of the animation - that way we'll end up scrolled to the right position, even if we don't do a smooth animation to it. The attached patch seems to work, and includes the test case converted to a mochitest.

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea5795a031d1
Assignee: nobody → bugmail
The try push showed an intermittent failure in the test I wrote. I have to put this on the back burner for now to go fix other intermittent failures but I'll get back to it later.
Comment on attachment 8786149 [details]
Bug 1247074 - When a compositor-based smooth scroll animation is in progress and the scrollframe is reconstructed, restore to the animation destination.

https://reviewboard.mozilla.org/r/75116/#review73870
Attachment #8786149 - Flags: review?(tnikkel) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f478daa490df
When a compositor-based smooth scroll animation is in progress and the scrollframe is reconstructed, restore to the animation destination. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/f478daa490df
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Since this didn't appear to fix bug 1297419 (the reporter can still repro that one) it's probably not worth uplifting.
You need to log in before you can comment on or make changes to this bug.