Closed Bug 1257288 Opened 4 years ago Closed 4 years ago

APZ hit testing does not take frame metrics clips into account

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- fixed

People

(Reporter: mstange, Assigned: botond)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(5 files)

Attached file testcase
STR:
 1. Load the testcase.
 2. Move your mouse on top of the blue circle.
 3. Scroll down.

The subframe is scrolled, even though the mouse is outside the subframe.

If I make the root scrollable (by setting height:4000px on the body), we correctly scroll the root.
This should be pretty easy to fix, we just need to pass in the FM's clip rect to the HitTestingTreeNode at [1] and incorporate it into the check at [2], instead of just using the clip rect from the layer. I don't recall though what the semantics are of the layer's clip rect vs the FM's clip rect - what order do they apply in?

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/HitTestingTreeNode.cpp?rev=77d65c5c2422#219
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/HitTestingTreeNode.cpp?rev=77d65c5c2422#234
Keywords: regression
Whiteboard: [gfx-noted]
The layer's clip rect is always moved along with the layer, except for fixed layers that have the scrollingClip annotation.
The frame metrics clips are just outside the scroll frame for that frame metrics; in other words, the clip on a frame metrics is affected by scroll offsets of all ancestor frame metricss but not of the frame metrics that it's on.
The final clip of a layer is the intersection of all those clips.
Going to have a look at this.
Assignee: nobody → botond
Review commit: https://reviewboard.mozilla.org/r/47879/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47879/
Attachment #8743566 - Flags: review?(bugmail.mozilla)
Attachment #8743567 - Flags: review?(bugmail.mozilla)
Attachment #8743568 - Flags: review?(bugmail.mozilla)
Attachment #8743569 - Flags: review?(bugmail.mozilla)
The idea behind this simple fix is that APZ hit testing already takes care of applying async transforms while walking the hit testing tree; we just need to include the scroll metadata's scroll clip in the clip rect reported by the LayerMetricsWrapper.
Comment on attachment 8743566 [details]
MozReview Request: Bug 1257288 - Move IntersectMaybeRects() to gfx/2d/Rect.h. r=kats

https://reviewboard.mozilla.org/r/47879/#review44845
Attachment #8743566 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8743567 [details]
MozReview Request: Bug 1257288 - Fix a bug in APZCTM logging code. r=kats

https://reviewboard.mozilla.org/r/47881/#review44847
Attachment #8743567 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8743568 [details]
MozReview Request: Bug 1257288 - Improve the APZ gtest infrastructure to make writing multi-FrameMetrics tests easier. r=kats

https://reviewboard.mozilla.org/r/47883/#review44849
Attachment #8743568 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8743569 [details]
MozReview Request: Bug 1257288 - Have APZ hit testing respect scroll clips. r=kats

https://reviewboard.mozilla.org/r/47885/#review44851

Thanks for splitting up the patches, made it easy to review!
Attachment #8743569 - Flags: review?(bugmail.mozilla) → review+
Autolanded.
I don't think this is worth uplifting to 47. Let me know if you disagree.
The handling of clip rects during compositor hit testing was more broken than I realized when fixing this bug: the layer's mClipRect applies on top of the layer's CSS transform, but below the async transform of the bottommost ScrollMetadata on the layer.

The way LayerMetricsWrapper::GetClipRect() is used, it's considered to apply on top of the async transform of the ScrollMetadata. This means it has been incorrect ever since scroll clips have been introduced for LayerMetricsWrapper::GetClipRect() on a bottommost metrics to include Layer::GetClipRect(). Rather, Layer::GetClipRect() should be hit-tested separately against a point which has the bottommost async transform unapplied, but the layer's CSS transform not yet unapplied.

I'll fix this in bug 1267438 or a follow-up to that.
(In reply to Botond Ballo [:botond] from comment #18)
> the layer's mClipRect applies on top
> of the layer's CSS transform, but below the async transform of the
> bottommost ScrollMetadata on the layer.

That's assuming the layer is not fixed. If the layer is fixed, its mClipRect applies on top of whichever async transforms do not scroll the layer as a result of it being fixed. This is something we don't currently handle in the general case either.
See Also: → 1269537
I'm going to fix the issues described in comment 18 and comment 19 in bug 1269537, as a follow-up to bug 1267438.
You need to log in before you can comment on or make changes to this bug.