1257288 - APZ hit testing does not take frame metrics clips into account

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Markus Stange [:mstange]]

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

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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

Comment 2

[Markus Stange [:mstange]]

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.

Comment 3

[Botond Ballo [:botond]]

Going to have a look at this.

Comment 4

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1257288 - Move IntersectMaybeRects() to gfx/2d/Rect.h. r=kats

Review commit: https://reviewboard.mozilla.org/r/47879/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47879/

Comment 5

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1257288 - Fix a bug in APZCTM logging code. r=kats

Review commit: https://reviewboard.mozilla.org/r/47881/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47881/

Comment 6

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1257288 - Improve the APZ gtest infrastructure to make writing multi-FrameMetrics tests easier. r=kats

Review commit: https://reviewboard.mozilla.org/r/47883/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47883/

Comment 7

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1257288 - Have APZ hit testing respect scroll clips. r=kats

Review commit: https://reviewboard.mozilla.org/r/47885/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47885/

Comment 8

[Botond Ballo [:botond]]

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 9

[Botond Ballo [:botond]]

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

Comment 10

[Kartikaya Gupta (email:[e-mail])]

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

Comment 11

[Kartikaya Gupta (email:[e-mail])]

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

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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

Comment 13

[Kartikaya Gupta (email:[e-mail])]

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!

Comment 14

[Botond Ballo [:botond]]

Autolanded.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f579f80a80b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d88322c06d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/256e448ecd4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e16cf77ae90c

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f579f80a80b9
https://hg.mozilla.org/mozilla-central/rev/2d88322c06d8
https://hg.mozilla.org/mozilla-central/rev/256e448ecd4b
https://hg.mozilla.org/mozilla-central/rev/e16cf77ae90c

Comment 17

[Botond Ballo [:botond]]

I don't think this is worth uplifting to 47. Let me know if you disagree.

Comment 18

[Botond Ballo [:botond]]

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.

Comment 19

[Botond Ballo [:botond]]

(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.

Comment 20

[Botond Ballo [:botond]]

I'm going to fix the issues described in comment 18 and comment 19 in bug 1269537, as a follow-up to bug 1267438.