Though I didn't know that mFrame of nsDisplayOpacity for continuations frame is the last continuation frame (at least on an element in layout/reftests/css-animations/continuation-opacity.html). Whereas KeyframeEffectReadOnly::GetAnimationFrame() returns the first continuation instead. So we are trying to add an WebRenderAnimationData to different frames. This inconsistency makes somehow continuation-opacity.html failure (bug 1437036). One clearly visible effect of the inconsistency is that the opacity animation in the reftest doesn't run on the compositor. If we use the first continuation frame for WebRenderUserData, the reftest passes with the patch for bug 1437036). I don't know enough knowledge about continuations, but I believe continuation doesn't happen for image frame, so it will not be a problem for WebRenderImageData. I have no idea about other types (WebDenderGroupData and WebRenderFallbackData). So I just pushed a try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=4af827b76b4f5b358673f2bded289569c9ec8947 And here is a try with the patch for bug 1437036; https://treeherder.mozilla.org/#/jobs?repo=try&revision=0929cf9bb560a36b6e2ee186b7ebd7cd721ef3be

Pretty broken. :/ I am going to call nsLayoutUtils::FirstContinuationOrIBSplitSibling only for WebRenderAnimationData. https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d3651d0856708419aa1cc1dce630a58ae72a3e2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28ea9462f8825c3e2dd9e267beb982fdd087f922 There was a missing closing parenthesis.

I've changed to convert the first continuation frame to the last one in AnimationInfo::GetGenerationFromFrame(). With this approach, mFrame for all kind of WebRenderUserData matches nsDisplayItem's mFrame, but it's slightly slower than the way to make WebRenderAnimationData have the first continuation frame since converting the frame gets called every time we call GetGenerationFromFrame. But it will not a big problem since opacity animation on continuation is merely used. Here is a try with the patch for bug 1437036. continuation-opacity.html passed there; https://treeherder.mozilla.org/#/jobs?repo=try&revision=87ddf07c0257f20eef2626715f5d1d9b9d83ea09

Comment on attachment 8973561 [details] Bug 1459388 - Use the last continuation frame in case of continuation to find WebRenderAnimationData. https://reviewboard.mozilla.org/r/241910/#review247760 Can we also add an assertion to verify the assumption that aFrame either is not part of a continuation/IBSplit, or that it is the first sibling?

(In reply to Matt Woodrow (:mattwoodrow) from comment #6) > Comment on attachment 8973561 [details] > Bug 1459388 - Use the last continuation frame in case of continuation to > find WebRenderAnimationData. > > https://reviewboard.mozilla.org/r/241910/#review247760 > > Can we also add an assertion to verify the assumption that aFrame either is > not part of a continuation/IBSplit, or that it is the first sibling? Sure, it should. Thanks for the review!

A try with the assertion; https://treeherder.mozilla.org/#/jobs?repo=try&revision=77a160820a0819ea13d08853a4f9aa35949181ab

Comment on attachment 8973560 [details] Bug 1459388 - Drop 'explicit' for constructors that take two arguments. https://reviewboard.mozilla.org/r/241908/#review247876

Comment on attachment 8973561 [details] Bug 1459388 - Use the last continuation frame in case of continuation to find WebRenderAnimationData. https://reviewboard.mozilla.org/r/241910/#review247878

Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/035d920a1e28 Drop 'explicit' for constructors that take two arguments. r=kats https://hg.mozilla.org/integration/autoland/rev/06ac03b612a7 Use the last continuation frame in case of continuation to find WebRenderAnimationData. r=kats,mattwoodrow

Kats, is there any reason for nsDisplayItem using the last continuation? The first continuation should never go away, however, last continuations can go away during reflow.

I don't really know why that is, sorry. If the last continuations can go away during reflow wouldn't that affect the non-WR codepath? If I'm reading the code right we would end up discarding and recreating the layer for that item instead of reusing it.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15) > I don't really know why that is, sorry. If the last continuations can go > away during reflow wouldn't that affect the non-WR codepath? If I'm reading > the code right we would end up discarding and recreating the layer for that > item instead of reusing it. Sounds likely. Matt, do you know why that is? Generally all the owned data by a given frame should be in the first continuation...

Ah, so this is because of merging. Bug 1399527 discusses ways we could do better for this, input would be appreciated. This occurs when an element that creates a stacking context (like opacity), gets split into multiple continuation frames. When display list building, we visit each continuation frame separately, and build an nsDisplayOpacity wrapping the contents of that subtree. When we get to layer building, we detect sets of consecutive nsDisplayOpacity items that are all from the same split Element, and merge them back together to get a single display items that represents the set of split frames (and lets us draw all the content within a single temporary surface, and get correct group opacity rendering). Even though the merged item represents multiple frames, we still have to pick one to be mFrame, and it's an implementation detail that chooses the last one (though unfortunately a detail that is causing a bug here). It would be totally ok to change the merging logic to pick the first frame instead. Retained data for the display item (like Layer assignments, and invalidation data) is stored for all the frames in the merged display item, not just mFrame (see DisplayItemData::AddFrame), so we should already be handling the case where the last continuation gets recreated without losing our Layer.

https://hg.mozilla.org/mozilla-central/rev/035d920a1e28 https://hg.mozilla.org/mozilla-central/rev/06ac03b612a7

(In reply to Matt Woodrow (:mattwoodrow) from comment #17) > When we get to layer building, we detect sets of consecutive > nsDisplayOpacity items that are all from the same split Element, and merge > them back together to get a single display items that represents the set of > split frames (and lets us draw all the content within a single temporary > surface, and get correct group opacity rendering). Does this happen in FrameLayerBuilder? Is there equivalent code that runs on the WR side?

Here's the equivalent WR code: https://searchfox.org/mozilla-central/rev/f30847c12e5fb13791401ed4330ced3e1b9c8d81/gfx/layers/wr/WebRenderCommandBuilder.cpp#1211-1229

Ah, makes sense. Thanks!