[Affected versions]:

• Firefox Latest Nightly 66.0a1
• Firefox Beta 65.0b9
• Firefox Release 64.0

[Affected platforms]:

• Win 10 x64
• Win 7 x64
• Ubuntu 18.04 x64
• macOS X 10.13

[Steps to reproduce]:

1. Access www.tumblr.com.
2. Reach the sign up landing page.
3. Scroll down to the bottom of the page.

[Expected result]:
-The website scrolls smoothly between every transition.

[Actual result]:
-Between the last to transitions there is a noticeable flicker.

[Regression range]:

• Will investigate as soon as possible.

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=83fb6c53e740cd7d3f8125a684cb5851c5ef4dd8&tochange=a2eccfbeb0ae9c9d71b76a9e6fcea48addaa84ca

Found commit message:
Bug 1320608 - Make sure we wait for the next frame in the case where the animation started at the current frame. r=birtles

Brian: Do you have some time to take a look at this regression?

Forgot Brian is on PTO for a little while. Clearing his NI.

Hiro: Maybe you have any thoughts here?

The flickering element has display:table, this is actually a regression caused by bug 1320608. Bug 132068 made transform animations on table kind elements run on the compositor. And the animation in question looks running on the compositor correctly. So it looks a gfx issue to me... I can see the background image in the middle of the transition (i.e. during the flicker). Anyway moving into DOM:Animation for now.

My understanding is we're not going to have another 64 dot release given the impending 65 release.

It looks like JS is toggling classes on these sections as they animate in and out.

While a section is shown or transitioning out it has the 'active' class. Once one of the subsequent sections is showing, it gets the 'old-hat' class. As a result it's possible for an element to have both the 'active' and 'old-hat' classes.

The active class is used to trigger the transitions that start when a particular section is shown.

The old-hat class is used in the following CSS declaration block:

.about-tumblr-showcase .section.old-hat {
transform:translateY(-100%)
}

The 'transform' property is transitioned, making the sections slide up.

I see the flicker most prominently when going backwards from the purple 'blogs' section back to the green 'about' section. It's really bad there.

The 'about' section is also 'display: table' so I believe this is the same bug. I am getting the Japanese version of the site though, and this is on Windows 10 without Web Render, so it may differ for others.

The only thing I notice in that green section is that the main graphic is a large inline SVG graphic. It has its opacity and transform transitioned for 0.5s and then JS is used to animate the transform of the individual grey background elements.

Unfortunately setting layers.draw-borders doesn't seem to produce any layer borders I can see.

I notice, however, that if I don't wait for the animation to finish then I don't get any flicker. As best I can tell, there seems to be some sort of synchronization issue that arises when the animated layer becomes no longer active. If I turn off OMTA there is no issue.

Adding Matt and Markus to see if they have any suggestions about where to look.

Brian, could you please find someone to look into this for our 67 soft freeze (Mar 11)?

(In reply to Brian Birtles (:birtles) from comment #6)

Adding Matt and Markus to see if they have any suggestions about where to look.

I don't have suggestions on where to look. I think a good next step would be to try to create a reduced testcase.

The fact that this only occurs on display:table content made me suspect there was something wrong with the way we set up the animations (since display:table has a separate primary/style frame and the transform, unlike other properties, goes on the transform frame only, even though the style frame still has the transform style applied).

However, even after fixing a number of these issues in bug 1527210 and locally in bug 1532568 I don't see any difference.

I also tried commenting out the code we have for updating transitions on the compositor but it didn't make any difference.

From my previous analysis the thing that stands out most is the comment:

"I notice, however, that if I don't wait for the animation to finish then I don't get any flicker. As best I can tell, there seems to be some sort of synchronization issue that arises when the animated layer becomes no longer active."

So I think it might make more sense to pass this on to graphics for further analysis.

Matt, do you know anyone who might be able to look into this?

I took a screen recording of transitioning from the second to last panel (orange) to the last.

I can see the orange pane smoothly transitioning up, and then for a single frame it jumps back to its original position, and then resumes the transition.

I see this happen both with and without WR, so it's unlikely to be a rendering bug.

Debugging this with RR would probably be fairly easy if anyone has time, otherwise I can try look further next week.

I think the easiest thing to do would be record the flicker, with the pref layers.dump=true. That should dump all the compositor side Layer trees to stdout, and you can locate the composite where the layer transform jumped.

Run the replay to that point, and inspect the animation data on the Layer (looks like we don't include much in the way of details in the Layer dump). From there you should be able to identify if the animation data is incorrect, or if not, then the compositor must be interpolating it incorrectly. The latter happens in AsyncCompositionManager::TransformShadowTree, which can be stepped through.

This is the layer tree through the broken frame.

The first layer tree has an async animation, and renders correctly.

The second layer tree there's no transform container, and we don't render the orange content all.

The third layer tree still has no transform container, but now renders correctly.

There's also a lot of layout assertions, fixing things might help.

Here's the client side Layer tree and display list for a good broken, and then a broken frame. Display lists are printed twice for each frame, use the second ("after optimization") one.

This isn't the same instance as the previous dump (so don't try to compare exact transform values), but is for the same glitch. In this case the orange is transitioning down into view, and right as it finishes (orange covers the whole screen), we get a frame without the orange painted at all.

In the first (working) frame we have "nsDisplayTransform p=0x15c78bc20 f=0x1594c8020(TableWrapper(div)(4) class:section create-section" but the building/visible rect has a height of 357 (app units, so just under 6 CSS pixels).

It looks like we've decided to pre-render this transform though, so the childrenBuidingRect has the full height (49140), allowAsync is true, the inner items have the large building/visible rects, and we have all the text items.

In the second (broken) frame, the equivalent nsDIsplayTransform has the same building/visible rect, but it looks like we're not pre-rendering, so childrenBuildingRect has the tiny size, and we haven't even built the text display items.

So it appears that the calculation of what's visible of the transform is entirely wrong, but when we prerender content (that we think is offscreen) it wallpapers over the problem.

Looking at the frame tree, the TableWrapper frame is out-of-flow, so the building rect will have been intersected with the overflow rect.

That makes the assertions super suspicious: ###!!! ASSERTION: InitialOverflowProperty must be set first.: 'frame->GetProperty(nsIFrame::DebugInitialOverflowPropertyApplied())'

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)

Looking at the frame tree, the TableWrapper frame is out-of-flow, so the building rect will have been intersected with the overflow rect.

That makes the assertions super suspicious: ###!!! ASSERTION: InitialOverflowProperty must be set first.: 'frame->GetProperty(nsIFrame::DebugInitialOverflowPropertyApplied())'

That could well be because we're failing to look up the primary frame correctly for display:table contents in RestyleManager. I fixed a number of instances of that in bug 1527210 (specifically changeset 5cf9e494f690) but there may be some I missed.

Your patch indeed fixes the assertion, but not the rendering bug.

Trying to add more logging changes the timing and stops the bug from occurring, so I'm struggling to make much more progress on OSX.

Will need to get an rr recording to work backwards though it.

(In reply to Brian Birtles (:birtles) from comment #14)

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)

Looking at the frame tree, the TableWrapper frame is out-of-flow, so the building rect will have been intersected with the overflow rect.

That makes the assertions super suspicious: ###!!! ASSERTION: InitialOverflowProperty must be set first.: 'frame->GetProperty(nsIFrame::DebugInitialOverflowPropertyApplied())'

That could well be because we're failing to look up the primary frame correctly for display:table contents in RestyleManager. I fixed a number of instances of that in bug 1527210 (specifically changeset 5cf9e494f690) but there may be some I missed.

One of the places I am still suspecting is EffectCompositor::HasAnimationsForCompositor(). It's probably still broken.

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

One of the places I am still suspecting is EffectCompositor::HasAnimationsForCompositor(). It's probably still broken.

Oh yeah, I found at least one call site (actually the only one I've checked so far) that is using it incorrectly:

• https://searchfox.org/mozilla-central/rev/b2d35912da5b2acecb0274eb113777d344ffb75e/layout/painting/nsDisplayList.cpp#1046-1048

IsTransformed() will return true for the primary frame but we should pass the style frame to EffectCompositor::HasAnimationsForCompositor.

There are more:

• https://searchfox.org/mozilla-central/rev/b2d35912da5b2acecb0274eb113777d344ffb75e/layout/painting/nsDisplayList.cpp#1869-1873
• https://searchfox.org/mozilla-central/rev/b2d35912da5b2acecb0274eb113777d344ffb75e/layout/painting/nsDisplayList.cpp#7731-7733
• https://searchfox.org/mozilla-central/rev/b2d35912da5b2acecb0274eb113777d344ffb75e/layout/painting/nsDisplayList.cpp#8122-8123

We should just make EffectCompositor::HasAnimationsForCompositor look up the style frame from the primary frame rather than make every call site remember to.

And RefusedAsyncAnimation() bit usage, here and probably somewhere too. I am not sure it's related to this glitches though.

i.e. around here https://searchfox.org/mozilla-central/rev/b2d35912da5b2acecb0274eb113777d344ffb75e/dom/animation/EffectCompositor.cpp#123 we should add a call to nsLayoutUtils::GetStyleFrame

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

And RefusedAsyncAnimation() bit usage, here and probably somewhere too. I am not sure it's related to this glitches though.

Yeah, I suspect that doesn't effect display:table content.

Here's a patch that seems to fix it for me but perhaps I was just lucky.

I'm also a little unsure how many of the other places in that function should use styleFrame (and if we should explicitly return false if aPropertySet is a subset of TYPE_TRANSFORM and we're on the inner table frame).

If this fixes it for you Matt then I guess this does belong in DOM: Animation after all.

Looking at the uses of aFrame in FindAnimationsForCompositor we have:

• Calls aFrame->PresContext()->Document()->GetPendingAnimationTracker()
  • We should have the same pres context for both frames so either should be fine here.
• Calls EffectCompositor::AllowCompositorAnimationsOnFrame(aFrame, warning)
  • Calls aFrame->RefusedAsyncAnimation().
    • That property will, as best I can tell, be set on the primary frame so we should use aFrame here.
  • Calls aFrame->GetContent() which should return the same for both style and primary frame (according to my testing anyway)
• Calls EffectCompositor::SetPerformanceWarning(aFrame, property, AnimationPerformanceWarning(warning))
  • Calls EffectSet::GetEffectSet(aFrame) which should use the style frame.
• Calls EffectCompositor::GetAnimationElementAndPseudoForFrame(aFrame)
  • This should use the style frame since although both primary and style frame will have the same content, their pseudo types will differ and its the pseudo type of the style frame we want.
