Closed Bug 1527210 Opened 6 years ago Closed 6 years ago

transform: none and opacity: 1 animations on display: table elements do not create a stacking context

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

Test case for changing the effect
4.37 KB, patch
Details | Diff | Splinter Review
Reftest for creating a stacking context for opacity:1 animation on display:table
439 bytes, text/html
Details
Bug 1527210 - Be more consistent about only applying transforms to primary frames; r?hiro
47 bytes, text/x-phabricator-request
Details | Review
Bug 1527210 - Fix handling of animation of opacity on display:table; r?hiro
47 bytes, text/x-phabricator-request
Details | Review
Bug 1527210 - Drop KeyframeEffect::MaybeUpdateFrameForCompositor; r?hiro
47 bytes, text/x-phabricator-request
Details | Review

This is a test case for changing the effect in such a case. It currently fails the following assertion:

https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/layout/base/RestyleManager.cpp#1194-1200

with stack as follows:

0:24.13 GECKO(15136) [Child 14492, Main Thread] ###!!! ASSERTION: Unexpected UpdateTransformLayer hint: '!(aChange & nsChangeHint_UpdateTransformLayer) || aFrame->IsTransformed() || aFrame->StyleDisplay()->HasTransformStyle()', file c:/Users/Brian/src1/layout/base/RestyleManager.cpp, line 1200
...
0:25.74 GECKO(15136) #01: mozilla::PresShell::DoFlushPendingNotifications (c:\Users\Brian\src1\layout\base\PresShell.cpp:4138)
0:25.76 GECKO(15136) #02: nsRefreshDriver::Tick (c:\Users\Brian\src1\layout\base\nsRefreshDriver.cpp:1883)
0:25.77 GECKO(15136) #03: mozilla::RefreshDriverTimer::TickRefreshDrivers (c:\Users\Brian\src1\layout\base\nsRefreshDriver.cpp:313)
0:25.77 GECKO(15136) #04: mozilla::RefreshDriverTimer::Tick (c:\Users\Brian\src1\layout\base\nsRefreshDriver.cpp:338)
0:25.79 GECKO(15136) #05: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver (c:\Users\Brian\src1\layout\base\nsRefreshDriver.cpp:692)
0:25.79 GECKO(15136) #06: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync (c:\Users\Brian\src1\layout\base\nsRefreshDriver.cpp:593)
...

