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

RESOLVED FIXED in Firefox 66

Status

()

defect
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: denschub, Assigned: emilio)

Tracking

({regression})

unspecified
mozilla66
Points:
---
Dependency tree / graph
Bug Flags:
webcompat ?
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)

Details

()

Attachments

(1 attachment)

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.

Flags: webcompat?

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

Flags: needinfo?(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 :(

Component: CSS Parsing and Computation → Layout
Flags: needinfo?(dschubert)

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. :))

Flags: needinfo?(emilio)

Thanks!

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.

We should be able to do better.

Flags: needinfo?(emilio)
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.
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9aa345641084
Reframe less as a result of UpdateContainingBlock. r=dholbert
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Flags: needinfo?(dschubert)
You need to log in before you can comment on or make changes to this bug.