• Calls effect->IsMatchForCompositor(aPropertySet, aFrame, *effects, effectWarning)
  • Calls mAnimation->ShouldBeSynchronizedWithMainThread(aPropertySet, aFrame, aPerformanceWarning)
    • Calls keyframeEffect->ShouldBlockAsyncTransformAnimations(aFrame, aPerformanceWarning)
      • Calls CanAnimateTransformOnCompositor(aFrame, aPerformanceWarning)
        • Calls aFrame->Combines3DTransformWithAncestors()
          • I'm pretty sure we want the primary frame here
        • Calls aFrame->StyleDisplay()->mTransformStyle
          • As below, transform-style probably needs to be inherited. Once we do that, it shouldn't matter which frame we use.
        • Calls aFrame->BackfaceIsHidden()
          • Again, we probably want to inherit backface-visibility so we can look this up on the primary frame.
        • Calls aFrame->IsSVGTransformed()
      • Calls aFrame->StyleDisplay()->HasIndividualTransform()
        • Assuming we correctly inherit the individual transforms to the table wrapper (Do we? I don't see the appropriate rule in ua.css but there probably should be one there) then it doesn't matter which frame we call this on
  • Calls aFrame->IsVisibleOrMayHaveVisibleDescendants()
    • visibility is inherited so I think this will be ok to call on the primary frame
  • Calls aFrame->IsScrolledOutOfView()
    • Again, I think the primary frame is fine for this.
  • Calls aFrame->GetContent()
    • Either is fine here
• Calls EffectCompositor::SetPerformanceWarning(aFrame, property, AnimationPerformanceWarning(effectWarning))
  • As above, should use the style frame here.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=982c0caf959c91c34f5ad85f5286c4c28394fbeb

If this patch works, it would probably be a good idea to go back and rename some of the arguments to these functions.

e.g.

• EffectCompositor::AllowCompositorAnimationsOnFrame → aPrimaryFrame
• EffectCompositor::SetPerformanceWarning → aStyleFrame
• EffectCompositor::GetAnimationElementAndPseudoForFrame → aStyleFrame
• KeyframeEffect::IsMatchForCompositor → aPrimaryFrame
• KeyframeEffect::ShouldBlockAsyncTransformAnimations → aPrimaryFrame
• Animation::ShouldBeSynchronizedWithMainThread → aPrimaryFrame

We should also possibly change ua.css to include:

*|*::-moz-table-wrapper {
  backface-visibility: inherit;
  rotate: inherit;
  scale: inherit;
  transform-style: inherit;
  translate: inherit;
}

(In reply to Brian Birtles (:birtles) from comment #24)

We should also possibly change ua.css to include:

*|*::-moz-table-wrapper {
  backface-visibility: inherit;
  rotate: inherit;
  scale: inherit;
  transform-style: inherit;
  translate: inherit;
}

Presumably this should probably include will-change too since ActiveLayerTracker checks that.

I thought I'd just tidy up the old patch, rename a few variables and put it up for review. However, the deeper I looked into this, the more bugs I discovered. This is part of the code is very brittle.

In particular, we are often fetching the EffectSet without being careful to use the style frame. At the same time, we probably shouldn't be using the style frame if, for example, we are looking for opacity animations (since that would lead us to thinking we have opacity animations on a table wrapper frame, for example).

After hacking away like mad for a while, I started to think we need to wrap up that logic into where we get an EffectSet itself. This patch attempts to do that and fix a number of other issues too.

It's not right yet (it barely compiles, will certainly fail some tests, and still needs splitting up) but it hopefully illustrates the approach I'd like to take towards fixing this.

Oh, and I just realized this won't compile on WIDGET_GTK because I dropped nsLayoutUtils::HasCurrentTransitions. The call sites for that should really be looking up the effect set by the element not the frame.

We test the transform-style of a frame in places like
KeyframeEffect::CanAnimateTransformOnCompositor but this will likely not work as
expected for display:table content since the transform-style will not be
inherited to the primary frame (and later in this patch series we will ensure
that we are dealing with the primary frame in
KeyframeEffect::CanAnimateTransformOnCompositor).

The individual transform properties are new but they should also be inherited so
that all the appropriate tests for "is this frame transformed?" produce the
correct result when these properties are applied.

Depends on D23278

The trouble with utility functions that take an nsIFrame is it's not clear what
the caller's intention is. For example, with
nsLayoutUtils::HasCurrentTransition, is the caller asking for transitions on
that frame? Or animations on both that frame and its corresponding
style/primary frame?

Probably the caller hasn't even thought about it and there are likely to be bugs
when display:table content is encountered.

Where practical it's much better to take an element/pseudo pair since it's clear
that the caller is concerned with all animations (or transitions in this case)
on the element regardless of how it is represented in the frame tree.

This patch updates nsLayoutUtils::HasCurrentTransition to take an element/pseudo
pair and moves it to mozilla::AnimationUtils at the same time.

Depends on D23279

It took me a long time to understand why
KeyframeEffect::HasEffectiveAnimationOfPropertySet behaved so differently to
KeyframeEffect::HasAnimationOfPropertySet. This patch attempts to clarify that
while making KeyframeEffect::HasEffectiveAnimationOnPropertySet a little more
generally useful. This will allow us to tidy up the various animation checks in
nsLayoutUtils later in this patch series.

Ultimately, however, we should make this check part of the regular compositor
animation vetting machinery in bug 1534884. That should remove a number of
inconsistencies such that we don't need the extended comments added in this
patch.

Depends on D23280

There are many bugs regarding our use of EffectSet::GetEffectSet(nsIFrame*)
because the intention of the caller is not clear. If it is called for the
primary frame of display:table content do we expect it to get the EffectSet
associated with the style frame or not? Generally it depends on if we are
looking for transform animations or not.

Rather than inspecting each call site and making it choose the appropriate frame
to use, this patch introduces a new method to EffectSet to get the appropriate
EffectSet based on the properties the caller is interested in.

This patch also uses this function in FindAnimationsForCompositor which in turns
fixes the glitching observed on Tumblr that arose since a number of places in
our display list code were passing the style frame to
EffectCompositor::HasAnimationsForCompositor.

Over the remainder of this patch series we will convert more callers of
EffectSet::GetEffectSet(nsIFrame*) to this new method before renaming
EffectSet::GetEffectSet to GetEffectSetForStyleFrame to make clear how the
method is intended to work.

Depends on D23281

For most of the functions we call on this frame there will be no difference in
result since the transform styles are inherited from the style frame to the
primary frame. However, for Combines3DTransformWithAncestors() at least, which
calls IsCSSTransformed(), the result will differ.

Depends on D23282

Depends on D23283

Depends on D23284

Depends on D23285

I did expect that automated test cases are coming at last. :)

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

I did expect that automated test cases are coming at last. :)

