Closed Bug 1594298 Opened 5 years ago Closed 5 years ago

Is is possible to retain scroll position after the root element is reframed?

Categories

(Core :: Panning and Zooming, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: TYLin, Unassigned)

Details

Attachments

(1 file)

Filed this bug per emilo's suggestion in https://phabricator.services.mozilla.com/D51889#1577724.

As reported in bug 1593752, the site https://www.osram.com.br/cb/ uses a script in a scroll event listener that inserting a primary <body> element, which causes the root element to reframe, resulting in a very laggy scrolling experience by using the mouse wheel.

Any Nightly version between 2019-10-15 and 2019-11-05 should be able to reproduce this issue. (Or by reverting the patch in bug 1593752.)

I wonder whether it is possible for APZ to retain the scroll position after the root element is reframed. If this is not possible, feel free to close this bug.

I'm happy to look into this when I get back next week.

However, I don't currently understand in what way we lose the scroll position when the root element is reframed. nsIScrollableFrame does contain logic to remember its scroll offset across a frame reconstruction -- is that what is at issue, or something else?

A reduced testcase that demonstrates the issue would be very helpful.

A reduced test case based on bug 15937527 comment 3.

To reproduce this locally, you could revert https://hg.mozilla.org/mozilla-central/rev/f2c17f2a388c, or simply delete in bodyWM != aFrame->GetWritingMode() at here to let the frame constructor always reframe root element whenever <body> element is inserted.

So it's not that we're losing the scroll position, it's that wheel ticks kick off an animation, and the reframing interrupts the animation.

If you disable smooth scrolling, such that wheel ticks immediately jump to their destination, the page behaves much better, because there's nothing for the reframing to interrupt. (If you scroll faster you still see a bit of jumping, I think because the wheel events get queued, and end up behaving like a short coarse-grained animation.)

Unlike in bug 1592435 and bug 1592902, we can't make main-thread scroll updates not interrupt a wheel animation in general, because wheel animations are destination-based and a main-thread scroll update can completely change the destination. We could potentially try to make wheel animations not be destination-based, but I think that would require a nontrivial refactor to the ScrollAnimationPhysics hierarchy.

Rather, I think a more realistic approach here would be to make reframes not trigger main-thread scroll updates, or at least have them only trigger updates of type eRestore (which are clobbered by user scrolling).

(In reply to Botond Ballo [:botond] from comment #3)

Rather, I think a more realistic approach here would be to make reframes not trigger main-thread scroll updates, or at least have them only trigger updates of type eRestore (which are clobbered by user scrolling).

Is this issue affecting sites in the wild (which are not fixed by other measures like bug 1593752)? While I'm happy to try this approach in principle, our scroll offset synchronization code is really fragile, and due to regression risk I'd like to avoid touching it unless there's a pressing reason to do so.

Re comment 3:

So it's not that we're losing the scroll position, it's that wheel ticks kick off an animation, and the reframing interrupts the animation.

Thank you for the investigation, Botond! This explains why I can only reproduce the bug with the mouse wheel scrolling, but not with the touchpad on macOS.

Re comment 4:

Is this issue affecting sites in the wild (which are not fixed by other measures like bug 1593752)? While I'm happy to try this approach in principle, our scroll offset synchronization code is really fragile, and due to regression risk I'd like to avoid touching it unless there's a pressing reason to do so.

I do not aware of other bug reports about malfunctioning sites other than Bug 1593752. Bug 1593752 already makes us reframe the root element only if the used writing-mode is changed, so we should be good now. Given there's no easy way to fix the scroll offset synchronization code, I'll close this bug as WORKSFORME.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME

Thanks! If this issue comes up again on a real site, and cannot be easily fixed with another approach like bug 1593752, please feel free to re-open, and we can investigate the potential solutions outlined in comment 3.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: