Closed Bug 1318697 Opened 8 years ago Closed 8 years ago

Elements disappear after sliding in

Categories

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

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 + fixed
firefox-esr52 52+ fixed
firefox53 + fixed

People

(Reporter: freaktechnik, Assigned: hiro)

References

(
URL
)

Details

(Keywords: dev-doc-complete, regression, site-compat)

Attachments

(4 files, 1 obsolete file)

A fix
717 bytes, patch
Details | Diff | Splinter Review
A reduced test case
535 bytes, text/html
Details
Another case without important rule changes
502 bytes, text/html
Details
Bug 1318697 - Part 1: Skip composing styles for properties depending on cascade level.
58 bytes, text/x-review-board-request
birtles
: review+
jcristau
: approval-mozilla-release+
jcristau
: approval-mozilla-esr52+
Details 
On http://www.sigareal.ch/de/Angebote.6.html when scrolling on Nightly 53 the parts that get slided-in disappear after reaching 100% opacity until they're moused over.

This does not happen in Firefox 50 without e10s due to add-ons, the panels get displayed as expected.

I've not been able to identify which part of the animation is the problem, but unsetting the "opacity: 1" and then re-enabling it in the style editor will fix the display of all slid-in elements.
(moving to correct component for diagnosis)
Component: General → Desktop
Product: Web Compatibility → Tech Evangelism
Whiteboard: [needsdiagnosis]
Better STR:

1) Load http://www.sigareal.ch/de/Angebote.6.html 
2) Scroll to bottom of page

Expected: elements slide into view and are visible
Actual: by the time you get to the bottom, content is blank

Hiro, can you take a look to see if this is fall-out from Bug 1304922?
Blocks: 1304922
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected
status-firefox53: --- → affected
tracking-firefox53: --- → ?
Component: Desktop → Layout
Flags: needinfo?(hiikezoe)
Keywords: regression
Product: Tech Evangelism → Core
Attached patch A fix — — Splinter Review 
Thank you, Mike, for narrowing the cause down.  This is actually caused by this change <https://hg.mozilla.org/mozilla-central/rev/01bdca9df2c7>.

This is caused by unnecessary restyle request for Animations level if there are only transitions.  I am surprised that unnecessary restyle request causes this kind of issue.  I guess there is a bug revealed by the change. Or there is something I am totally missing.

FWIW, attaching patch fixes this issue.
Assignee: nobody → hiikezoe
Flags: needinfo?(hiikezoe)
Component: Layout → DOM: Animation
Attached file A reduced test case — 
There are opacity and transform transitions.  

opacity: 0.5 -> 1
transfrom: translateY(300px) -> translateY(0px) !important

With these transitions, we send a request restyle for Animations level initially, and the result of the request (i.e. an  AnimValuesStyleRule) is painted when these transitions finish.  I don't know why yet.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bd288296b82fb136b5e067bc2d29231336438ae

I am still not sure why the unnecessary AnimValuesStyleRule for Animations level is used when the transition finish, but anyway the call stack is:

 mozilla::AnimValuesStyleRule::MapRuleInfoInto()
 nsRuleNode::WalkRuleTree()
 nsRuleNode::GetStyleEffects()
 nsStyleContext::DoGetStyleEffects()
 nsStyleContext::StyleEffects()
 nsStyleContext::CalcStyleDifferenceInternal()
 nsStyleContext::CalcStyleDifference()
 mozilla::ElementRestyler::CaptureChange()
 mozilla::ElementRestyler::RestyleSelf()
 mozilla::ElementRestyler::Restyle()
 mozilla::ElementRestyler::ComputeStyleChangeFor()
 mozilla::RestyleManager::ComputeAndProcessStyleChange()
 mozilla::RestyleManager::RestyleElement()

Before the culprit change <https://hg.mozilla.org/mozilla-central/rev/01bdca9df2c7>, we also sent the unnecessary request but it was filtered out in EffectCompositor::ComposeAnimationRule [1].