Yeah, that would be nice. Unfortunately I don't have any good ideas for how to test these. (Except the second patch regarding will-change: transform. I plan to add a test for that.)

(In reply to Brian Birtles (:birtles) from comment #38)

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

I did expect that automated test cases are coming at last. :)

Yeah, that would be nice. Unfortunately I don't have any good ideas for how to test these. (Except the second patch regarding will-change: transform. I plan to add a test for that.)

Does ActiveLayerTracker::IsScaleSubjectToAnimation return correct value for the table frame? If it doesn't we have a chance to write a test case at least for this very case.

Oh wait, with those fixes transform animation starting with none (or having positive delay) on a table element runs on the compositor properly? If so, we can easily add a test case for this.

It looks like we don't need the second patch in this series after all. I'm not sure quite how it works, but we seem to apply the will-change style correctly already for display:table content. It might be worth adding a test case anyway just to ensure we continue to apply this correctly.

Depends on D23286

Depends on D23614

Currently the way we set the NS_FRAME_MAY_BE_TRANSFORMED frame bit for transform
animations fails to take into account the primary/style frame distinction for
display:table content.

This patch moves setting that bit for animations to the point where we already
have a handle on the appropriate EffectSet and already detect the primary/style
frame distinction.

This should be equivalent because:

a) Although it is inside a branch that is only run when |mContent| is set,
nsLayoutUtils::HasAnimationOfPropertySet will return false if the passed-in
frame does not have associated content (see
EffectCompositor::GetAnimationElementAndPseudoForFrame).

b) EffectSet::MayHaveTransformAnimation() should be set if we have any
transform animations in the EffectSet so this should be equivalent to
querying nsLayoutUtils::HasAnimationOfPropertySet.

The only other consideration is that this code is only executed when aPrevInFlow
is nullptr. As a result, this patch also updates the branch where aPrevInFlow is
set to copy the NS_FRAME_MAY_BE_TRANSFORMED bit in that case too.

Depends on D23635

https://hg.mozilla.org/mozilla-central/rev/dd7400edcf02
https://hg.mozilla.org/mozilla-central/rev/925ae534d521
https://hg.mozilla.org/mozilla-central/rev/b6614bc4be3f
https://hg.mozilla.org/mozilla-central/rev/8d9300956e10
https://hg.mozilla.org/mozilla-central/rev/296f63c52362
https://hg.mozilla.org/mozilla-central/rev/a87a6c5b5550
https://hg.mozilla.org/mozilla-central/rev/e5bf7eaa0189
https://hg.mozilla.org/mozilla-central/rev/aa46ab774756
https://hg.mozilla.org/mozilla-central/rev/3dbf8012e5a4
https://hg.mozilla.org/mozilla-central/rev/39e15f2bd75c
https://hg.mozilla.org/mozilla-central/rev/e7c2055118e5
https://hg.mozilla.org/mozilla-central/rev/93b549966e0a