1451424 - Audit hit testing with blob images

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Markus Stange [:mstange]]

We don't produce any hit test items within blob images at the moment. This may lead to wrong hit testing in the presence of SVGs.
Notably, hit-testable elements inside an SVG do not need to be contained in the bounds of the <svg> element's frame, if the SVG element is overflow:visible.

One proposal that came up during conversations about this was to emit a dispatch-to-content hitregion item for the size of the entire blob image, for every blob image.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Markus fleshed this out some more:
    - If the <svg> element has overflow:hidden and pointer-events:auto (the default), create one hit testing item with the <svg> frame's rectangle. This achieves correct hit testing and can be handled on the compositor without involvement of the main thread.
    - If the <svg> element has overflow:visible, create a dispatch-to-content-region hit testing item behind the regular hit testing item. That dtc item covers the entire bounds of the nsDisplaySVGWrapper and the regular hit test item only covers the SVG frame's rect.
    - If the <svg> element has pointer-events:none (which can be overridden from the inside), make the regular hit test item with the SVG frame's rect also dispatch-to-content.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Kats, does this all seem reasonable to you?

Comment 3

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

Seems reasonable, with the caveat that if the SVG element has apz-aware listeners then the hit-testing item should be dispatch-to-content (this is true for all elements, just making sure it applies here as well).

Also note that although we normally create WR hit-test display items that are separate from the "painting" display items, it's possible to embed the hit-test info directly onto the painting items, since every WR display item has fields for hit info. If you're going to be adding special code to the blob image codepaths to generate hit-test info, it might be easier/convenient to just emit the hit-test info on the display items that you're generating anyway. I had planned to do this as a further optimization for other items at some point.

Comment 4

[Markus Stange [:mstange]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> Markus fleshed this out some more:
>     - If the <svg> element has overflow:hidden and pointer-events:auto (the
> default), create one hit testing item with the <svg> frame's rectangle. This
> achieves correct hit testing and can be handled on the compositor without
> involvement of the main thread.

We need to add a few more conditions to this case (the one where we're trying to avoid dispatch-to-content regions):
 - The CSS touch-action property must not be used on any elements inside the <svg>
 - There must not be a <foreignObject> inside the SVG. Once bug 1449634 is fixed, those will be treated as an "active" item that gets its own WR StackingContext and causes the outer <svg> to be split into multiple groups. <foreignObject> can contain APZ- scrollable HTML elements.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
>     - If the <svg> element has overflow:visible, create a
> dispatch-to-content-region hit testing item behind the regular hit testing
> item. That dtc item covers the entire bounds of the nsDisplaySVGWrapper and
> the regular hit test item only covers the SVG frame's rect.

In this case, if there are animated transforms inside the <svg>, those can move outside of the nsDisplaySVGWrapper's bounds off-main-thread. So we probably need some dispatch-to-content hit test items for those animated stacking contexts (or the image items for the groups inside of them) as well.

Comment 5

[Aria Beingessner [:Gankra]]

Attachment: Bug 1451424 - emit a dispatch-to-content hittest region for all svg blobs.

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

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8974463 [details]
Bug 1451424 - emit a dispatch-to-content hittest region for all svg blobs.

https://reviewboard.mozilla.org/r/242814/#review248630

I think we need to emit this for each blob, where we push the blob image.

Otherwise if you have an off-main-thread animated transform in the middle of the SVG, that animated piece might not be included in the dispatch-to-content region. If you emit the item during EndGroup, the animated piece will have its own dispatch-to-content item and it'll along with it, and then things should work correctly.

Comment 7

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

Comment on attachment 8974463 [details]
Bug 1451424 - emit a dispatch-to-content hittest region for all svg blobs.

https://reviewboard.mozilla.org/r/242814/#review248688

Dropping flag until Markus' concern is addressed. But the actual code to generate the hit test info item seems reasonable enough. I'd prefer less usage of "auto" but they're local variables so I don't care too much about it.

Comment 8

[Aria Beingessner [:Gankra]]

Comment on attachment 8974463 [details]
Bug 1451424 - emit a dispatch-to-content hittest region for all svg blobs.

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

Comment 9

[Aria Beingessner [:Gankra]]

Addressed markus' comments

Comment 10

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

Comment on attachment 8974463 [details]
Bug 1451424 - emit a dispatch-to-content hittest region for all svg blobs.

https://reviewboard.mozilla.org/r/242814/#review248696

::: gfx/layers/wr/WebRenderCommandBuilder.cpp:323
(Diff revision 2)
> +  bool mBackfaceVisible;
>    LayerIntRect mLayerBounds;
>    Maybe<wr::ImageKey> mKey;
>    std::vector<RefPtr<SourceSurface>> mExternalSurfaces;
>  
>    DIGroup() : mAppUnitsPerDevPixel(0) {}

new fields need initialization here

::: gfx/layers/wr/WebRenderCommandBuilder.cpp:646
(Diff revision 2)
> -    PushImage(aBuilder, bounds);
> +    PushImage(aBuilder, layoutBounds);
> +
> +    // Emit a dispatch-to-content hit test region covering this area
> +    auto hitInfo = CompositorHitTestInfo::eVisibleToHitTest |
> +                   CompositorHitTestInfo::eDispatchToContent;
> +
> +    aBuilder.SetHitTestInfo(mScrollId, hitInfo);
> +    aBuilder.PushRect(layoutBounds, layoutBounds, mBackfaceVisible, wr::ToColorF(gfx::Color()));
> +    aBuilder.ClearHitTestInfo();

So what might be preferable is to just call SetHitTestInfo before the PushImage call, and then ClearHitTestInfo after the PushImage call, and skip the PushRect. That will set the hit-test info directly on the image display item and you won't need the rect.

Also it's not clear to me why the PushImage call uses backfacevisible=true always while the hit info uses the wrapping item's backface visibility. I would expect the two to be the same (which is something that would happen for free with my above suggestion).

Comment 11

[Aria Beingessner [:Gankra]]

Comment on attachment 8974463 [details]
Bug 1451424 - emit a dispatch-to-content hittest region for all svg blobs.

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

Comment 12

[Aria Beingessner [:Gankra]]

I moved the code to PushImage, as I wasn't handling the "no invalidation" case right otherwise. Also addressed kats' comments.

Comment 13

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

Comment on attachment 8974463 [details]
Bug 1451424 - emit a dispatch-to-content hittest region for all svg blobs.

https://reviewboard.mozilla.org/r/242814/#review248758

Comment 14

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

Markus should review the updated version too, I'm not confident in the svg bits.

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8974463 [details]
Bug 1451424 - emit a dispatch-to-content hittest region for all svg blobs.

https://reviewboard.mozilla.org/r/242814/#review249050

Looks good!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2dac19dd1ea1
emit a dispatch-to-content hittest region for all svg blobs. r=kats,mstange

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2dac19dd1ea1