[1] https://hg.mozilla.org/mozilla-central/rev/01bdca9df2c7#l3.36

We shouldn't, anyway, call RequestRestyle(Layer) for Animations level if there is no animations.
Priority: -- → P1
Now I understand what's going on there. 

1) Start two transitions, opacity: 0.5 -> 1; transform: translateY(300px) -> translateY(0px)!important;
2) RequestRestyle(Layer, Animations) and RequestRestyle(Layer, Transitions)
3) ComposeAnimationRule() for both of cascade levels
  For opacity:  AnimationRule[Animations] = 0.5, AnimationRule[Transitions] = 0.5
4) On initial paint of the transition, AnimationRule[Animations] style is used because animations level is prior to transitions level but we never notice it.
5) While the transitions are running, we don't call RequestRestyle() because both of opacity and transform run on the compositor.
6) When the transitions finish, we call RequestRestyle(Layer, Transitions)[1]
7) The AnimationRule[Animations] is still alive because it's animations level
8) As a result of 6), nsStyleSet::RuleNodeWithReplacement is invoked with eRestyle_CSSTransitions
9) In the RuleNodeWithReplacement, the AnimationRule[Animations] is copied to a new RuleNode[2], on the other hand at this point, a new AnimationRule[Transitions] is null, the new RuleNode has no animation rule for transitions.

[1] https://hg.mozilla.org/mozilla-central/file/0534254e9a40/dom/animation/Animation.cpp#l1233
[2] https://hg.mozilla.org/mozilla-central/file/0534254e9a40/layout/style/nsStyleSet.cpp#l1652
[3] https://hg.mozilla.org/mozilla-central/file/0534254e9a40/layout/style/nsStyleSet.cpp#l1585
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> 3) ComposeAnimationRule() for both of cascade levels
>   For opacity:  AnimationRule[Animations] = 0.5, AnimationRule[Transitions]
> = 0.5

This is the part I don't understand. Why do we make an animations rule for opacity? Do we just always do that because we know transitions will override it?
(In reply to Brian Birtles (:birtles) from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> > 3) ComposeAnimationRule() for both of cascade levels
> >   For opacity:  AnimationRule[Animations] = 0.5, AnimationRule[Transitions]
> > = 0.5
> 
> This is the part I don't understand. Why do we make an animations rule for
> opacity? Do we just always do that because we know transitions will override
> it?

