1592902 - Janky scrolling on itest.5ch.net due to add network dynamically changing the viewport element

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

Attachment: testcase that shows the issue

See the test-case. This is the cause of https://github.com/webcompat/web-bugs/issues/36826.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Seems like we maybe should be able to restore the APZ scroll offset when reconstructing the viewport, somehow...

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Or avoid reconstructing the viewport in the first place I guess.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Ah, interesting, this works on WebRender... so probably an apz issue of sorts?

Comment 4

[Botond Ballo [:botond]]

Hmm. I see comparably janky scrolling both with and without WebRender.

Comment 5

[Botond Ballo [:botond]]

The behaviour is much worse on Android (it initially seems like you can't scroll at all, though you eventually can), but again, I see the same thing with and without WebRender.

Comment 6

[Botond Ballo [:botond]]

From the testcase:

function messWithScrolling() {
  let iframe = document.querySelector("iframe");
  let oldOverflow = document.documentElement.style.overflow;
  document.documentElement.style.overflow = 'hidden';
  let someSize = iframe.contentWindow.innerWidth;
  document.documentElement.style.overflow = "";
  setTimeout(messWithScrolling, 200);
};

It looks like the declaration of someSize is important to trigger the problem, even though it's never used. Do you understand why?

Comment 7

[Botond Ballo [:botond]]

The underlying issue here is closely related to the one in bug 1592435, and an extension to the approach taken there improves the behaviour by keeping scrolling responsive, although the scroll offset still jumps back and forth.

Comment 8

[Botond Ballo [:botond]]

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

It looks like the declaration of someSize is important to trigger the problem, even though it's never used. Do you understand why?

This is answered in the Github issue:

innerWidth requires updating layout on the parent document, for obvious reasons, and the overflow-x: hidden on the body means that when overflow on the root is visible, overflow is propagated from the body to the viewport, and not otherwise. So the body needs to become a scrolling box, and thus Firefox rebuilds the layout tree, and it seems some scrolling state gets messed up while doing this.

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1592902 - Add some logging. r=tnikkel

Comment 10

[Botond Ballo [:botond]]

Attachment: Bug 1592902 - Factor out a ShouldCancelAnimationForScrollUpdate() helper. r=tnikkel

Depends on D51445

Comment 11

[Botond Ballo [:botond]]

Attachment: Bug 1592902 - Use ShouldCancelAnimationForScrollUpdate() for visual scroll updates as well. r=tnikkel[C

Depends on D51446

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1592902 - Include the TOUCHING state in CanHandleScrollOffsetUpdate(). r=tnikkel

Otherwise, a main-thread update can interrupt a touch drag near its very
start, when we're still in the TOUCHING state while we're overcoming the
touch start tolerance threshold.

Depends on D51447

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1592902 - Extend gtest to cover this scenario. r=tnikkel

Depends on D51448

Comment 14

[Botond Ballo [:botond]]

So, the patches I posted help, in that they prevent the scroll updates coming from layout from interrupting the user's touch gestures. However, those updates still cause the scroll offset to jump, at least on the test page.

I'm not sure what the impact on the original page is, as I can't actually repro any scrolling misbehaviour on that page myself. I would suggest filing a new bug if there are still issues on the original page (such as the mentioned jump occuring and being noticeable), and we can explore a separate fix for that.

Comment 15

[Timothy Nikkel (:tnikkel)]

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

It looks like the declaration of someSize is important to trigger the problem, even though it's never used. Do you understand why?

Probably because that would force flushing of layout to get that size.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a407a5cf22c5
Add some logging. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/701479e9ecf7
Factor out a ShouldCancelAnimationForScrollUpdate() helper. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/fba08c688f41
Use ShouldCancelAnimationForScrollUpdate() for visual scroll updates as well. r=tnikkel[C
https://hg.mozilla.org/integration/autoland/rev/17e20df1d092
Include the TOUCHING state in CanHandleScrollOffsetUpdate(). r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/d9da7dd620fa
Extend gtest to cover this scenario. r=tnikkel

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/a407a5cf22c5
https://hg.mozilla.org/mozilla-central/rev/701479e9ecf7
https://hg.mozilla.org/mozilla-central/rev/fba08c688f41
https://hg.mozilla.org/mozilla-central/rev/17e20df1d092
https://hg.mozilla.org/mozilla-central/rev/d9da7dd620fa