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

RESOLVED FIXED in Firefox 66

Status

()

defect
RESOLVED FIXED
5 months ago
4 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)

Reporter

Description

5 months ago

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?
Reporter

Comment 1

5 months ago

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

Flags: needinfo?(emilio)
Assignee

Comment 2

5 months ago

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)
Reporter

Comment 3

5 months ago

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)
Assignee

Comment 4

5 months ago

Thanks!

Assignee

Comment 5

5 months ago

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.

Assignee

Comment 6

5 months ago

We should be able to do better.

Flags: needinfo?(emilio)
Assignee

Comment 8

5 months ago
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 10

4 months ago
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9aa345641084
Reframe less as a result of UpdateContainingBlock. r=dholbert

Comment 11

4 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Reporter

Updated

4 months ago
Flags: needinfo?(dschubert)
You need to log in before you can comment on or make changes to this bug.