1519371 - Reframe after adding `will-change: transform;` breaks scroll event handler

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Dennis Schubert [:denschub]]

This was originally filed as a WebCompat bug, but it looks like this is a regression of bug 1479859.

The site in question has a full-viewport overlay that gets scrolled away via JS on the scroll event. When dragging starts, the site adds a CSS class that adds will-change: transform to the element, which sometimes results in us no longer processing the scroll events, and the element not getting scrolled away.

Comment 1

[Dennis Schubert [:denschub]]

Emilio, since you helped in the WebCompat bug, is this something you can look at?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Is there any chance you could try to find a bit more reduced test-case for this? I tried to come up with one, but couldn't (latest attempt is https://crisal.io/tmp/crappy-scroll.html).

If the answer is not is ok, I'll take a quick look at rr today, but if I don't figure out I might need to move on, since I'm swamped with other stuff :(

Comment 3

[Dennis Schubert [:denschub]]

Is there any chance you could try to find a bit more reduced test-case for this?

I'll give it a try. Let's cancel your ni? for now, I'll ping back if I have a testcase (or when I have given up. :))

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Thanks!

Comment 5

[Emilio Cobos Álvarez (:emilio)]

So the issue here is that adding will-change: transform to an already-transformed frame will reframe it.

I think there's no underlying correctness issue, but reframing in this case is inefficient and results in this havoc.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

We should be able to do better.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Current basically untested attempt since I got to run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f056e003eb7f6f2aa235ef0ce2c2dcebb7b23c51

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1519371 - Reframe less as a result of UpdateContainingBlock.

This patch should make the detection of whether we should reframe in
UpdateContainingBlock exact.

It should have no behavior change, but sometimes reframing can confuse event
handling code or what not.

We don't have a reduced test-case for the event handling regression this fixes,
but I added a test to ensure we don't uselessly reframe in this case that fails
without this patch and passes with.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7d37d08b900659d5f102aae54d2ddec9bea3249

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9aa345641084
Reframe less as a result of UpdateContainingBlock. r=dholbert

Comment 11

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/9aa345641084