1961140 - [css-view-transitions] Avoid clips from ancestors to affect view-transition captures (was: css/css-view-transitions/new-content-ancestor-clipped-2.html fails)

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P3)

Description

[Emilio Cobos Álvarez [:emilio]]

.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1961140 - Make view transition contents not be affected by ancestor clips. r=nical,#vt

This also improves the rendering of new-content-ancestor-clipped.html,
but doesn't fully fix it. I think there's something going on with the
box shadow / ink overflow coordinates.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfe3b36a0ea9
Make view transition contents not be affected by ancestor clips. r=nical

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1961140 - Null out ASR for view transition captured contents. r=nical,mstange,botond

We don't want these to scroll relative to their regular ASR or anything
really, as they get captured and removed from the rendering
conceptually.

This is needed to avoid "finite clip" assertions with the previous patch, but
it also fixes cases when scrolling creates bad rendering during live capturing
(e.g., this fixes the test-case in bug 1960839 for me).

Comment 4

[Iulian Moraru]

Backed out for causing debug-only assertions.

• Push with failures

• Backout link

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Yup, comment 3 should take care of them...

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

This bug has been marked as a regression. Setting status flag for Nightly to affected.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

So this seems to cause (other) ASR / APZ invariants to break like css/css-view-transitions/inline-with-offset-from-containing-block.html triggering:

0:08.38 pid:178114 [178114] Assertion failure: false (Two layers that scroll together have different ancestor transforms), at /home/emilio/src/moz/gecko-5/gfx/layers/apz/src/APZCTreeManager.cpp:1459

That said, it seems that for view transition snapshots, we shouldn't be considering them as scrollable via APZ or so... What's the easiest way of basically marking these as not be scrolled by ancestors and so on? Do I need to define a separate ASR / scroll clip / something else?

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Botond suggested trying to nerf the two apzEnabled bits in here when inside a view transition capture. Let's see how that goes, will try tomorrow.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

So I tried that here, and it doesn't seem so easy at least. It seems that if we have a scroller child, that child's contents need to be at the right scroll position (that's the css/css-view-transitions/scroller.html and css/css-view-transitions/scroller-child.html regressions in that try run...).

Any other suggestions? :)

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Copying from Matrix (from Botond):

emilio: After paging back in the discussion in this bug, I think I understand the problem: WebRender has a weirdness where, if during WebRender display list building, you create a ScrollFrame spatial node, that messes up the rendering in a way that's only fixed if you also have corresponding APZ state that sends the right messages to WebRender.
So, if we're going to disable the creation of APZ state for view transition captured contents, we need to make a corresponding change to disable the creation of ScrollFrame spatial nodes.
I wrote up a patch to do this, and confirmed that it makes the scroller.html and scroller-child.html testcases pass.

Where the patch is https://hg-edge.mozilla.org/try/rev/5c3025fd7c1ce426eb9cff3ca4bbf2bc6a6a917a, which looks sensible to me.

Comment 11

[Botond Ballo [:botond]]

Clearing needinfo, this should hopefully be addressed by the combination of Emilio's patch to not build WebRenderScrollData for content inside a view transition capture, and my patch to not activate scroll frames inside such content. Feel free to re-ping if other APZ related issues come up.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:tnikkel, could you have a look please?

For more information, please visit BugBot documentation.

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1961140 - Do not activate scroll frames inside view transition captured content. r=#vt,nical,tnikkel

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1961140 - Don't create compositor hit test info in view transition captured content. r=#vt,nical,botond

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1961140 - Don't create WebRender scroll data inside view transition captures. r=#vt,nical,botond

Comment 16

[Phabricator Automation]

Comment on attachment 9485731 [details]
Bug 1961140 - Don't create WebRender scroll data inside view transition captures. r=#vt,nical,botond

Revision D248043 was moved to bug 1965424. Setting attachment 9485731 [details] to obsolete.

Comment 17

[Phabricator Automation]

Comment on attachment 9485730 [details]
Bug 1961140 - Don't create compositor hit test info in view transition captured content. r=#vt,nical,botond

Revision D248042 was moved to bug 1965424. Setting attachment 9485730 [details] to obsolete.

Comment 18

[Phabricator Automation]

Comment on attachment 9485729 [details]
Bug 1961140 - Do not activate scroll frames inside view transition captured content. r=#vt,nical,tnikkel

Revision D248041 was moved to bug 1965424. Setting attachment 9485729 [details] to obsolete.

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1961140 - Don't update container ASR for a new VT node. r=mstange,tnikkel

VT nodes will have a null ASR, but that doesn't affect its container's
ASR.

Without this change, we might hit display lists like:

  nsDisplayTransform p=0x7f1c221f6130 f=0x7f1c21e19ba0(Block(div)(0) class:outer) key=67 bounds(829,471,75112,1338) componentAlpha(829,471,75112,1338) clip(0,0,76800,57330) asr() clipChain(0x7f1c221f5288 <0,0,76800,57330> [0xg7f1c21e191b0], 0x7f1c221f5170 <0,0,76800,57330> [root asr])  reuse-state(None)[ 0.99 0; 0 0.99; 28.64 16.22; ] prerender(no) chi[rr 298151 304623]ldrenBuildingRect(x=0, y=0, w=75840, h=1320)
    CompositorHitTestInfo p=0x7f1c221f5440 f=0x7f1c21e19ba0(Block(div)(0) class:outger) key=19 bounds(0,0,0,0) componentAlpha(0,0,0,0) clip() asr(<0x7f1c21e191b0>) clipChain()  hitTestInfo(0x1) hitTestArea(0,0,75[rr 298151 304627]840,1320) reuse-state(None)
    VTCapture p=0x7f1c221f5fb8 f=0x7f1c21e19c68(Block(div)(1) class:inner) key=69 bounds(-30,-15,758g70,1350) componentAlpha(-30,-15,4552,1350) clip() asr() clipChain()  (opaque 0,0,75840,1320) reuse-state(None) (flags 0x0) (scro[rr 298151 304631]lltarget 0)
      Text p=0x7f1c221f55c8 f=0x7f1c21e19d30(Text(0)"Some gtext") key=64 bounds(-30,-15,4552,1350) componentAlpha(-30,-15,4552,1350) clip() asr() clipChain()  reuse-state(None) ("Some tex[rr 298151 304639]t")

After merging (with basically the test-case in
https://bugzilla.mozilla.org/show_bug.cgi?id=1968672), where the
CompositorHitTestInfo ends up with the right ASR (but its containing
nsDisplayTransform doesn't).

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ab1fcc205d97
https://hg.mozilla.org/integration/autoland/rev/c3452ad78518
Make view transition contents not be affected by ancestor clips. r=nical,tnikkel
https://github.com/mozilla-firefox/firefox/commit/b0cfcb904e16
https://hg.mozilla.org/integration/autoland/rev/abec7e080843
Don't update container ASR for a new VT node. r=tnikkel
https://github.com/mozilla-firefox/firefox/commit/3ee77d6e6f35
https://hg.mozilla.org/integration/autoland/rev/81ee9257f3d2
1966724: apply code formatting via Lando

Comment 21

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/c3452ad78518
https://hg.mozilla.org/mozilla-central/rev/abec7e080843
https://hg.mozilla.org/mozilla-central/rev/81ee9257f3d2