1465935 - Hit-testing on position:fixed items inside iframes isn't correct

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

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

Nightly, WR enabled. Load gfx/layers/apz/test/mochitest/helper_scroll_on_position_fixed.html and wheel-scroll over the lower green div (the position:fixed one inside the iframe).

Expected: the iframe scrolls (this is what happens with WR off)
Actual: the root document scrolls

Comment 1

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

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

Comment 2

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

Try push is green; the only orange is due to a patch from bug 1465892 which I'll hold off on.

Comment 3

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

Attachment: Bug 1465935 - Fix hit-testing for fixed-pos items inside iframes.

Without this patch, the scrollId for display items inside a fixed-pos
item end as the ASR of the item. In the case of fixed-pos items that are
inside iframes, the ASR is the outer document's root scrollframe. This
means that e.g. wheel-scrolling while over a fixed-pos item inside an
iframe ends up scrolling the outer document's root scrollframe instead
of the iframe's root scrollframe.

In the non-WR codepath, there some APZ machinery that walks up in the
HitTestingTreeNode tree from the hit result, looking to see if that node
has a fixed-pos ancestor, and if so, uses the fixed-pos item's target
APZ as the real hit result. This machinery doesn't exist in WR, because
we don't use the HitTestingTreeNode tree for hit-testing in APZ.
Instead, we need to make sure that the item tag for those display items
already has the appropriate scrollid set.

This patch accomplishes this by introducing a new RAII class that is
pushed into the wr::DisplayListBuilder while we are building display
items inside a nsDisplayFixedPosition, and allows the desired scroll id to
be set on the hit-testing display items.

This behaviour is exercised by test_group_wheelevents, which can now be
enabled with this fix.

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

Comment 4

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

Comment on attachment 8982484 [details]
Bug 1465935 - Fix hit-testing for fixed-pos items inside iframes.

https://reviewboard.mozilla.org/r/248458/#review254668

::: commit-message-9900c:10
(Diff revision 1)
> +In the non-WR codepath, there some APZ machinery that walks up in the
> +HitTestingTreeNode tree from the hit result, looking to see if that node
> +has a fixed-pos ancestor, and if so, uses the fixed-pos item's target
> +APZ as the real hit result. This machinery doesn't exist in WR, because

The machinery I'm referring to this in comment is the stuff at https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/gfx/layers/apz/src/APZCTreeManager.cpp#2639

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8982484 [details]
Bug 1465935 - Fix hit-testing for fixed-pos items inside iframes.

https://reviewboard.mozilla.org/r/248458/#review256020

Sorry for the slow review; I had the comment below written two days ago but forgot to submit it and then MozReview threw it away when I restarted my browser...

::: layout/painting/nsDisplayList.cpp:7442
(Diff revision 1)
> +  // share the same ASR as this item) use the correct scroll target. That way
> +  // attempts to scroll on those items will scroll the root scroll frame.
> +  mozilla::wr::DisplayListBuilder::FixedPosScrollTargetTracker tracker(
> +      aBuilder,
> +      GetActiveScrolledRoot(),
> +      nsLayoutUtils::ScrollIdForRootScrollFrame(Frame()->PresContext()));

How does this behave in the case where you have a scrolled transform as the ancestor of your position:fixed element? In that case, the position:fixed element should scroll with the transform, because transforms are containing blocks for position:fixed descendants.

FrameLayerBuilder handles this case by calling nsLayoutUtils::IsReallyFixedPos and by treating elements for which that returns false as not fixed.

Comment 6

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

(In reply to Markus Stange [:mstange] from comment #5)
> How does this behave in the case where you have a scrolled transform as the
> ancestor of your position:fixed element? In that case, the position:fixed
> element should scroll with the transform, because transforms are containing
> blocks for position:fixed descendants.
> 
> FrameLayerBuilder handles this case by calling
> nsLayoutUtils::IsReallyFixedPos and by treating elements for which that
> returns false as not fixed.

As far as I can tell this case works fine. I have this test page:

<!DOCTYPE html>
<body style="height: 5000px">
    <div style="overflow: auto; height: 500px; width: 500px">
        <div style="transform: translateX(10px); width: 400px; height: 800px; background-image: linear-gradient(green,blue)">
            <div style="position: fixed; width: 100px; height: 100px; background-color: red">fixed</div>
        </div>
    </div>
</body>

and the gecko DL for this doesn't have any nsDisplayFixedPosition items. The relevant bit looks like this:

nsDisplayTransform p=0x1237f2720 f=0x123153490(Block(div)(1)) key=69 bounds(1020,480,24060,48000) layerBounds(1020,480,24060,48000) visible(480,480,29100,30000) componentAlpha(1020,480,24060,48000) clip(480,480,29100,30000) asr(<0x1231521e0>) clipChain(0x1237baca0 <480,480,29100,30000> [0x1231521e0], 0x1237bb920 <0,0,75900,44220> [root asr]) ref=0x123152020 agr=0x1231520b0[ 1 0; 0 1; 36 16; ]
  CompositorHitTestInfo p=0x1237f1020 f=0x123153490(Block(div)(1)) key=27 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(-30,0,24030,30000) componentAlpha(0,0,0,0) clip() asr(<0x1231521e0>) clipChain() ref=0x123153490 agr=0x123153490 (hitTestInfo 0x1) hitTestArea(x=0, y=0, w=24000, h=48000)
  Background p=0x1237f1620 f=0x123153490(Block(div)(1)) key=2 bounds(0,0,24000,48000) layerBounds(0,0,24000,48000) visible(-30,0,24030,30000) componentAlpha(0,0,0,0) clip(0,0,24000,48000) asr(<0x1231521e0>) clipChain(0x1237f2260 <0,0,24000,48000> [0x1231521e0]) ref=0x123153490 agr=0x123153490 (opaque 0,0,24000,48000)
  WrapList p=0x1237f1820 f=0x123153540(Block(div)(1)) key=72 bounds(-60,0,6060,6000) layerBounds(-60,0,6060,6000) visible(-30,0,6030,6000) componentAlpha(-60,66,2039,1020) clip() asr(<0x1231521e0>) clipChain() ref=0x123153490 agr=0x123153490
    CompositorHitTestInfo p=0x1237bbb60 f=0x123153540(Block(div)(1)) key=27 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(-30,0,6030,6000) componentAlpha(0,0,0,0) clip() asr(<0x1231521e0>) clipChain() ref=0x123153490 agr=0x123153490 (hitTestInfo 0x1) hitTestArea(x=0, y=0, w=6000, h=6000)
    BackgroundColor p=0x1237bba60 f=0x123153540(Block(div)(1)) key=4 bounds(0,0,6000,6000) layerBounds(0,0,6000,6000) visible(-30,0,6030,6000) componentAlpha(0,0,0,0) clip() asr(<0x1231521e0>) clipChain() uniform ref=0x123153490 agr=0x123153490 (opaque 0,0,6000,6000) (rgba 1,0,0,1)
    Text p=0x1237f2320 f=0x123153de8(Text(0)"fixed") key=67 bounds(-60,66,2039,1020) layerBounds(-60,66,2039,1020) visible(-30,0,6030,6000) componentAlpha(-60,66,2039,1020) clip() asr(<0x1231521e0>) clipChain() ref=0x123153490 agr=0x123153490


Since there's no nsDisplayFixedPosition item, it doesn't use the new code I'm adding and everything works the way it did before (which seems correct). Do you know if there's magic somewhere in the display list building code that also detects this scenario, or if maybe my test page is too simple somehow?

Comment 7

[Markus Stange [:mstange]]

Oh, interesting. Here's the condition that decides whether we create an nsDisplayFixedPosition:
https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/layout/generic/nsFrame.cpp#2983-2984

  bool useFixedPosition = disp->mPosition == NS_STYLE_POSITION_FIXED &&
    (nsLayoutUtils::IsFixedPosFrameInDisplayPort(this) || BuilderHasScrolledClip(aBuilder));

nsLayoutUtils::IsFixedPosFrameInDisplayPort returns false for frames that aren't attached to the document's viewport frame. But the scrolled clip condition could return true. So could you test this again on a page that has a scrolled clip on a fixed-to-transform element? I think layout/reftests/async-scrolling/fixed-pos-scrolled-clip-3.html exercises this case.

Comment 8

[Markus Stange [:mstange]]

Oh, hmm, you said try is green, so that test is obviously passing. But I'd like to understand why.

Comment 9

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

(In reply to Markus Stange [:mstange] from comment #8)
> Oh, hmm, you said try is green, so that test is obviously passing. But I'd
> like to understand why.

Well the reftest is passing because the pixels are correct, but hit-testing is not correct on layout/reftests/async-scrolling/fixed-pos-scrolled-clip-3.html. If I put the mouse over the black rect and scroll, it will scroll the RCD instead of the scrollframe. On non-WR it scrolls the scrollframe. So yeah I guess that's another case that I need to handle.

Comment 10

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

I wrote a second patch on top of the first to handle this other scenario, along with a mochitest.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12b3567b253e32873dfb8785364aa8b3c5f900aa

Comment 11

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

Attachment: Bug 1465935 - Handle another edge case with hit-testing inside fixed-pos items.

Transforms are containing blocks for fixed-pos items, so if a fixed-pos
item is inside a scrolled transform, then it should use that scrollframe
as the scroll target for hit-testing. This patch handles this case for
WebRender by stashing the appropriate ASR on the nsDisplayFixedPosition
item and using it instead of the presShell's root scrollframe in this
scenario.

The patch also adds a mochitest (which is basically a mochitested
version of the reftest in fixed-pos-scrolled-clip-3.html, with a
hit-test check to ensure that it's hitting the right scrollframe).

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

Comment 12

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

Comment on attachment 8982484 [details]
Bug 1465935 - Fix hit-testing for fixed-pos items inside iframes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248458/diff/1-2/

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8982484 [details]
Bug 1465935 - Fix hit-testing for fixed-pos items inside iframes.

https://reviewboard.mozilla.org/r/248458/#review256376

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 8984222 [details]
Bug 1465935 - Handle another edge case with hit-testing inside fixed-pos items.

https://reviewboard.mozilla.org/r/250020/#review256378

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b04f4c9b15ab
Fix hit-testing for fixed-pos items inside iframes. r=mstange
https://hg.mozilla.org/integration/autoland/rev/ada5a8476472
Handle another edge case with hit-testing inside fixed-pos items. r=mstange

Comment 16

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/b04f4c9b15ab
https://hg.mozilla.org/mozilla-central/rev/ada5a8476472