Closed
Bug 962594
Opened 10 years ago
Closed 9 years ago
[Power] Invisible CSS animations (on 'display:none' elements) activate the refresh driver
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
People
(Reporter: rvitillo, Assigned: kats)
References
(Blocks 1 open bug, )
Details
(Keywords: perf, power)
Attachments
(4 files, 3 obsolete files)
832 bytes,
text/html
|
Details | |
1.01 KB,
text/html
|
Details | |
6.58 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
5.26 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
Firefox is consuming an excessive amount of CPU resources while idling on Facebook compared to other browsers (see URL). The following trace seems to be the culprit: mozilla::RefreshDriverTimer::Tick() nsRefreshDriver::Tick(long long, mozilla::TimeStamp) PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) mozilla::RestyleManager::ProcessPendingRestyles() mozilla::RestyleTracker::DoProcessRestyles() mozilla::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element*, nsRestyleHint, nsChangeHint) mozilla::RestyleManager::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, bool) nsCSSFrameConstructor::MaybeRecreateFramesForElement(mozilla::dom::Element*) nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*) nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*, TreeMatchContext&) nsStyleSet::FileRules(bool (*)(nsIStyleRuleProcessor*, void*), RuleProcessorData*, mozilla::dom::Element*, nsRuleWalker*) bool EnumRulesMatching<ElementRuleProcessorData>(nsIStyleRuleProcessor*, void*) nsCSSRuleProcessor::RulesMatching(ElementRuleProcessorData*) RuleHash::EnumerateAllRules(mozilla::dom::Element*, ElementDependentRuleProcessorData*, NodeMatchContext&) ContentEnumFunc(RuleValue const&, nsCSSSelector*, ElementDependentRuleProcessorData*, NodeMatchContext&, AncestorFilter*) mozilla::RefreshDriverTimer::Tick is invoked about 60 times per second even though the page "appears" to be static (chat disabled).
Comment 1•10 years ago
|
||
with any addons? FWIW, there are other bugs citing RestyleManager
Flags: needinfo?(rvitillo)
Reporter | ||
Updated•10 years ago
|
OS: Mac OS X → All
Comment 3•10 years ago
|
||
We should break in the code that schedule the timer. This should let us know what mutation is causing this. The refresh driver is schedule when something when a new frame is (potentially) required. Sometimes we know ahead of time that we're going to produce something identical and stop. Understanding what is causing is refresh driver to trigger in the first place is a good first step.
Reporter | ||
Comment 4•10 years ago
|
||
The culprit for the RefreshDriverTimer::Tick seems to be the css class _55ym: <html> <head> <style> ._55ym{animation:rotateSpinner 1.2s linear infinite} @keyframes rotateSpinner{0%{transform:rotate(0deg)} 100%{transform:rotate(360deg)}} </style> </head> <body> <span class="_55ym"> </span> </body> </html> The _55ym class is used to animate the spinning wheels on the left toolbar of Facebook. The wheels are visible only when the user clicks on one of the menu items and the content is being loaded though.
Comment 5•10 years ago
|
||
Is the refresh driver ticking even when the animation is trivially hidden? If so that's a bug and should be fix. We can compute if something is not visible and should be able to keep the refresh driver off. Nice find.
Reporter | ||
Comment 6•10 years ago
|
||
Right, there is no visible animation but refresh driver gets scheduled anyway.
Comment 7•10 years ago
|
||
See the animation in Comment 4. Can we get someone from Layout to look at optimizing how CSS animations trigger the refresh driver? We should avoid scheduling the timer for no change animations.
Flags: needinfo?(roc)
Summary: High power consumption on Facebook while idling → [Power] Invisible CSS animations activate the refresh driver
Reporter | ||
Comment 8•10 years ago
|
||
Note that the refresh driver ticks even when the animation is trivially hidden, e.g.: <html> <head> <style> ._5tqs{display: none;} ._55ym{animation:rotateSpinner 1.2s linear infinite} @keyframes rotateSpinner{0%{transform:rotate(0deg)} 100%{transform:rotate(360deg)}} </style> </head> <body> <div class="_55ym _5tqs">foo</div> </body> </html> There are two possible optimizations here: 1) Don't tick when the animation is not visible 2) Don't tick when a visible animation is not going to produce a delta in the frame buffer (e.g. Comment 4) For this particular bug addressing 1) should fix the issue.
Updated•10 years ago
|
Component: General → CSS Parsing and Computation
Product: Firefox → Core
Hardware: x86 → All
Comment 9•10 years ago
|
||
> 2) Don't tick when a visible animation is not going to produce a delta in the frame buffer
Animations can affect layout even without affecting what's painted, and that's observable from script. So optimizing out animations that are not on display:none stuff without breaking compat is pretty tough.
Comment 10•10 years ago
|
||
Yeah, I suspect we want to go with something along the lines of (1) -- something like: "Don't tell the refresh driver about animations for an element IF that element's (un-animated) computed 'display' is none, and also there are currently no animations on its 'display' value." This might want to happen in nsAnimationManager::GetElementAnimations() (e.g. maybe don't call AddElementData there), or in a related function. We also probably need to also worry about SMIL animations, which could be another way of actually-showing something with un-animated "display:none"... Maybe as a fast pass, we could check if there are any SMIL animations targeting the element at all.
Comment 11•10 years ago
|
||
s/fast pass/quick and easy hack/
We could probably do something for animations in a display:none subtree, involving keeping the animations in the animation manager but not updating them -- especially if we can depend on the element not having a persistent style context.
Keywords: power
Comment 13•10 years ago
|
||
One other note. The testcase in comment 8 in fact has an observable effect: if you query the computed "transform" of the object, it should change over time as the animation progresses. So either that needs to tick the refresh driver to update the style or we need to somehow update it when getComputedStyle happens or something...
Comment 14•10 years ago
|
||
Here's a live testcase, based on comment 8. It's got a (short) delay before the animation is applied, so you can watch (in a debugger) when we register as a refresh observer & start ticking.
Comment 15•10 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #7) > See the animation in Comment 4. Can we get someone from Layout to look at > optimizing how CSS animations trigger the refresh driver? Tagging dbaron instead of roc [to pick an assignee], since I believe style-system / animations are more his area of expertise. (This came up in the Platform meeting today, too, as something that Power folks are looking for help on.) [I'd be open to taking this myself, though I'm unfamiliar with nsAnimationManager.cpp, so I'd be digging a bit.]
Flags: needinfo?(roc) → needinfo?(dbaron)
Comment 16•10 years ago
|
||
Roberto you know that we do worse on power usage on this page. But do you know if other browsers optimize away the timer or are they just more efficient at returning early from their refresh timer? If it's the later then perhaps the problem is easier to optimize. We might just have to get better at returning faster.
Flags: needinfo?(rvitillo)
Comment 17•10 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #16) > If it's the later then perhaps the problem is easier to optimize. We might > just have to get better at returning faster. I don't think that'll do it; if I recall correctly from other bugs, it's the frequent wakeups that are responsible for the power usage in cases like this (even if they're short). They never let the CPU get into a lower power state, or something like that.
Comment 18•10 years ago
|
||
I don't think so either. But it's probably worth checking this assumption before we take on a potentially difficult bug. Unless this bug is easy, then we should do just do it.
In the Facebook case, is the offending animation actually in a display:none subtree?
Comment 20•10 years ago
|
||
See comment 8. Not a subtree in that case but it has both the animation and display none.
Comment 21•10 years ago
|
||
Robert's question was whether comment 8 is actually an accurate representation of what's going on with the original page this bug was filed on or whether it's a synthetic testcase.
Reporter | ||
Comment 22•10 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #16) > Roberto you know that we do worse on power usage on this page. But do you > know if other browsers optimize away the timer or are they just more > efficient at returning early from their refresh timer? > > If it's the later then perhaps the problem is easier to optimize. We might > just have to get better at returning faster. Chrome optimizes away the timer. Returning early wouldn't really help though in terms of power usage (see Daniel's comment).
Flags: needinfo?(rvitillo)
Reporter | ||
Comment 23•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #21) > Robert's question was whether comment 8 is actually an accurate > representation of what's going on with the original page this bug was filed > on or whether it's a synthetic testcase. Comment 8 is an accurate representation of what's causing the issue with Facebook.
Comment 24•10 years ago
|
||
It looks like they simply don't apply animations at all to display:none elements. It's not clear to me that this is actually spec-compliant... David?
Comment 25•10 years ago
|
||
Per conversation with bz in IRC, we have (at least) two options here: (a) Change the spec (and our implementation) to match the Chrome behavior, so that animations on properties in a 'display:none' subtree have no effect (except perhaps for animations targeting the 'display' property itself). (b) Optimize our current behavior by lazily computing animations on elements inside of a 'display:none' subtree (again, probably excluding the 'display' property itself). Store such animations in a separate list on the animation manager, without letting them prompt the refresh driver to tick. (but if the refresh driver is already ticking for other reasons, we might as well sample them with everything else). And then -- whenever getComputedStyle() is called, flush these animations. (That's the lazy part.) (and perhaps skip the flush if we had a refresh driver tick or a flush "recently".)
(b) sounds good.
Comment 27•10 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #25) > (but if the refresh driver is already ticking for other reasons, we might as > well sample them with everything else). Sorry to nitpick but ideally i'd be best to behave the same way regardless and not sample them at all. This way if we think an animation has no impact then it stop regardless. This make bugs where we incorrectly disable an animation obvious. Otherwise you get a bug that only reproduces if we're not painting.
Comment 28•10 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #27) > (In reply to Daniel Holbert [:dholbert] from comment #25) > > (but if the refresh driver is already ticking for other reasons, we might as > > well sample them with everything else). > > Sorry to nitpick but ideally i'd be best to behave the same way regardless > and not sample them at all. Sure, that would work. The reason I'm suggesting sampling them during not-requested-by-us ticks is (a) it should be low-cost -- as long as we're already taking the hit of ticking the refresh driver (for something else), the marginal cost of sampling these animations is minimal (b) it will let us keep getComputedStyle() more efficient (no need to worry about flushing these animations there, if we've had a recent tick(). > Otherwise you get a bug that only reproduces if we're not > painting. I don't understand what the hypothetical bug you're talking about would look like. These animations shouldn't have any observable effect, since they're by definition in content that doesn't render. (except via getComputedStyle, which will only have a slight observable difference -- whether it reports the animation as of the last tick (if we have other animations running; note that this is the current behavior) or whether it reports the up-to-the-millisecond animation state (if no other animations are running & hence no ticks are happening).
Updated•10 years ago
|
Whiteboard: [possible solution in comment 25 part b]
Updated•10 years ago
|
Summary: [Power] Invisible CSS animations activate the refresh driver → [Power] Invisible CSS animations (in 'displya:none' subtree) activate the refresh driver
Updated•10 years ago
|
Summary: [Power] Invisible CSS animations (in 'displya:none' subtree) activate the refresh driver → [Power] Invisible CSS animations (in 'display:none' subtree) activate the refresh driver
wouldn't mind doing this after 960465, although I'm not sure how strong the ordering constraints actually are
Depends on: 960465
Comment 30•10 years ago
|
||
So here's an interesting question. http://dev.w3.org/csswg/css-animations/ seems to pretty explicitly say that animations don't run on display:none elements. David, what _is_ the spec status here?
Updated•10 years ago
|
Summary: [Power] Invisible CSS animations (in 'display:none' subtree) activate the refresh driver → [Power] Invisible CSS animations (on 'display:none' elements) activate the refresh driver
Note that the proposed solution is similar to what we do for animations that are running on the compositor thread, and can probably share code with it.
Blocks: 993259
Comment 32•10 years ago
|
||
Since we had the recent regression with bug 1062119, we'd really like to fix this in Gecko rather than having to find the causes in gaia. Is there anyway we could get this prioritized or what needs to be done so that we can get to this? Thanks!
Flags: needinfo?(dholbert)
Comment 33•10 years ago
|
||
Would be nice for 2.1, but that seems unlikely. Perhaps we can have this prioritized for our 2.2 release?
blocking-b2g: --- → 2.2?
Comment 34•10 years ago
|
||
I agree that we should probably prioritize this, and I also agree that this is unlikely to be fixable for 2.1 (since a patch here is likely to involve some risk, and hence would be scary to backport). I'm told that 2.2 will be based on gecko36 or later, so I think we've got ~12 weeks, if this is targeted for 2.2. I'll bring this up with dbaron in our 1:1 next week.
Comment 35•10 years ago
|
||
I think this is more of a "feature-b2g":2.2? rather than "blocking-b2g":2.2?
Comment 36•10 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #35) > I think this is more of a "feature-b2g":2.2? rather than "blocking-b2g":2.2? right
blocking-b2g: 2.2? → ---
feature-b2g: --- → 2.2?
Comment 37•10 years ago
|
||
Not only is this important for battery use, on low-end devices we've seen this eat 10% of the CPU while in a fullscreen hosted app (running webrtc, which is desparate on such hardware for every cycle it can steal).
Comment 38•10 years ago
|
||
Related: we have seen 20% CPU save when disabling animations on statusbar when a fullscreen app is on top of it.
Comment 39•10 years ago
|
||
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #38) > Related: we have seen 20% CPU save when disabling animations on statusbar > when a fullscreen app is on top of it. Nice. That could explain a lot of the laggish behavior I have on the Nexus S when doing network activity :)
Updated•10 years ago
|
Blocks: b2g-nexuss
Comment 40•10 years ago
|
||
The B2G system app is affected: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/update_manager/update_manager.css#L9 Even if FxOS is idle, a restyle is triggered constantly. And the Gallery and Marketplace apps are probably affected too.
Comment 41•10 years ago
|
||
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #40) > The B2G system app is affected: > > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/ > update_manager/update_manager.css#L9 > Filed bug 1069997 for this one (it's actually caused by an element that can never be displayed... :/)
Comment 42•9 years ago
|
||
Michael, can you follow up this topic? Can you either + or - it? Thanks.
Flags: needinfo?(mtreese)
Comment 43•9 years ago
|
||
Johnny, This looks like it may be a platform issue, can you take a look and see where it should go? Thanks, Michael
Flags: needinfo?(mtreese) → needinfo?(jst)
Comment 44•9 years ago
|
||
For display:none, we shouldn't be running animations on display:none subtrees at all, per recent changes to CSS animations. See bug 984850 comment 5 and 6. But we should also be smarter about animations that are simply not visible too.
Depends on: 822092
Comment 45•9 years ago
|
||
Jet, is this filed where it belongs?
Flags: needinfo?(jst) → needinfo?(bugs)
Comment 46•9 years ago
|
||
Moved it to Core:Layout. Leaving NI:me to reassign.
Component: CSS Parsing and Computation → Layout
Comment 47•9 years ago
|
||
As a workaround, I have this in my userContent.css: * { -moz-animation: none !important; animation: none !important; } Without the workaround, CPU usage stays high enough for my MBP to get hot and for its fans to spin up. This issue is sadly trigged by several popular sites like Twitter and Facebook. If you want to try the workaround, but don't know what userContent.css is, see: https://ffeathers.wordpress.com/2013/03/10/how-to-override-css-stylesheets-in-firefox/
Comment 48•9 years ago
|
||
(In reply to Birunthan Mohanathas [:poiru] from comment #47) > As a workaround, I have this in my userContent.css: > > * { > -moz-animation: none !important; > animation: none !important; > } This didn't work for me. For FF 35.0.1, I see between 33-37 "Energy Impact" in Activity Monitor on OSX 10.10.2. For FF 38.0.1 (nightly) with e10s, I see between 4.5-5.7 "Energy Impact" with the same profile. To test, I created a clean profile and loaded twitter and left it untouched until the energy impact settled down after a few seconds.
I believe twitter's energy problem is related to animated images, not CSS animations. (At least it used to be.)
Assignee | ||
Comment 51•9 years ago
|
||
(In reply to Not doing reviews right now from comment #30) > So here's an interesting question. http://dev.w3.org/csswg/css-animations/ > seems to pretty explicitly say that animations don't run on display:none > elements. David, what _is_ the spec status here? If I'm understanding [1] and [2] correctly, setting display:none should stop the animation entirely. If somebody can outline where to fix this I can probably rustle up some spare cycles to write the patch. [1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=14785 [2] http://lists.w3.org/Archives/Public/www-style/2011Nov/0709.html
Comment 52•9 years ago
|
||
layout/style/nsAnimationManager.cpp is where we actually create the animations. We create the whole set of animations in BuildAnimations then compare it to the existing set (in CheckAnimationRule) and cancel any animations that are in the old list but not the new one. So simply updating BuildAnimations might be enough. Regarding events, I think that will give us the behavior we want--i.e. no 'animationend' event if you set display:none part-way through. In the future we'll probably add an 'animationcancel' event for that. If you set display:block part-way through, the animation should restart which is also the behaviour that will just fall out of updating BuildAnimations. The bigger issue is probably around invisible animations. For compositor animations, when the target is offscreen we don't create a layer and hence we can't run it on the compositor and throttle it so we keep ticking the refresh driver and use more power when the animation is invisible than when it is visible. Or at least I think that was the issue. I think dbaron had some ideas for this but I think he was also waiting to finish some other refactoring first.
Comment 53•9 years ago
|
||
(In reply to Brian Birtles (:birtles, away most of 21 Feb~6 Mar) from comment #52) > The bigger issue is probably around invisible animations. For compositor > animations, when the target is offscreen we don't create a layer and hence > we can't run it on the compositor and throttle it so we keep ticking the > refresh driver and use more power when the animation is invisible than when > it is visible. Or at least I think that was the issue. Correct, we have bug 1105509 about just that.
Assignee | ||
Comment 54•9 years ago
|
||
This fixes the issue I was seeing in bug 1139918, but surely there must be more to it than just this. What am I missing? Or should I do a try push and see what breaks?
Comment 55•9 years ago
|
||
Comment on attachment 8573339 [details] [diff] [review] Naive patch >diff --git a/layout/style/nsAnimationManager.cpp b/layout/style/nsAnimationManager.cpp [...] >- BuildAnimations(aStyleContext, aElement, timeline, newPlayers); >+ if (disp->mDisplay != NS_STYLE_DISPLAY_NONE) { >+ BuildAnimations(aStyleContext, aElement, timeline, newPlayers); I think you need to check whether any of the ancestor nsStyleContexts are display:none as well, too. Also, we need to consider what happens if we *use an animation* to set an element's 'display' property to none. (I assume you can do that.) The links in comment 51 don't really say. Seems like we'd get into a catch-22, where we stop applying animations because we're "display:none"; but the reason we're display:none is *because* of an animation... so maybe that means our animation is no longer applied & we go back to display:block again... but then we can be animated again so we reapply our animation and go back to display:none?
Flags: needinfo?(dholbert)
Assignee | ||
Comment 56•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #55) > I think you need to check whether any of the ancestor nsStyleContexts are > display:none as well, too. Updated patch. I assumed we would want to cache the display:none rather than walking up the style context ancestor chain every time. I'm unsure though if the cache needs recomputing when nsStyleContext::MoveTo is called - it doesn't look like the other bits get reset when that happens. > Also, we need to consider what happens if we *use an animation* to set an > element's 'display' property to none. (I assume you can do that.) Thankfully [1] doesn't say display is animatable (thereby implying it is not). Good thing, too, or you'd have to implement ease-in transitions between display:none and display:block! [1] http://www.w3.org/TR/2012/WD-css3-transitions-20120403/#animatable-properties
Attachment #8573339 -
Attachment is obsolete: true
Attachment #8573558 -
Flags: feedback?(dholbert)
Comment 57•9 years ago
|
||
Comment on attachment 8573558 [details] [diff] [review] Slightly less naive patch (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56) > I'm unsure though if > the cache needs recomputing when nsStyleContext::MoveTo is called - it > doesn't look like the other bits get reset when that happens. Yeah -- I'd expect we'd need to check the new parent after MoveTo, though I'm not sure either. It does seem like there are some other bits (like the INLINE_DESCENDANT_OF_RUBY one) should be updated if MoveTo is called; that may be a bug. > Thankfully [1] doesn't say display is animatable (thereby implying it is > not). Good thing, too, or you'd have to implement ease-in transitions > between display:none and display:block! Ah, nice. I'd thought that maybe CSS animations allowed you to interpolate between keywords in stepwise manner, but I guess not. However, SVG SMIL animation *does* allow you to animate "display"[1], http://www.w3.org/TR/SVG/propidx.html -- and we have that encoded into Gecko at [2]. We manage SMIL animations separately from CSS animations right now (not via this BuildAnimations call, AFAIK) -- but when we someday make all animations share a common implementation (via a single grand Web Animations backend), we'll have to worry about this problem a bit more... [1] http://www.w3.org/TR/SVG/painting.html#DisplayProperty (see "Animatable: yes") [2] http://mxr.mozilla.org/mozilla-central/source/dom/smil/nsSMILCSSProperty.cpp#186 Anyway, the patch looks OK to me (perhaps birtles or heycam should review, when you're ready for full review). Nits below, all around a central theme: ># HG changeset patch ># User Kartikaya Gupta <kgupta@mozilla.com> >Bug 962594 - Don't build CSS animations for elements that are display:none. r= s/are display:none/are in a display:none subtree/ >+++ b/layout/style/nsStyleContext.h >@@ -153,16 +153,20 @@ public: > >+ // Does this style context or any of its ancestor have display:none set? >+ bool IsDescendantOfDisplayNone() const >+ { return !!(mBits & NS_STYLE_IS_DESCENDANT_OF_DISPLAY_NONE); } s/ancestor/ancestors/ Also: The use of "Descendant" is slightly misleading here, since we're including the "I myself am 'display:none'" scenario, and I am *not* a "descendant" of myself. I'd suggest changing to use the term "display:none subtree", e.g. IsInDisplayNoneSubtree() and NS_STYLE_IN_DISPLAY_NONE_SUBTREE.
Attachment #8573558 -
Flags: feedback?(dholbert) → feedback+
Comment 58•9 years ago
|
||
Don't forget to bump NS_STYLE_CONTEXT_TYPE_SHIFT when adding a new style context bit. (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56) > Updated patch. I assumed we would want to cache the display:none rather than > walking up the style context ancestor chain every time. I'm unsure though if > the cache needs recomputing when nsStyleContext::MoveTo is called - it > doesn't look like the other bits get reset when that happens. We should not be calling MoveTo if the bits differ; that is handled by comparing the bits in ElementRestyler::ComputeRestyleResultFromNewContext. Might be good to have an assertion in MoveTo, though, that the old and new parents agree on these flags bits. While you could recompute the NS_STYLE_IN_DISPLAY_NONE_SUBTREE bits in MoveTo, it's probably not worth it, so just add another check to ComputeRestyleResultFromNewContext.
Normally we won't even construct style contexts for descendants of display:none elements -- unless somebody asks for computed style on them. So I wonder if there's a better way that doesn't use up a bit on the style context. Then again, now that we've switched to 64-bits there, we're probably a little less constrained. (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56) > > Also, we need to consider what happens if we *use an animation* to set an > > element's 'display' property to none. (I assume you can do that.) > > Thankfully [1] doesn't say display is animatable (thereby implying it is > not). Good thing, too, or you'd have to implement ease-in transitions > between display:none and display:block! > > [1] > http://www.w3.org/TR/2012/WD-css3-transitions-20120403/#animatable-properties That said, there's a working group resolution in https://lists.w3.org/Archives/Public/www-style/2014Oct/0290.html that changes that, since it makes all non-interpolable properties settable by animations. (It's not clear if it's going to end up being Web-compatible.) At some point we should also fix this for visual-only properties that are offscreen. That might lead to a stronger fix that allows this one to be removed.
Flags: needinfo?(dbaron)
Assignee | ||
Comment 60•9 years ago
|
||
So I made all the changes in comments 57-58, including the MoveTo assertion, and pushed to try [1]. The try push shows crashes on the assert that I added. I reproduced it locally and it looks like MoveTo is getting called in a scenario where the two parents have a different value for the NS_STYLE_HAS_TEXT_DECORATION_LINES bit. Is that expected? Maybe my assertion was checking the wrong bits? See [2] for what I did. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=38ac2f818b93 [2] https://hg.mozilla.org/try/rev/6c2b17a1fc8b#l3.15
Flags: needinfo?(cam)
Assignee | ||
Comment 61•9 years ago
|
||
(And yes, that code is ugly. I will use a DebugOnly<uint64_t> mask to remove the duplication for the final version)
(In reply to David Baron [:dbaron] (UTC-8) from comment #59) > At some point we should also fix this for visual-only properties that are > offscreen. That might lead to a stronger fix that allows this one to be > removed. Actually, ignore that. Since this bug is also fixing things to match a statement in the spec, not building animations should be the right fix here, whereas the work for visual-only properties would be an optimization that changes mNeedsRefreshes but doesn't change whether we build animations. That said, given that there's spec prose here, there should probably be some relevant tests in test_animations.html to test it too! (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #60) > So I made all the changes in comments 57-58, including the MoveTo assertion, > and pushed to try [1]. > > The try push shows crashes on the assert that I added. I reproduced it > locally and it looks like MoveTo is getting called in a scenario where the > two parents have a different value for the > NS_STYLE_HAS_TEXT_DECORATION_LINES bit. Is that expected? Maybe my assertion > was checking the wrong bits? See [2] for what I did. That may well be a regression from bug 931668. The optimization for bug 931668 should probably get disabled if the text-decoration bit is different or if your new display-none bit is different.
(In reply to David Baron [:dbaron] (UTC-8) from comment #62) > (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #60) > > So I made all the changes in comments 57-58, including the MoveTo assertion, > > and pushed to try [1]. > > > > The try push shows crashes on the assert that I added. I reproduced it > > locally and it looks like MoveTo is getting called in a scenario where the > > two parents have a different value for the > > NS_STYLE_HAS_TEXT_DECORATION_LINES bit. Is that expected? Maybe my assertion > > was checking the wrong bits? See [2] for what I did. > > That may well be a regression from bug 931668. The optimization for bug > 931668 should probably get disabled if the text-decoration bit is different > or if your new display-none bit is different. Though ElementRestyler::ComputeRestyleResultFromNewContext looks like it already checks the text-decoration bit. Presumably it should also check your new bit. That doesn't explain the assertion, though.
Assignee | ||
Comment 64•9 years ago
|
||
If I remove the NS_STYLE_HAS_TEXT_DECORATION_LINES from the assert check, there's still another call to MoveTo that's bad, resulting the Android crashtest failure on this try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea3b75d91521 I don't think I'll be able to reproduce this one locally.
Assignee | ||
Comment 65•9 years ago
|
||
I decided to just check my new bit in the assert, since technically that's what my patch cares about. If we want to add other bits to the assert we can do that in follow-up patches.
Attachment #8573558 -
Attachment is obsolete: true
Flags: needinfo?(cam)
Attachment #8574363 -
Flags: review?(cam)
Assignee | ||
Comment 66•9 years ago
|
||
Try with this patch is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fccff6c4bb7a
Comment 67•9 years ago
|
||
Comment on attachment 8574363 [details] [diff] [review] Patch Review of attachment 8574363 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. I am also not sure about those assertion failures in the previous patches you pushed to try. They look like a real problem -- can you file a bug for that (or mention it in the followup bug you file for expanding |mask| to cover more flags)? We really need some tests for this, too, before we land it.
Attachment #8574363 -
Flags: review?(cam) → review+
Assignee | ||
Comment 68•9 years ago
|
||
Tests attached. These fail without the patch and pass with the patch, tested locally on OS X (with OMTA forced on). Also pushed to try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=26f7f12c25de
Assignee: nobody → bugmail.mozilla
Attachment #8576681 -
Flags: review?(cam)
Assignee | ||
Updated•9 years ago
|
status-b2g-v2.1:
--- → wontfix
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → affected
Whiteboard: [possible solution in comment 25 part b]
Assignee | ||
Comment 69•9 years ago
|
||
I filed bug 1142531 for adding the asserts into MoveTo and fixing the failures, or otherwise dealing with that problem.
Comment 70•9 years ago
|
||
[Canceling stale needinfo from before this bug had an owner] (kats, thanks for taking this!)
Flags: needinfo?(bugs)
Comment 71•9 years ago
|
||
Comment on attachment 8576681 [details] [diff] [review] Part 2 - Tests Review of attachment 8576681 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but can you also add a subtest that checks this behaviour on a child of a display:none element?
Attachment #8576681 -
Flags: review?(cam)
Assignee | ||
Comment 72•9 years ago
|
||
Attachment #8576681 -
Attachment is obsolete: true
Attachment #8577232 -
Flags: review?(cam)
Comment 73•9 years ago
|
||
Comment on attachment 8577232 [details] [diff] [review] Part 2 - Tests (v2) Review of attachment 8577232 [details] [diff] [review]: ----------------------------------------------------------------- Maybe use a helper function in test_animations_omta.html too to keep the files more similar?
Attachment #8577232 -
Flags: review?(cam) → review+
Assignee | ||
Comment 74•9 years ago
|
||
I wanted to do that but the yields made it non-trivial to do and it was cleaner to just have two separate test functions.
Assignee | ||
Comment 75•9 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d00224526057 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d034e0c62f9
Comment 76•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d00224526057 https://hg.mozilla.org/mozilla-central/rev/7d034e0c62f9
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1197620
You need to log in
before you can comment on or make changes to this bug.
Description
•