Don't use cross-doc checks for perspective scrolling.

ASSIGNED
Assigned to

Status

()

enhancement
P2
normal
ASSIGNED
6 months ago
15 days ago

People

(Reporter: emilio, Assigned: hiro)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

No description provided.
Blocks: 1523439

Would be pretty surprising if a perspective transform scrolled stuff in an
iframe for example.

Priority: -- → P3
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1885a467de3c
Don't use cross-doc checks for perspective scrolling. r=mattwoodrow

I don't really have time to figure those out right now :(

Assignee: emilio → nobody
Flags: needinfo?(emilio)

When this gets fixed up, could we fix the nsDisplayPerspective::CreateWebRenderCommands call too - Matt said that should also not be using the cross-document variant of the helper. I assume it should be fairly easy to add that in?

If not, we should be sure to open a separate bug so we don't forget that, and make it block 1523439.

Priority: P3 → P2

The tests (test_resizeby.html and test_pointerPreserves3DPerspective.html) in question work fine locally, so I just pushed a new try based on the current trunk.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=07b8aeeb92b7045c70466ea87039d9723db8bc77

The assertion happens only if the mochitest in question run in iframe.

Assignee: nobody → hikezoe
Status: NEW → ASSIGNED

So, test_pointerPreserves3DPerspective.html fails because of MOZ_ASSERT(aChild->GetApzc() != parent) in HitTestingTreeNode::SetLastChild, this is pretty much same as bug 1458145. As per a comment by Botond in bug 1458145 comment 4, it means the layer tree is malformed.

Though I don't quite understand how ASR stuff works, my speculation is that we need to preserve the current ASR if we couldn't find the appropriate ASR for perspective item in the same document, like this change.

Markus, does this change reasonable? The change also fixes the first test case in bug 1458145 (the second one doesn't cause the assertion now), so I am hoping I am on the right track.

That's said, the change makes perspective-scrolling-3.html fail on non-E10S Android (the test passes on E10S Android), so there needs some more work. But I guess it's due to another pre-existing issue on reftest-async-scroll or relevant stuff.

CCing Botond, he might have some ideas about the perspective-scrolling-3.html failure.

Flags: needinfo?(mstange)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)

CCing Botond, he might have some ideas about the perspective-scrolling-3.html failure.

The failure seems to be a direct consequence of the patch: the if-branch that's introduced is not taken, and therefore the stated purpose of that code block ("APZ needs us to put the scroll frame's FrameMetrics on our child transform ContainerLayer instead") is not achieved; the FrameMetrics gets put on the perspective layer itself, which results in the transforms being in the wrong order.

I haven't investigated in more detail, but I believe that null is a valid value for an ASR (it means "this content does not scroll with anything"), and therefore we should not be conditioning the ASR replacement on the ASR being null.

I don't think the patch has the desired effect on test_pointerPreserves3DPerspective.html, either. It causes the FrameMetrics to be placed on an ancestor of the perspective layer. I think that for correct async scrolling, we still want it to be the child.

Here is the relevant part of the client-side layers dump from test_pointerPreserves3DPerspective.html:

        ClientContainerLayer (0xba018800) [clip=(x=2, y=2, w=796, h=1116)] [transform=[ 0.408163 0; 0 0.408163; 89.7959 94.6939; ]] [effective-transform=[ 2.45 0; 0 2.45; 220 232; ]] [visible=< (x=-82, y=0, w=164, h=164); >] [extend3DContext] [metrics0={ [metrics={ [cb=(x=0.000000, y=0.000000, w=800.000000, h=1120.000000)] [sr=(x=0.000000, y=0.000000, w=980.000000, h=1372.000000)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=980.000000, h=1372.000000)] [cdp=(x=0.000000, y=0.000000, w=980.000000, h=1372.000000)] [scrollId=6] [rcd] [z=0.816] }] [color=rgba(255, 255, 255, 1.000000)] [scrollParent=3] }] [preScale=2.45, 2.45] [presShellResolution=1]
          ClientContainerLayer (0xba019c00) [transform=[ 0.408163 0; 0 0.408163; 0 0; ]] [effective-transform=[ 2.45 0; 0 2.45; 220 232; ]] [visible=< (x=-82, y=0, w=164, h=164); >] [combines3DTransformWithAncestors] [is3DContextLeaf] [usesTmpSurf] [preScale=2.45, 2.45] [presShellResolution=1]
            ClientTiledPaintedLayer (0xbcc4f800) [not visible] { hitregion=< (x=0, y=0, w=784, h=164); >} [opaqueContent]
              SingleTiledContentClient (0xb97227c0)
            ClientContainerLayer (0xba01ac00) [transform=[ 0.408163 0 0 0; 0 0.408163 0 0; -1.95918 -0.408163 1 -0.005; 0 0 0 1; ]] [effective-transform=[ 0.408163 0 0 0; 0 0.408163 0 0; -1.95918 -0.408163 1 -0.005; 0 0 0 1; ]] [perspective] [visible=< (x=-200, y=0, w=400, h=400); >] [extend3DContext] [presShellResolution=1]
              ClientContainerLayer (0xba01bc00) [transform=[ 1 0; 0 1; -200 0; ]] [effective-transform=[ 0.408163 0 0 0; 0 0.408163 0 0; -1.95918 -0.408163 1 -0.005; -81.6327 0 0 1; ]] [visible=< (x=0, y=0, w=400, h=400); >] [combines3DTransformWithAncestors] [is3DContextLeaf] [metrics0={ [metrics={ [cb=(x=0.000000, y=0.000000, w=800.000000, h=1120.000000)] [sr=(x=0.000000, y=0.000000, w=980.000000, h=1372.000000)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=980.000000, h=1372.000000)] [cdp=(x=0.000000, y=0.000000, w=980.000000, h=1372.000000)] [scrollId=6] [rcd] [z=2] }] [color=rgba(255, 255, 255, 1.000000)] [scrollParent=3] }] [presShellResolution=1]
                ClientTiledPaintedLayer (0xba01d000) [clip=(x=0, y=0, w=0, h=0)] [effective-transform=[ 0.408163 0; 0 0.408163; -81.6327 0; ]] [not visible]
                  SingleTiledContentClient (0xb9722940)
                ClientColorLayer (0xba01d400) [effective-transform=[ 0.408163 0; 0 0.408163; -81.6327 0; ]] [visible=< (x=0, y=0, w=400, h=400); >] { hitregion=< (x=0, y=0, w=400, h=400); >} [opaqueContent] [color=rgba(0, 0, 255, 1.000000)] [bounds=(x=0, y=0, w=400, h=400)]