Regarding the case of ComposeAnimationRule with CascadeLevel::Animations, we need to know transitions level's style for additive or accumulative animations.  We can add an assertion that there should be at least one animations' level effect in case of aCascadeLevel == Animations.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> (In reply to Brian Birtles (:birtles) from comment #8)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> > > 3) ComposeAnimationRule() for both of cascade levels
> > >   For opacity:  AnimationRule[Animations] = 0.5, AnimationRule[Transitions]
> > > = 0.5
> > 
> > This is the part I don't understand. Why do we make an animations rule for
> > opacity? Do we just always do that because we know transitions will override
> > it?
> 
> Regarding the case of ComposeAnimationRule with CascadeLevel::Animations, we
> need to know transitions level's style for additive or accumulative
> animations.  We can add an assertion that there should be at least one
> animations' level effect in case of aCascadeLevel == Animations.

Oh, I think I understand.

If we only have a transition then normally we'd want it to look like this:

 CSS transition: opacity 0 -> 1

 @ 50%:

 transitions level animation-rule: opacity: 0.5
 animations level animation-rule: <nothing>

If we have an animation too, then, when producing the animation-level rule, the animation wins over the cascade, and when producing the transition-level rule, we skip the transition so we have:

 CSS transition: opacity 0 -> 0.5
 Other animation: opacity 0.5 -> 1.0

 @ 50%:

 transitions level animation-rule: <nothing>
 animations level animation-rule: 0.75

But, if we have an additive animation the animation result might end up in the animations level of the cascade:

 CSS transition: opacity 0 -> 0.5
 Other animation: opacity 0.5 -> 1.0 (additive)

 @ 50%:

 transitions level animation-rule: <nothing>
 animations level animation-rule: 1.0

So, for this last case, we need the transition animation value to build on, but for the first case we don't want it to be added to the rule for the animations level of the cascade.

The solution you proposed makes sense to me provided we do it on a per-property basis. i.e. we should only add properties from transitions to the animations-level rule if we know they are going to be overridden or added to by animations at the animation-level.
Thanks for the summarizing.  Your comment noticed me that there is another case that we need to filter effects in KeyframeReadOnly::ComposeStyle(), i.e. compose per properties.
To fix the case in comment 11, I think we need to pass CascadeLevel to ComposeStyle(), and in case of Animations Level, need to pass PropertiesForAnimationsLevel() as well and then compose styles if the composing property is included in PropertiesForAnimationsLevel() in contrast to transition level composing.
Tracking 53+ since this is a new regression in DOM animation.
tracking-firefox53: ? → +
Whiteboard: [needsdiagnosis]
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> Created attachment 8813107 [details]
> Another case without important rule changes

I should note this case in more detail.

There is a 0.3s opacity animation and 1.0s transform transitions. When the opacity animation finishes:

1) We call RequestRestyle(Layer, CascadeLevel::Animations).
2) As a result of 1), we call EffectCompositorAnimationRule(CascadeLevel::Animations).
3) At the moment of 2), the effect of the opacity animation has been already removed.
4) We call KeyframeReadOnly::ComposeStyle() against only the effect of the transform transition
5) As a result of 4), a transformed style at this moment is added in an AnimValueStyleRule of *Animations* level.
6) After 5), we never call RequestRestyle(CascadeLevel::Animations) so the AnimValueStyleRule generated in 5) is cached in nsRuleNode and is shown up after the transition finished.
Each reftest in each commit fails without corresponding fix.

Here is a try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ac88da6d0dce07872f4783d3f301ef9a440ede4

Both reftests are in 'R4' on Linux64 opt and 'R21' on Android debug.  Unfortunately a very frequent orange (bug 1318953) is also in the 'R4', but these reftests did not fail there.
Comment on attachment 8813955 [details]
Bug 1318697 - Part 1: Skip composing styles for properties depending on cascade level.

https://reviewboard.mozilla.org/r/95258/#review96262

