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

RESOLVED FIXED in Firefox 62

Status

()

defect
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: kats, Assigned: kats)

Tracking

(Blocks 1 bug)

Other Branch
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments)

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
Try push is green; the only orange is due to a patch from bug 1465892 which I'll hold off on.
Comment hidden (mozreview-request)
Assignee

Comment 4

a year ago
mozreview-review
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 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.
(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?
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.
Oh, hmm, you said try is green, so that test is obviously passing. But I'd like to understand why.
(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.
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8982484 [details]
Bug 1465935 - Fix hit-testing for fixed-pos items inside iframes.

https://reviewboard.mozilla.org/r/248458/#review256376
Attachment #8982484 - Flags: review?(mstange) → review+
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
Attachment #8984222 - Flags: review?(mstange) → review+

Comment 15

a year ago
Pushed by kgupta@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b04f4c9b15ab
https://hg.mozilla.org/mozilla-central/rev/ada5a8476472
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1467260
You need to log in before you can comment on or make changes to this bug.