Wait, that might be wrong. I failed to test that without a few other patches applied (I thought I popped them but I just noticed I didn't.)

(In reply to Brian Birtles (:birtles, away Feb 8, 11, 18) from comment #1)

Created attachment 9043188 [details] [diff] [review]
Test case for changing the effect

This is a test case for changing the effect in such a case. It currently fails the following assertion:

https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/layout/base/RestyleManager.cpp#1194-1200

with stack as follows:

0:24.13 GECKO(15136) [Child 14492, Main Thread] ###!!! ASSERTION: Unexpected UpdateTransformLayer hint: '!(aChange & nsChangeHint_UpdateTransformLayer) || aFrame->IsTransformed() || aFrame->StyleDisplay()->HasTransformStyle()', file c:/Users/Brian/src1/layout/base/RestyleManager.cpp, line 1200

This probably means that we don't properly set neither NS_FRAME_MAY_BE_TRANSFORMED nor mMayHaveTransformAnimation. I believe the |aFrame| is the style frame for the table element.

(In reply to Brian Birtles (:birtles, away Feb 8, 11, 18) from comment #2)

Wait, that might be wrong. I failed to test that without a few other patches applied (I thought I popped them but I just noticed I didn't.)

Confirmed that this test does fail and the assertion also fails on a trunk build (from a few days ago). As a result, I don't think this needs to block bug 1524480.

Moving the patch that drops KeyframeEffect::MaybeUpdateFrameForCompositor from bug 1524480 to here as requested.

See Also: → 1528301

I started looking into this.

One thing I notice is that in a couple of places we are setting transform flags / bits on continuations:

As far as I can tell, however, we don't need to do that for transforms. Transforms only apply to transformable elements which do not include (non-replaced) inline boxes.

I believe that is why nsIFrame::HasOpacityInternal checks continuations but nsIFrame::HasAnimationOfTransform does not (and why bug 1406211 only changed opacity and not transforms).

As part of this bug, it's probably worth try to remove those parts where we set transform flags/bits on continuations.

Assignee: nobody → bbirtles
Status: NEW → ASSIGNED

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

Oops. I just updated this line in Bug 1505225 [1]. You may need to rebase this after autoland is merged to m-c. :)

[1] https://phabricator.services.mozilla.com/D20412

We also have continuations for blocks, when they're split within a multicol. And I think transforms do apply there, although what they do is an interesting question...

(In reply to Boris Chiou [:boris] from comment #7)

Oops. I just updated this line in Bug 1505225 [1]. You may need to rebase this after autoland is merged to m-c. :)

Great, thanks for the heads up.

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it) from comment #8)

We also have continuations for blocks, when they're split within a multicol. And I think transforms do apply there, although what they do is an interesting question...

Oh, I didn't know that. I'll see if I can make a failing test case like that from bug 1406211 but for transforms and multicol then we might need to update nsIFrame::HasAnimationOfTransform accordingly.

So for my own digging into the style frame distinction, the best explanation is here:

"I think the underlying problem here is that nsTransitionManager::UpdateThrottledStylesForSubtree is assuming that the primary frame for a content node is the frame associated with that content node's styles. I think nsTableOuterFrame is the only exception to this (see, say, nsComputedDOMStyle::GetPropertyCSSValue)."

nsComputedDOMStyle::GetPropertyCSSValue is now just nsComputedDOMStyle::GetPropertyValue but the distinction mentioned is still present in nsComputedDOMStyle::UpdateCurrentStyleSources:

Another useful comment appears in an old version of GetStyleContextForFrame

  /* For tables the primary frame is the "outer frame" but the style
   * rules are applied to the "inner frame".  Luckily, the "outer
   * frame" actually inherits style from the "inner frame" so we can
   * just move one level up in the style context hierarchy....
   */

Now, assuming we want to set the NS_FRAME_MAY_BE_TRANSFORMED bit on the style frame (which makes sense since that's the same frame we'll want to query to see if the transform style is set), then let's check each of the call sites where we do that (excluding the SVG ones since they don't use table layout as far as I know):

  • KeyframeEffect::MaybeUpdateFrameForCompositor - OK, uses the style frame
  • RestyleManager::ProcessRestyledFrames #1, #2, #3 - Doesn't take care to use the style frame but probably doesn't need to since it should already be dealing with the frame that the style change applied to
  • nsFrame::Init - Again, doesn't take care to use the style frame but as far as I understand shouldn't need to
  • nsIFrame::IsCSSTransformed - This is where I believe the problem lies. We are requiring a primary frame but we are querying its style (and we are expecting to set the bit on the style frame too).

Debugging the test case from attachment 9043188 [details] [diff] [review] I can see that the style frame reports false for IsPrimaryFrame() when we query it in nsIFrame::HasAnimationOfTransform().

I guess we really want to check IsStyleFrame() (which doesn't exist, yet).

In terms of adding IsStyleFrame(), I'm not sure yet if it's legitimate to check for Type() == LayoutFrameType::Table as a way of checking if we are an inner table frame or not. Assuming it is, perhaps the check would be something like:

return Type() == LayoutFrameType::Table
  ? GetParent() && GetParent()->IsPrimaryFrame()
  : Type() != LayoutFrameType::TableWrapper && IsPrimaryFrame()

?

Or perhaps it might be better to always check for a parent table wrapper--that would be a better parallel to the logic in nsLayoutUtils::GetStyleFrame.

It turns out we want to apply the transforms to the outer frame, not the inner (see bug 816458 and bug 722777 and also nsTableFrame::IsFrameOfType).

Digging in a little further, it's not entirely clear to me how we want this to work. I think we want to treat the wrapper as transformed but the inner as not-transformed. That, at least, matches the implementation of nsTableFrame::IsFrameOfType (which returns false for eSupportsCSSTransforms).

Given that, I guess we want nsIFrame::IsTransformed to return true for the wrapper but not the inner frame. That also matches the current logic in nsIFrame::HasAnimationOfTransform() where we currently check for a primary frame.

That would suggest we want to set the NS_FRAME_MAY_BE_TRANSFORMED frame bit on the wrapper.

That matches the existing logic where we check both IsFrameOfType and NS_FRAME_MAY_BE_TRANSFORMED on the same frame in nsIFrame::IsCSSTransformed. However, it is the opposite of what we currently do in KeyframeEffect::MaybeUpdateFrameForCompositor where we set the frame bit on the style frame. (Incidentally I hope to get rid of MaybeUpdateFrameForCompositor in this bug.)

How about the mMayHaveTransformAnimation flag though? That is set on the style frame currently, but should it be?

It appears we have decided "for animation stuff we basically use the style frame" (https://hg.mozilla.org/mozilla-central/rev/30449867a680).

That seems to be the difficulty here. If we ever try to combine checks like frame->IsFrameOfType(eSupportsCSSTransforms) and nsLayoutUtils::HasAnimationOfProperty(frame, eCSSProperty_transform) (like we do in HasAnimationOfTransform) we'll run into trouble because the former will only return true for the primary frame but the latter will only return true for the style frame. (I think we only get away with this currently because aStyleDisplay->HasTransform(this) happens to return true earlier.)

We really need to decide what we expect the result of the following to be for the case where we have a separate style frame and primary frame:

  • nsIFrame::IsTransformed? style frame: false, primary frame: true ?
  • nsIFrame::IsCSSTransformed? style frame: false, primary frame: true ?
  • nsIFrame::HasAnimationOfTransform? style frame: false primary frame: true ?
  • nsLayoutUtils::HasAnimationOfProperty(frame, eCSSProperty_transform)? style frame: ?, primary frame: ?
    (Should this distinction only apply for transforms? Or for all properties?)
  • (Similarly for nsLayoutUtils::HasEffectiveAnimation)

We run into places where we mix and match these conditions frequently and we have logic like the following in RestyleManager:

    if ((aChange & nsChangeHint_UpdateTransformLayer) &&
        aFrame->IsTransformed()) {

The change hint here corresponds to the style frame, but we really want to check IsTransformed() on the primary frame instead.

Oh, and for reference, the following snippet should outline the current situation pretty well:

bool nsIFrame::IsTransformed(const nsStyleDisplay* aStyleDisplay) const {
  return IsCSSTransformed(aStyleDisplay) || IsSVGTransformed();
}

bool nsIFrame::IsCSSTransformed(const nsStyleDisplay* aStyleDisplay) const {
  MOZ_ASSERT(aStyleDisplay == StyleDisplay());
  return ((mState & NS_FRAME_MAY_BE_TRANSFORMED) &&
          (aStyleDisplay->HasTransform(this) || HasAnimationOfTransform()));
}

bool nsIFrame::HasAnimationOfTransform() const {
  return IsPrimaryFrame() &&
         nsLayoutUtils::HasAnimationOfProperty(this, eCSSProperty_transform) &&
         IsFrameOfType(eSupportsCSSTransforms);
}

Particularly, note the last bit which means we only return true from HasAnimationOfTransform() for the primary frame (both the check for IsPrimaryFrame() and the check for IsFrameOfType(eSupportsCSSTransforms) will ensure we fail for an inner table frame).

On the other hand, when we call HasAnimationOfTransform() on the outer table frame nsLayoutUtils::HasAnimationOfProperty() will return false it expects the style frame (and will fail both because we currently set the "may have transform animations" bit on the style frame and because it will get a null EffectSet in HasMatchingAnimations too).

So basically, we'll always return false from HasAnimationOfTransform for both inner and outer table frames.

Attached patch WIP patch (obsolete) — — Splinter Review

Alright, here is a patch that will get the test in question to pass. (It possibly breaks all the other tests though, we'll see).

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae9468b1391d5ea2b872999625d8b74b8b16f4f8

Initially I tried making the distinction that "the style frame returns true for animated-related questions but the primary frame doesn't".

i.e.

Outer table frame:

  • nsIFrame::IsTransformed → true
  • nsIFrame::IsCSSTransformed → true
  • nsIFrame::HasAnimationOfTransform → false
  • nsLayoutUtils::HasAnimationOfProperty(frame, eCSSProperty_transform) → false

Inner table frame:

  • nsIFrame::IsTransformed → false
  • nsIFrame::IsCSSTransformed → false
  • nsIFrame::HasAnimationOfTransform → true
  • nsLayoutUtils::HasAnimationOfProperty(frame, eCSSProperty_transform) → true

That didn't work, however, because nsIFrame::HasAnimationOfTransform is the part that is checking that we are a primary frame and that seems quite deliberate according to the changes in bug 816458.

So this patch makes us do:

Outer table frame:

  • nsIFrame::IsTransformed → true
  • nsIFrame::IsCSSTransformed → true
  • nsIFrame::HasAnimationOfTransform → true
  • nsLayoutUtils::HasAnimationOfProperty(frame, eCSSProperty_transform) → false

Inner table frame:

  • nsIFrame::IsTransformed → false
  • nsIFrame::IsCSSTransformed → false
  • nsIFrame::HasAnimationOfTransform → false
  • nsLayoutUtils::HasAnimationOfProperty(frame, eCSSProperty_transform) → true

Those last two are a bit weird. We could make them consistent but I probably need to understand how opacity works for table wrappers first to know if this is something that should apply to transform only, or more generally.

(Also, part of the reason for the odd-ness in all this is that when we call HasAnimationOfProperty, we look up the EffectSet for the passed-in frame. However, the EffectSet that actually holds the animations is the one on the style frame.)

I had a quick look into this for opacity and it seems we do the reverse there. Specifically, we typically return false for the table wrapper and true for the inner frame (and presumably end up creating the nsDisplayOpacity for the inner frame).

However, if we have an opacity animation setting the value to 1 on a display:table element, it seems we end up returning false for both wrapper and inner frame since, for the inner frame, we fail the primary frame check. We should be creating a stacking context however for that case and I can't see any stacking context reftests for display:table and opacity so it's possible we have a bug there. I'll have a better look tomorrow.

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

We should be creating a stacking context however for that case and I can't see any stacking context reftests for display:table and opacity so it's possible we have a bug there.

Looks like we do have a bug here.

So comparing the transform and opacity cases. For transforms we put the transform on the wrapper frame. That's very deliberate as per bug 722777 comment 4:

The table wrapper box is what's used as a containing block for positioned stuff if the table is used at all, but since that's not transformed it's not actually a positioned containing block.

If we fix the transform to apply to the table wrapper box, the abs/fixed thing will automagically work, I suspect...

We achieve that by making the wrapper inherit the transform from its inner frame (recall that the style context of the wrapper frame is the child of its inner frame) using this UA stylesheet rule:

But then we need to be careful to ignore the transform on the inner frame in a bunch of places (e.g. nsTableFrame::IsFrameOfType).

For opacity, however, we just should leave the opacity applying only to the inner frame. That suggests the check in nsIFrame::HasOpacityInternal should check not for a primary frame, but a "primary style frame". Something like the nsLayoutUtils::IsStyleFrame I proposed in comment 10.

e.g.

/*static*/ bool nsLayoutUtils::IsPrimaryStyleFrame(const nsFrame* aFrame) {
  if (aFrame->IsTableWrapperFrame()) {
    return false;
  }
  const nsIFrame* parent = aFrame->GetParent();
  if (parent && parent->IsTableWrapperFrame()) {
    return parent->PrincipalChildList().FirstChild() == aFrame;
  }
  return aFrame->IsPrimaryFrame();
}
Summary: transform: none animations on display: table elements do not create a stacking context → transform: none and opacity: 1 animations on display: table elements do not create a stacking context

For display:table content we generate two frames: a table wrapper frame and an
inner table frame. The styles are applied to the inner frame (referred to as the
style frame), whilst the wrapper frame is the primary frame for the content.

However, in order to make tables with transforms behave as a container for
abspos/fixed-pos content as required by the spec, we apply the transform to the
wrapper frame (bug 722777) by inheriting the transform from inner to wrapper and
then ignoring the transform on the inner frame (bug 722777 and bug 816458).

When handling animations on table elements we need to be careful of this
distinction. in particular, css animations[1] and web animations[2] require that
when we have an unfinished transform animation targetting an element, the
element acts as if it had will-change: transform applied and therefore
generates a stacking context. As a result we need to accurately detect when
a frame should be considered as having transform animations applied to it or not
for the purpose of creating a stacking context.

Previously our handling of display:table content was quite inconsistent and
contradictory. For example, nsIFrame::HasAnimationOfTransform would check for
a primary frame AND for animations on that frame, despite the fact that we only
ever store animations on the style frame. As a result it could never return true
for either a table wrapper or inner table frame.

This patch attempts to make this handling a least a little more consistent,
producing the following result:

Outer table frame (primary frame):

nsIFrame::IsTransformed → true
nsIFrame::IsCSSTransformed → true
nsIFrame::HasAnimationOfTransform → true
nsLayoutUtils::HasAnimationOfProperty(frame, eCSSProperty_transform) → false

Inner table frame (style frame):

nsIFrame::IsTransformed → false
nsIFrame::IsCSSTransformed → false
nsIFrame::HasAnimationOfTransform → false
nsLayoutUtils::HasAnimationOfProperty(frame, eCSSProperty_transform) → true

We maintain that the NS_FRAME_MAY_BE_TRANSFORMED bit is only set on the primary
frame whilst the mMayHaveTransformAnimation flag is only set on the style frame.

Note that we don't simply always put everything on the primary frame because for
other property types (e.g. opacity) the default setup of putting all styles and
animations on the style frame is simpler and correct. So far it is only
transforms that require special handling to apply the effect to the wrapper
frame.

This patch adds a reftest that fails without the code changes included in this
patch.

[1] https://drafts.csswg.org/css-animations/#animations
[2] https://drafts.csswg.org/web-animations-1/#side-effects-section

As with the previous patch in this series, we need to pay particular attention
to how we handle display:table content when detecting animations on a element.
Please see the extended description in that patch for an explanation of
different frame types involved.

As with transforms, our handling of opacity is also inconsistent. In
particular, we fail to return true from nsIFrame::HasOpacityInternal for
display:table content with opacity animations applied due to the conflicting
requirements for a primary frame and having opacity animations (which are stored
on the style frame).

Unlike transforms, however, we do not inherit the opacity to the table wrapper.
Instead we leave it on the inner table frame. As a result, we should not check
for a primary frame, but instead we should check for the style frame for the
primary frame.

This patch adjusts this handling to check instead for the appropriate style
frame as opposed to requiring a primary frame. It includes a reftest that fails
without the code changes in this patch.

Depends on D21883

Since bug 1524480 we set the NS_FRAME_MAY_BE_TRANSFORMED frame bit when needed
in RestyleManager::ProcessRestyledFrames so that it is now redundant to also set
it from KeyframeEffect.

Furthermore, setting frame bits from KeyframeEffect is a little fragile since it
depends on the life cycle of the KeyframeEffect which is independent of the
nsFrame. If we can avoid doing that, we probably should.

Depends on D21884

Attachment #9045827 - Attachment is obsolete: true
Attachment #9044059 - Attachment is obsolete: true
Pushed by bbirtles@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5cf9e494f690 Be more consistent about only applying transforms to primary frames; r=hiro https://hg.mozilla.org/integration/autoland/rev/211f6573535b Fix handling of animation of opacity on display:table; r=hiro https://hg.mozilla.org/integration/autoland/rev/a76b23d70349 Drop KeyframeEffect::MaybeUpdateFrameForCompositor; r=hiro
Regressions: 1567108
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: