1592435 - www.matele.be causes completely broken scrolling on Fenix with scroll-anchoring on.

Status: RESOLVED FIXED

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

Description

[Thomas Wisniewski [:twisniewski]]

As just discovered in https://webcompat.com/issues/37532, the Add This widget is conflicting with scroll anchoring on touchscreens in Firefox to cause a sub-optimal scrolling experience (though it can be a subtle effect).

The reason is that the script adds a scroll event listener to the window, which throttles the following function to test whether the widget is visible on-screen or not:

function(e, t) {
        var i, o = (new Date).getTime();
        if (t = t || {}, t.cacheDuration = void 0 !== t.cacheDuration ? t.cacheDuration : 3e3, !e) return !1;
        if (e.scrollCheckID) {
            if (i = e.scrollCheckID, !(o - n[i] > t.cacheDuration)) return a[i];
            n[i] = o 
        } else e.scrollCheckID = n.length, n[n.length] = o, i = e.scrollCheckID;
        var r = e.getBoundingClientRect(),
            s = {
                top: 0,
                left: 0, 
                bottom: window.innerHeight || document.documentElement.clientHeight,
                right: window.innerWidth || document.documentElement.clientWidth
            },
            d = 0,
            u = Math.max(r.top, s.top),
            c = Math.min(r.bottom, s.bottom),
            l = Math.max(r.left, s.left),
            f = Math.min(r.right, s.right),
            p = (r.right - r.left) * (r.bottom - r.top);
        return d = c > u && f > l ? (c - u) * (f - l) : 0, a[i] = d / p, a[i]
    }

My full diagnosis with more code fragments is on the webcompat.com link, but suffice to say that this seems very similar to other cases we've seen (and apparently resolved) with scroll anchoring, like the one in bug 1562793.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Loading http://www.matele.be/conflit-abbaye-lhoist-stop-aux-fake-news:

• With tracking protection enabled I cannot go past the image, it gets stuck.
• With tracking protection disabled I can scroll, though quite jankily.

I can see the page flicker a lot on desktop if I reduce the viewport, and if you execute addEventListener("scroll", () => console.log("scroll")) on the terminal you can see that it is indeed a scroll event loop with scroll anchoring.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

The function that's causing scroll anchoring to come into play is:

	$( window ).scroll(function()
	{
		windowPos = $( window ).scrollTop();
		bannerHeight = $( "#nav-banners" ).height();
		
		if ( windowPos >= bannerHeight )
		{
			$( "#nav-banners" ).stop().slideUp();
		}else{
			$( "#nav-banners" ).stop().slideDown();
		}
	});

Where #nav-banners is an overflow: hidden element which is in-flow, (and thus scroll-anchoring should apply when animating). So I think the rest is a matter of timeout scheduling. If we schedule the timeout fast enough, or too slow, the animation restarts and never completes.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: + a gradient to visualize scroll position better.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

The test-case in comment 4 blocks scrolling completely for me on Fenix, mind confirming?

If so, this is a scheduling difference I think (the test-case is wacky on chrome desktop)... :(

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: + bigger viewport-relative height

Comment 7

[Thomas Wisniewski [:twisniewski]]

Yes, I can reproduce on Fennec and Fenix nightlies (after scrolling down a bit, I can't scroll anymore at all). Chrome also acts a bit funky while scrolling the testcase for me, but at least I can still scroll.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Botond, I suspect Chrome is also hitting something like this under the hood (since the test-case is waky on Chrome desktop too). But somehow in Fennec/Fenix (unlike in desktop with apz, looks like) our touch scrolling seems to somehow block on the main thread, and thus we can never scroll...

Is this something you could provide some advice into how to fix?

Comment 9

[Botond Ballo [:botond]]

The issue seems to be related to touch events vs. other event types: I can repro the "can't scroll at all" issue on a touchscreen laptop as well, if I'm using the touchscreen rather than the touchpad to scroll.

Comment 10

[Botond Ballo [:botond]]

Attachment: Testcase for touch-scrolling issue

Here is a reduced testcase for the touch-scrolling issue. It's unrelated to scroll anchoring.

STR:

1. Load the attached testcase on a touch-capable device.
2. Try to pan the red area up out of view in a single gesture (without releasing the finger).

With touch events, the gesture is interrupted halfway through, and doesn't resume until you lift the finger and touch it down again.

With touchpad scrolling, you can keep scrolling without lifting the finger.

Comment 11

[Botond Ballo [:botond]]

What's happening is the main thread's call to scrollBy() when you've scrolled the red area halfway out of view interrupts the current scroll gesture by calling CancelAnimation(), which resets the gesture state to NOTHING.

Touchpad scrolling generates pan gesture events, which have logic to resume the gesture if something interrupts it and puts it into the NOTHING state, if the fingers are still on the touchpad.

There is no comparable resumption logic for touch events.

Comment 12

[Botond Ballo [:botond]]

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

There is no comparable resumption logic for touch events.

While this has been the case all along, I can't think of a good reason not to allow resuming touch drags like this, so I'm happy to try changing it.

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1592435 - Avoid a shutdown-crash during gtests. r=tnikkel

This is unrelated to this bug, I just ran into it while running the tests.

Comment 14

[Botond Ballo [:botond]]

Attachment: Bug 1592435 - Don't allow a main thread scroll update to interrupt a touch-drag gesture. r=tnikkel

This makes the behaviour for touch-drags match the behaviour we already have
for pan gesture events.

Depends on D51201

Comment 15

[Botond Ballo [:botond]]

Attachment: Bug 1592435 - Add a gtest. r=tnikkel

Depends on D51202

Comment 16

[Botond Ballo [:botond]]

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

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

There is no comparable resumption logic for touch events.

While this has been the case all along, I can't think of a good reason not to allow resuming touch drags like this, so I'm happy to try changing it.

For reference, my first attempt was to implement this similarly to pan gesture events: if we receive a touch-move event in the NOTHING state, transition to the TOUCHING state. However, this caused problems with multi-touch gestures and touch-action, where touch-action not allowing an event to be handled resulted in the gesture state changing to NOTHING, and we didn't want subsequent touch-moves in the input block to get us back into active states.

Instead, I took the approach of extending the mechanism added in bug 1522026 to avoid main-thread scroll updates cancelling momentum scrolling. This does mean that CancelAnimation() won't be called at all, but I think that's fine -- I've looked through the various things it does and I don't think any of them are relevant in the middle of a pan gesture.

That said, I do consider this somewhat of a risky change. If possible, it would be good to avoid uplifting it, or at least good to give it generous bake time before an uplift.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f170bef85771
Avoid a shutdown-crash during gtests. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/31601597fea2
Don't allow a main thread scroll update to interrupt a touch-drag gesture. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/05fbf853b8b7
Add a gtest. r=tnikkel

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/f170bef85771
https://hg.mozilla.org/mozilla-central/rev/31601597fea2
https://hg.mozilla.org/mozilla-central/rev/05fbf853b8b7