::: dom/animation/EffectCompositor.cpp:619
(Diff revision 1)
> -  // priority except properties in propertiesToSkip.
> -  const nsCSSPropertyIDSet& propertiesToSkip =
> -    aCascadeLevel == CascadeLevel::Animations
> -    ? nsCSSPropertyIDSet()
> -    : effects->PropertiesForAnimationsLevel();
> +  // priority.
> +  // In case composing Transitions level, properties in
> +  // propertiesForAnimationsLevel will be skiped to compose, on the other hand
> +  // in case of Animations level, properties _not_ in
> +  // propertiesForAnimationsLevel will be skipped.
> +  const nsCSSPropertyIDSet& propertiesForAnimationsLevel =
> +    effects->PropertiesForAnimationsLevel();
>    for (KeyframeEffectReadOnly* effect : sortedEffectList) {
> -    effect->GetAnimation()->ComposeStyle(animationRule, propertiesToSkip);
> +    effect->GetAnimation()->ComposeStyle(animationRule,
> +                                         propertiesForAnimationsLevel,
> +                                         aCascadeLevel);

Rather than push all this logic down to KeyframeEffectReadOnly, it seems like it would be a simpler API (and smaller patch!) if we just set up an appropriate nsCSSPropertyIDSet here?

From what I understand we currently have the following arrangement:

                      Property in animation level?
                          No             Yes
  ------------------+----------------+----------------
  Transitions Level |  Add property  | Skip property
  Animations Level  |  Add property  | Add property

And we want to change the behavior to:

                      Property in animation level?
                          No             Yes
  ------------------+----------------+----------------
  Transitions Level |  Add property  | Skip property
  Animations Level  |  Skip property | Add property

Can't we just invert the property set when passing we have the animations level?

We'll need to add a method to nsCSSPropertyIDSet but it should be simple: just doing a bitwise flip of mProperties.
Attachment #8813955 - Flags: review?(bbirtles)
Comment on attachment 8813955 [details]
Bug 1318697 - Part 1: Skip composing styles for properties depending on cascade level.

https://reviewboard.mozilla.org/r/95258/#review96262

> Rather than push all this logic down to KeyframeEffectReadOnly, it seems like it would be a simpler API (and smaller patch!) if we just set up an appropriate nsCSSPropertyIDSet here?
> 
> From what I understand we currently have the following arrangement:
> 
>                       Property in animation level?
>                           No             Yes
>   ------------------+----------------+----------------
>   Transitions Level |  Add property  | Skip property
>   Animations Level  |  Add property  | Add property
> 
> And we want to change the behavior to:
> 
>                       Property in animation level?
>                           No             Yes
>   ------------------+----------------+----------------
>   Transitions Level |  Add property  | Skip property
>   Animations Level  |  Skip property | Add property
> 
> Can't we just invert the property set when passing we have the animations level?
> 
> We'll need to add a method to nsCSSPropertyIDSet but it should be simple: just doing a bitwise flip of mProperties.

Actually I thought the same thing a little, but I did not do it because the inverted nsCSSPropertyIDSet has most of properties in many cases, it was little bit misleading to me (if someone looks into it, etc.).  But yeah, I am with the inveted set.
Comment on attachment 8813955 [details]
Bug 1318697 - Part 1: Skip composing styles for properties depending on cascade level.

https://reviewboard.mozilla.org/r/95258/#review96314

I tried to rewrite the commit message to make it more clear (to me at least!). 
Does this make sense:

"When we are composing style for the Animations level of the cascade,
if we have transitions-level animations, we *also* need to compose them if we
have animations-level animations that build on top of them using additive or
accumulative composite modes.

However, we should not build those transitions-level animations unless they will
be built on or overridden by a regular animations-level animation. Otherwise
we will end up with transitions-level animations in the animations-level and
while transitions-level will override the animations-level in the cascade,
once the transition finishes there will be nothing to remove the cached
animations-level animation rule."

?

r=me with the following addressed.

Thanks for doing this!

::: layout/reftests/css-transitions/transition-and-animation-with-different-durations.html:24
(Diff revision 2)
> +  target.style = "transition: opacity 0.3s; opacity: 1; " +
> +                 "animation: translate 0.1s;";

300ms is a bit long for a test. Does it still reproduce the problem reliable with a shorter duration? I guess not? Perhaps 200ms with an even shorter animation?

::: layout/style/nsCSSPropertyIDSet.h:70
(Diff revision 2)
> +    // Return new nsCSSPropertyIDSet which is the invert of this
> +    // nsCSSPropertyIDSet.

"Return a new nsCSSPropertyIDSet which is the inverse of this set."

::: layout/style/nsCSSPropertyIDSet.h:73
(Diff revision 2)
> +      nsCSSPropertyIDSet result;
> +      for (size_t i = 0; i < mozilla::ArrayLength(mProperties); ++i) {
> +        result.mProperties[i] = ~mProperties[i];
> +      }
> +      return result;

(I was wondering if here was anything in PodOperations we could use for this but it seems like there's not. Also, by my calculation, there are only 10 elements in this array so it's not a big deal.)
Attachment #8813955 - Flags: review?(bbirtles) → review+
Comment on attachment 8813954 [details]
Bug 1318697 - Part 1: Don't request restyle for animations level if there is no animations' level effect.

https://reviewboard.mozilla.org/r/95256/#review96318

Does the test here still fail if we apply just part 2 and not this part (only its test)?

I'm a little bit hesitant about landing this--at least at the same time. It seems like part 2 is the fix we really need so I'd like to land that first and then add this later as a (possibly slightly risky) optimization.

::: dom/animation/EffectCompositor.cpp:765
(Diff revision 1)
>    if (prevCompositorPropertiesWithImportantRules !=
> -        compositorPropertiesInSet(propertiesWithImportantRules)) {
> +        compositorPropertiesInSet(propertiesWithImportantRules) &&
> +      !compositorPropertiesInSet(propertiesForAnimationsLevel).none()) {

I think compositorPropertiesInSet(propertiesForAnimationsLevel).any() would be more clear than a double-negative?
Comment on attachment 8813954 [details]
Bug 1318697 - Part 1: Don't request restyle for animations level if there is no animations' level effect.

https://reviewboard.mozilla.org/r/95256/#review96318

No.  The part 1 does just stop a unnecessary RequestRestyle(Animations).  I will combine the test case in part 1 into part 2.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> Comment on attachment 8813954 [details]
> Bug 1318697 - Part 1: Don't request restyle for animations level if there is
> no animations' level effect.
> 
> https://reviewboard.mozilla.org/r/95256/#review96318
> 
> No.  The part 1 does just stop a unnecessary RequestRestyle(Animations).  I
> will combine the test case in part 1 into part 2.

And rename part 2 as part 1.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> No.  The part 1 does just stop a unnecessary RequestRestyle(Animations).  I
> will combine the test case in part 1 into part 2.

Do we need the test in part 1? Is it likely we'll change something that regressesthe test from part 1 but still passes the test in part 2?
(In reply to Brian Birtles (:birtles) from comment #26)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> > No.  The part 1 does just stop a unnecessary RequestRestyle(Animations).  I
> > will combine the test case in part 1 into part 2.
> 
> Do we need the test in part 1? Is it likely we'll change something that
> regressesthe test from part 1 but still passes the test in part 2?

Ah, OK.  So far, the test is not necessary because we have no way to check RequestRestyle() itself for now.
Comment on attachment 8813954 [details]
Bug 1318697 - Part 1: Don't request restyle for animations level if there is no animations' level effect.

https://reviewboard.mozilla.org/r/95256/#review96334

r=me with the change from comment 23 and without the test

Also, if possible, I'd like to land this separately to make regression tracking easier in case this has unexpected side-effects.
Attachment #8813954 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #22)
> Comment on attachment 8813955 [details]
> Bug 1318697 - Part 2: Skip composing styles for properties depending on
> cascade level.
> 
> https://reviewboard.mozilla.org/r/95258/#review96314
> 
> I tried to rewrite the commit message to make it more clear (to me at
> least!). 
> Does this make sense:
> 
> "When we are composing style for the Animations level of the cascade,
> if we have transitions-level animations, we *also* need to compose them if we
> have animations-level animations that build on top of them using additive or
> accumulative composite modes.
> 
> However, we should not build those transitions-level animations unless they
> will
> be built on or overridden by a regular animations-level animation. Otherwise
> we will end up with transitions-level animations in the animations-level and
> while transitions-level will override the animations-level in the cascade,
> once the transition finishes there will be nothing to remove the cached
> animations-level animation rule."

I think this sentence is correct (but honestly hard to read).  I know, and I understand the root cause of this bug is not easy to understand immediately, animations, animations-level, transtions, transitions level.. 

Thank you always for corrections of Engrish!

> :::
> layout/reftests/css-transitions/transition-and-animation-with-different-
> durations.html:24
> (Diff revision 2)
> > +  target.style = "transition: opacity 0.3s; opacity: 1; " +
> > +                 "animation: translate 0.1s;";
> 
> 300ms is a bit long for a test. Does it still reproduce the problem reliable
> with a shorter duration? I guess not? Perhaps 200ms with an even shorter
> animation?

Actually I tried 200ms locally, if I remember correctly, it sometimes passed *without* fix. But it will not be a problem.

> ::: layout/style/nsCSSPropertyIDSet.h:73
> (Diff revision 2)
> > +      nsCSSPropertyIDSet result;
> > +      for (size_t i = 0; i < mozilla::ArrayLength(mProperties); ++i) {
> > +        result.mProperties[i] = ~mProperties[i];
> > +      }
> > +      return result;
> 
> (I was wondering if here was anything in PodOperations we could use for this
> but it seems like there's not. Also, by my calculation, there are only 10
> elements in this array so it's not a big deal.)

Thanks, I did not know the PodOperations, it can be used somewhere.
Attachment #8813954 - Attachment is obsolete: true
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/autoland/rev/940a214b71f0
Part 1: Skip composing styles for properties depending on cascade level. r=birtles
https://hg.mozilla.org/mozilla-central/rev/940a214b71f0
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Version: unspecified → Trunk
QA Whiteboard: [good first verify]
Are you aware that this issue introduced regression in FF52?
I think this fix should be pushed with next update to v52 as it is ESR release.

You can see that it is already affecting people:
https://bugzilla.mozilla.org/show_bug.cgi?id=1318697
https://support.mozilla.org/t5/Firefox/CSS-and-or-script-animations-handling-broken-with-latest-52-0/td-p/1373309
Based on comment 4 this is a regression from bug 1304922 which landed in 52. It seems the status flags were set incorrectly meaning we shipped this regression to release :(.

Hiro, can you request uplift for this to ESR52? Apparently we're treating ESR 52 differently and are more accepting of patches so it's worth asking.
status-firefox52: unaffected → affected
Flags: needinfo?(hikezoe)
Comment on attachment 8813955 [details]
Bug 1318697 - Part 1: Skip composing styles for properties depending on cascade level.

I did confirm this patch can be applied to esr52.
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Users will see garbage paint of CSS transitions.
Fix Landed on Version: Firefox 53
Risk to taking this patch (and alternatives if risky): very low. The patch has been landed for about four months, it's been already beta as well.
String or UUID changes made by this patch:  None
Flags: needinfo?(hikezoe)
Attachment #8813955 - Flags: approval-mozilla-esr52?
[Tracking Requested - why for this release]: I've got to this bug while investigating the following issue reported today. I could confirm the fix using mozregression. It's an annoying issue for users, so it would be nice to get it fixed with Firefox 52.0.2 if any.

https://mail.mozilla.org/pipermail/firefox-dev/2017-March/005180.html
tracking-firefox52: --- → ?
Keywords: dev-doc-needed, site-compat
I think we need a little post mortem on this bug to figure out how it did NOT make it into Firefox 52. This was a regression that was found and fixed in plenty of time to make Firefox 52.

Do we have a process in place to make sure that regressions go forward to as many releases as possible?
(In reply to Mike Kaply [:mkaply] from comment #41)
> I think we need a little post mortem on this bug to figure out how it did
> NOT make it into Firefox 52. This was a regression that was found and fixed
> in plenty of time to make Firefox 52.
> 
> Do we have a process in place to make sure that regressions go forward to as
> many releases as possible?

The problem was that the status flags were set incorrectly back in comment 3. Everyone just trusted they were correct without double-checking which version the regressing bug landed in, so it didn't occur to anyone that this needed uplift. When the regressing bug was confirmed in comment 4 we should have checked the status flags were correct.
That was my fault, it seems. Sorry. :(
Hi Julien, should we consider including this one in the next 52 dot release? Thanks!
Flags: needinfo?(jcristau)
<opinion>
This should go in a 52 dot release if it happens and especially in the 52 esr.

This was a "break websites" kind of regression.
</opinion>
Comment on attachment 8813955 [details]
Bug 1318697 - Part 1: Skip composing styles for properties depending on cascade level.

let's take this animations regression fix for 52.0.2

uplift to release (default branch), esr52 (default and FIREFOX_ESR_52_0_X_RELBRANCH)
Flags: needinfo?(jcristau)
Attachment #8813955 - Flags: approval-mozilla-release+
Attachment #8813955 - Flags: approval-mozilla-esr52?
Attachment #8813955 - Flags: approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: