1449634 - Handle foreignobject better with blob invalidation

Status: RESOLVED FIXED

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

Comment 1

[Markus Stange [:mstange]]

This is intended to fix APZ scrolling inside scrollable elements which are inside a <foreignObject> inside an <svg>.

We need to:
 - Introduce a display item type called nsDisplayForeignObject
 - Make it wrap the contents of the foreign object frame
 - Detect it by its display item type in WebRenderCommandBuilder.cpp and cause it to set mDoGrouping to false for its contents, see the TODO comment at https://searchfox.org/mozilla-central/rev/08df4e6e11284186d477d7e5b0ae48483ecc979c/gfx/layers/wr/WebRenderCommandBuilder.cpp#1307-1311

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1449634. Handle foreignobject better with blob invalidation.

Turn off grouping and switch to regular WebRender building when
we encounter a foreignobject.

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

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 8972995 [details]
Bug 1449634. Handle foreignobject better with blob invalidation.

https://reviewboard.mozilla.org/r/241538/#review247600

::: layout/painting/nsDisplayList.cpp:10143
(Diff revision 1)
> +nsDisplayForeignObject::BuildLayer(nsDisplayListBuilder* aBuilder,
> +                                   LayerManager* aManager,
> +                                   const ContainerLayerParameters& aContainerParameters)
> +{
> +  ContainerLayerParameters newContainerParameters = aContainerParameters;
> +  newContainerParameters.mDisableSubpixelAntialiasingInDescendants = true;

Why?

::: layout/painting/nsDisplayList.cpp:10163
(Diff revision 1)
> +                                             aResources,
> +                                             aSc,
> +                                             aManager,
> +                                             aDisplayListBuilder);

indent

::: layout/painting/nsDisplayList.cpp:10167
(Diff revision 1)
> +    return nsDisplayWrapList::CreateWebRenderCommands(aBuilder,
> +                                             aResources,
> +                                             aSc,
> +                                             aManager,
> +                                             aDisplayListBuilder);
> +  } else {

let's make the return false an early return

Comment 4

[Jeff Muizelaar [:jrmuizel]]

This has some test failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2fc9c4587dd3cd8dc68b0965fab9efa5d154bed&selectedJob=195395799

Comment 5

[Aria Beingessner [:Gankra]]

It looks like the only failures are just Bug 1452337 rearing its head, in which case it should be fine for us to proceed with this patch. Sent reftest expectation adjustments to jrmuizel.

Comment 7

[Matt Woodrow (:mattwoodrow)]

As mentioned in the duplicate, this affects our tscrollx talos test.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e2a6bb9192d
Handle foreignobject better with blob invalidation. r=mstange

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4e2a6bb9192d

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

a perf improvement:
== Change summary for alert #15704 (as of Fri, 07 Sep 2018 00:45:49 GMT) ==

Improvements:

 24%  tscrollx linux64-qr opt e10s stylo     0.41 -> 0.31

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=15704