The perspective layer is 0xba01ac00. The scrollId=6 FrameMetrics is placed on the both its ancestor 0xba018800, and its child 0xba01bc00. The current patch makes it so that it's only placed on the ancestor 0xba018800. I think what we want is that it's only placed on the child 0xba01bc00. However, I'm not fully sure about this. Markus or Matt can probably give a better opinion.

Thank you, Botond! Now I checked the frame tree of test_pointerPreserves3DPerspective.html, and noticed that the frame for perspective is not scrollable at all, whereas the perspective frames in perspective-scrolling-X.html test cases are scrollable. Also the comment above the GetASRForPerspective call says "If the perspective item is scrolled", so checking whether the perspective frame is scrollable is the right way to fix the assertion? It actually fixes the assertion and also fixes the crash test case in bug 1458145.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)

so checking whether the perspective frame is scrollable is the right way to fix the assertion?

I'm not sure. Is it possible to modify test_pointerPreserves3DPerspective.html in such a way that the perspective frame is scrollable but the bad layer structure still arises? (If so, that would suggest it's not the right way.)

(In reply to Botond Ballo [:botond] from comment #12)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)

so checking whether the perspective frame is scrollable is the right way to fix the assertion?

I'm not sure. Is it possible to modify test_pointerPreserves3DPerspective.html in such a way that the perspective frame is scrollable but the bad layer structure still arises? (If so, that would suggest it's not the right way.)

Thanks for a clever way. Indeed, with scrollable perspective frame, both the parent and the child of the perspective layer have the same scrollId metrics. (See https://gist.github.com/hiikezoe/039b74fa270d716de1dce84bbedc9d44)

It's really really hard to understand how clip and perspective stuffs interact with each other. I don't still understand it yet, but I found a clue in a comment by Markus in bug 1298218 comment 26.

different layers can have scroll metadata for the same scroll ID but with a different mScrollClip in that scroll metadata.

In case of test_pointerPreserves3DPerspective.html, the parent layer's scroll metadata has clip region, the child one has no clip region. The layer dump in bug 1458145 comment 4 is the same, the parent one has clip region, the child one doesn't have.

The case in bug 1466224 seems to be different though, if we can have different layers that they have the same scrollId scroll metadata but different mScrollClip, can we relax the assert condition in HitTestingTreeNode::SetLastChild()? (It seems to me we should skip calling SetApzcParent() there)

Botond?

Flags: needinfo?(botond)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)

The case in bug 1466224 seems to be different though, if we can have different layers that they have the same scrollId scroll metadata but different mScrollClip, can we relax the assert condition in HitTestingTreeNode::SetLastChild()? (It seems to me we should skip calling SetApzcParent() there)

I don't think we can relax the assert in HitTestingTreeNode::SetLastChild. Even if we skip calling SetApzcParent(), if a pair of ancestor/descendant layers both have scroll metadata with the same scroll id, the compositor will apply any async scrolling transform to that layer twice, which would be incorrect.

What I am wondering about, is why the layer 0xbcc4f800 in the layers dump from comment 10 is created. I don't have the corresponding display list dump for it at the moment, but my recollection is that there were no display items assigned to it. Perhaps we can prevent it from being created, and perhaps FrameLayerBuilder can then successfully put the scroll metadata on the descendant (0xba01bc00) only.

Flags: needinfo?(botond)

I spoke to Markus about this, and made a couple of realizations. First:

(In reply to Botond Ballo [:botond] from comment #15)

What I am wondering about, is why the layer 0xbcc4f800 in the layers dump from comment 10 is created.

It seems to be created by a hit-test-info display item. I'm not sure why that can't go on the sibling ContainerLayer.

Second, the ASR manipulation code that was linked to earlier only applies to cases where "the perspective-inducing frame is outside the scroll frame". This is not the case for test_pointerPreserves3DPerspective.html.

Unfortunately, that still doesn't give us an explanation for the bad layer structure in comment 10. Markus said this is a question for Matt.

Flags: needinfo?(mstange) → needinfo?(matt.woodrow)

I think the change to GetASRForPerspective is just wrong.

It's looking for the first ASR that is an ancestor of the perspective frame, so crossing document boundaries makes sense.

Flags: needinfo?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.