Closed Bug 1372335 Opened 7 years ago Closed 7 years ago

stylo: Chromium's code-review appears blank.

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: emilio, Assigned: hiro)

References

(
URL
)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1372335 - A reftest to check that animation styles are surely discarded when selector matching happens simultaneously.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
See the url.

AFAICT the elements are properly styled and positioned, but the root element's opacity is "0".

It works fine if I add something to the page like |:root { opacity: 1!important }|.

They're using polymer and custom elements and what not, so it's somewhat hard to debug, but... on it.
So at a glance the offender rule is an animation rule:

> Animations Declarations([(opacity: 0, Normal)])

It seems to be originated from this script[1] (there are a few KeyframeEffect in there). If I add a script setting |window.KeyframeEffect| and |window.KeyframeEffectReadOnly| they polyfill it, and the bug still happens.

Hiro, Boris, could you take a look in case this rings a bell? It's not ultra-prioritary I guess, we have a bunch of well-identified animation bugs. But worth checking with you I guess :)

[1]: https://cdn.googlesource.com/polygerrit_ui/344.0/elements/gr-app.js
Flags: needinfo?(hikezoe)
Flags: needinfo?(boris.chiou)
A suspicious point is below script.
                                                                                
(function(scope) {                                                              
  var style = document.createElement("style");                                  
  style.textContent = "" + "body {" + "transition: opacity ease-in 0.2s;" + " } \n" + "body[unresolved] {" + "opacity: 0; display: block; overflow: hidden; position: relative;" + " } \n";
  var head = document.querySelector("head");                                    
  head.insertBefore(style, head.firstChild);                                    
})(window.WebComponents); 

This opacity transition might not work well.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> A suspicious point is below script.
>                                                                             
> 
> (function(scope) {                                                          
> 
>   var style = document.createElement("style");                              
> 
>   style.textContent = "" + "body {" + "transition: opacity ease-in 0.2s;" +
> " } \n" + "body[unresolved] {" + "opacity: 0; display: block; overflow:
> hidden; position: relative;" + " } \n";
>   var head = document.querySelector("head");                                
> 
>   head.insertBefore(style, head.firstChild);                                
> 
> })(window.WebComponents); 
> 
> This opacity transition might not work well.

Note that that transition is on the <body> element, not the <html> one, which is the problematic one. That transition works fine afaict.
Yeah, right. I confirmed now that the page works fine with disabling both of "dom.animations-api.element-animate.enabled" and "dom.animations-api.core.enabled". It means that we fail something related web-animations in sometwhere inside polymer. Ni? to Brian, he knows well about polymer.
Flags: needinfo?(hikezoe) → needinfo?(bbirtles)
Looks like its the feature detection script setting opacity on the root element:

  https://github.com/web-animations/web-animations-js/blob/dev/src/web-animations-bonus-object-form-keyframes.js

I guess the call to animation.cancel() is not working (or something else in that script is throwing before we get to that line).
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #5)
> Looks like its the feature detection script setting opacity on the root
> element:
> 
>  
> https://github.com/web-animations/web-animations-js/blob/dev/src/web-
> animations-bonus-object-form-keyframes.js
> 
> I guess the call to animation.cancel() is not working (or something else in
> that script is throwing before we get to that line).

Thanks. Looks like we should check this script to know why we throw before that line on stylo. We can take a deep look later.
Flags: needinfo?(boris.chiou)
Priority: -- → P2
(In reply to Boris Chiou [:boris] from comment #6)
> Thanks. Looks like we should check this script to know why we throw before
> that line on stylo. We can take a deep look later.

I'm not sure it's throwing. The code before that is in a try {} block so I guess it's not. More likely cancel() is failing somehow.
Or at least, it's in a try {} block in the code on trunk. Looking at the minified file, it looks to be the same:

  function(a){var b=document.documentElement,c=null,d=!1;try{var e=getComputedStyle(b).getPropertyValue("opacity"),
f="0"==e?"1":"0";c=b.animate({opacity:[f,f]},{duration:1}),c.currentTime=0,d=getComputedStyle(b).getPropertyValue("opacity")==f}catch(g){}finally{c&&c.cancel()}
Yeah, I think we don't reliably remove animation rules from rule trees in this case.
OK, we are returning a wrong RestyleKind from restyle_kind() for animation-only restyle so that we don't have a chance to remove the animation rules that the animation has been cancelled.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8877328 [details]
Bug 1372335 - Don't process RestyleKind::MatchAndCascade during animation-only restyle.

https://reviewboard.mozilla.org/r/148680/#review153176

::: servo/components/style/data.rs:640
(Diff revision 1)
> +
> +        if shared_context.traversal_flags.for_animation_only() {
> +            // return either CascadeWithReplacements or CascadeOnly in case of
> +            // animation-only restyle.
> +            if hint.has_animation_hint() {
> +                return RestyleKind::CascadeWithReplacements(hint.replacements);

This is going to need a rebase... Do you want all the replacements, or only `CascadeWithReplacements(hint & RestyleHint::for_animation())`?

Anyway, this looks sensible, r=me.
Attachment #8877328 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8877329 [details]
Bug 1372335 - A reftest to check that animation styles are surely discarded when selector matching happens simultaneously.

https://reviewboard.mozilla.org/r/148682/#review153178

r=me, thanks for fixing this Hiro! (And Brian, for all the help too! :))
Attachment #8877329 - Flags: review?(emilio+bugs) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> Comment on attachment 8877328 [details]
> Bug 1372335 - Don't process RestyleKind::MatchAndCascade during
> animation-only restyle.
> 
> https://reviewboard.mozilla.org/r/148680/#review153176
> 
> ::: servo/components/style/data.rs:640
> (Diff revision 1)
> > +
> > +        if shared_context.traversal_flags.for_animation_only() {
> > +            // return either CascadeWithReplacements or CascadeOnly in case of
> > +            // animation-only restyle.
> > +            if hint.has_animation_hint() {
> > +                return RestyleKind::CascadeWithReplacements(hint.replacements);
> 
> This is going to need a rebase... Do you want all the replacements, or only
> `CascadeWithReplacements(hint & RestyleHint::for_animation())`?

Thank you for reviewing! Fixed just like your comments.

https://github.com/servo/servo/pull/17305
Attachment #8877328 - Attachment is obsolete: true
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1aa3d817dd41
A reftest to check that animation styles are surely discarded when selector matching happens simultaneously. r=emilio
https://hg.mozilla.org/mozilla-central/rev/1aa3d817dd41
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: