Bugzilla

Updated

•

7 years ago

Priority: -- → P2

Brad Werth [:bradwerth]

Comment 3

•

7 years ago

Did a quick reproduction and saw that this is related to keyframes animation. Hiro or Brian will likely be fastest on fixing it.

Flags: needinfo?(hikezoe)

Xidorn Quan [:xidorn] UTC+11

Comment 4

•

7 years ago

Also NI Boris. Whoever gets to it first.

Flags: needinfo?(boris.chiou)

Comment 5

•

7 years ago

This looks more like parsing issue rather than animating code. Also I don't think that assertion needs to be a release assertion, we should probably downgrade it to debug_assert.

Assignee

Comment 6

•

7 years ago

Attached file Test case that causes the crash — Details

We don't allow important flag in keyframes rule parser, but we *do* allow it by setting CSSOM unfortunately.

Flags: needinfo?(hikezoe)

Xidorn Quan [:xidorn] UTC+11

Assignee

Comment 7

•

7 years ago

I take this. Given that CSSOM directly modifies property declaration blocks in keyframes rules, we should filter our !important rule there.

Assignee: nobody → hikezoe

Status: NEW → ASSIGNED

Flags: needinfo?(boris.chiou)

Comment 8

•

7 years ago

Hmm... it's probably worth a spec issue. Filed w3c/csswg-drafts#1824.

Xidorn Quan [:xidorn] UTC+11

Comment 9

•

7 years ago

So I think Gecko is just ignoring it, and doesn't assert on this. Not sure which is better.

Allowing setting priority on declarations in @keyframes means that its serialization may not round-trip, which is conceptually a problem as well.

Probably we should allow !important in @keyframes but just ignore them...

Assignee

Comment 10

•

7 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #9)
> So I think Gecko is just ignoring it, and doesn't assert on this. Not sure
> which is better.

To be clear, Gecko ignores the important property in CSS animation handling, does not ignore it in setting keyframes rules.

E.g. 
When setProperty('background-clor', 'red', 'important') is called for a keyframe at 0%, it replaces the target keyframe, then we can see the keyframes in devtools something like this;

@keyframes anim {
  0% { background-color: red !important; }
100% { background-color: blue; }
}

Then, CSS animation handles the important value invalid.

Simon Sapin (:SimonSapin)

Assignee

Updated

•

7 years ago

Summary: thread '<unnamed>' panicked at 'assertion failed: !importance.important()', servo/components/style/stylesheets/keyframes_rule.rs:379 → stylo: thread '<unnamed>' panicked at 'assertion failed: !importance.important()', servo/components/style/stylesheets/keyframes_rule.rs:379

Comment 11

•

7 years ago

Until CSSWG decides on this I suggest changing CSSOM setProperty and setPropertyPriority to not set declarations in keyframes rules to important. (Either succeed while pretending the "priority" parameter was the empty string, or fail, I don’t have an opinion on that.)

Brian Birtles (:birtles, Feb 26~ parental leave)

Assignee

Comment 12

•

7 years ago

Brian created an example that the important entry handles as invalid in setProperty(), and checked it for other browsers.

The example is: http://jsbin.com/suxabaxoce/edit?css,js,console,output

The result is that all browser store the important entry in keyframes, so stylo also should store it as it is, and handle it as invalid during generating keyframes.

Comment 13

•

7 years ago

Based on some cross-browser testing:

* EdgeHTML, WebKit, Gecko (non-stylo) all animate attachment 8909509 [details] from green to red (i.e. they treat the !important declaration as invalid)
* Only Blink fails to ignore it (tested with Chrome unstable on Linux and Chrome stable on Windows)

As for whether they simply drop the existing declaration or retain the invalid !important declaration, based on [1] it appears they all retain the (invalid) !important declaration.

As discussed with hiro, based on this, the safest thing here is probably just to match Gecko's existing behavior and retain the invalid !important declaration as-is, but ignore it when generating keyframes.

[1] http://jsbin.com/suxabaxoce/edit?css,js,console,output (Note: will crash Stylo builds!)

Assignee

Comment 14

•

7 years ago

Oh dear, the example causes an assertion on Gecko too.

Assertion failure: !aKeyframeDeclaration->HasImportantData() (Keyframe rule has !important data), at /home/ikezoe/autoland/layout/style/nsAnimationManager.cpp:390

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/181174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181174/

Comment hidden (mozreview-request)

Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181174/diff/1-2/

Assignee

Comment 17

•

7 years ago

mozreview-review

Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

https://reviewboard.mozilla.org/r/181174/#review186328

I think this is not perfect, but I'd like to get feedbacks about this approach.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=130ea0ff6de8b1975de29e5c3861aca1478bc743

::: servo/components/style/shared_lock.rs:208
(Diff revision 2)
> +impl Locked<PropertyDeclarationBlock> {
> +    /// Create a new Locked<PropertyDeclarationBlock> with the same SharedRwLock.
> +    pub fn new_with(&self, other: PropertyDeclarationBlock) -> Self {
> +        self.shared_lock.wrap(other)
> +    }
> +}

I did add this implementation to create a new Locked<PropertyDeclarationBlock> without important properties.  Should we pass SharedRwLock instead of stealing shared_lock from existent Locked<PropertyDeclarationBlock>?

Also, I did initially put this impl into declaration_block.rs, but it did not work since the private member, shared_lock can not be accessed there.

::: servo/components/style/stylesheets/keyframes_rule.rs:432
(Diff revision 2)
> +                    let block = keyframe.block.read_with(&guard);
> +                    block.clone_only_normal_importance()
> +                };
> +                Arc::new(keyframe.block.new_with(cloned))
> +            } else {
> +                keyframe.block.clone()

I'd actually want to avoid this clone() but I have no idea how to avoid it.

Assignee

Comment 18

•

7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> Oh dear, the example causes an assertion on Gecko too.
> 
> Assertion failure: !aKeyframeDeclaration->HasImportantData() (Keyframe rule
> has !important data), at
> /home/ikezoe/autoland/layout/style/nsAnimationManager.cpp:390

Filed bug 1401104 for Gecko.
With attachment 8909658 [details] Stylo does not cause any panic and assertions.

Comment 19

•

7 years ago

mozreview-review

Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

https://reviewboard.mozilla.org/r/181174/#review186518

Yeah, I don't really love this patch.

Why do we want this instead of, let's say, something like, adding a `normal_declarations` iterator that filters out important properties, and using it in the 4 or so relevant places, mentioning the spec issue raised by xidorn?

Seems a bit less heavy-weight.

::: servo/components/style/properties/declaration_block.rs:217
(Diff revision 2)
> +    /// no important properties.
> +    pub fn clone_only_normal_importance(&self) -> PropertyDeclarationBlock {
> +        let mut longhands = self.longhands.clone();
> +        let mut declarations = self.declarations.clone();
> +        let mut i = 0;
> +        for (decl, importance) in self.declarations.iter().zip(self.declarations_importance.iter()) {

There's a `declaration_importance_iter` that does just this and is faster.

Attachment #8909658 - Flags: review?(emilio)

Comment hidden (mozreview-request)

Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181174/diff/2-3/

Comment 22

•

7 years ago

mozreview-review

Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

https://reviewboard.mozilla.org/r/181174/#review186798

::: servo/components/style/properties/declaration_block.rs:109
(Diff revision 3)
>          self.iter.size_hint()
>      }
>  }
>  
> +/// Iterator over `PropertyDeclaration` for Importance::Normal.
> +pub struct DeclarationNormalIterator<'a> {

nit: `NormalDeclarationIterator`.

::: servo/components/style/stylesheets/keyframes_rule.rs:430
(Diff revision 3)
>              return result;
>          }
>  
>          for keyframe in keyframes {
>              let keyframe = keyframe.read_with(&guard);
> +            let block = if keyframe.block.read_with(&guard).any_important() {

Why do we need to clone the property declaration block? Let's filter it out instead whenever we go over the properties instead (in `glue.rs`).

Attachment #8909658 - Flags: review?(emilio)

Assignee

Comment 23

•

7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #22)

> ::: servo/components/style/stylesheets/keyframes_rule.rs:430
> (Diff revision 3)
> >              return result;
> >          }
> >  
> >          for keyframe in keyframes {
> >              let keyframe = keyframe.read_with(&guard);
> > +            let block = if keyframe.block.read_with(&guard).any_important() {
> 
> Why do we need to clone the property declaration block? Let's filter it out
> instead whenever we go over the properties instead (in `glue.rs`).

Because I'd like to fix this bug both for servo and stylo altogether.  OK, I am fine with fixing it in glue.rs.

Comment 24

•

7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #22)
> 
> > ::: servo/components/style/stylesheets/keyframes_rule.rs:430
> > (Diff revision 3)
> > >              return result;
> > >          }
> > >  
> > >          for keyframe in keyframes {
> > >              let keyframe = keyframe.read_with(&guard);
> > > +            let block = if keyframe.block.read_with(&guard).any_important() {
> > 
> > Why do we need to clone the property declaration block? Let's filter it out
> > instead whenever we go over the properties instead (in `glue.rs`).
> 
> Because I'd like to fix this bug both for servo and stylo altogether.  OK, I
> am fine with fixing it in glue.rs.

Isn't that just a matter of using the new iterator in `compute_style_for_animation_step`?

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/181542/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181542/

Comment hidden (mozreview-request)

Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181174/diff/3-4/

Assignee

Comment 27

•

7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #24)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> > (In reply to Emilio Cobos Álvarez [:emilio] from comment #22)
> > 
> > > ::: servo/components/style/stylesheets/keyframes_rule.rs:430
> > > (Diff revision 3)
> > > >              return result;
> > > >          }
> > > >  
> > > >          for keyframe in keyframes {
> > > >              let keyframe = keyframe.read_with(&guard);
> > > > +            let block = if keyframe.block.read_with(&guard).any_important() {
> > > 
> > > Why do we need to clone the property declaration block? Let's filter it out
> > > instead whenever we go over the properties instead (in `glue.rs`).
> > 
> > Because I'd like to fix this bug both for servo and stylo altogether.  OK, I
> > am fine with fixing it in glue.rs.
> 
> Isn't that just a matter of using the new iterator in
> `compute_style_for_animation_step`?

yeah, right. but we also need double ended iterator since we use rev() there (I don't know why rev() is necessary though).

Comment 28

•

7 years ago

mozreview-review

Comment on attachment 8910066 [details]
Bug 1400926 - Filter out !important property in keyframes for servo.

https://reviewboard.mozilla.org/r/181542/#review186930

::: servo/components/style/animation.rs:491
(Diff revision 1)
> +                // so we have to filter them out.
> +                // See the spec issue https://github.com/w3c/csswg-drafts/issues/1824
> +                // Also we filter our non-animatable properties.
> +                guard.normal_declaration_iter()
> +                     .filter(|declaration| declaration.is_animatable())
> +                     .rev()

Doesn't need `rev()`, let's get rid of this.

::: servo/components/style/properties/declaration_block.rs:151
(Diff revision 1)
>      fn size_hint(&self) -> (usize, Option<usize>) {
>          self.iter.size_hint()
>      }
>  }
>  
> +impl<'a> DoubleEndedIterator for NormalDeclarationIterator<'a> {

Let's get rid of this too.

Attachment #8910066 - Flags: review?(emilio) → review+

Comment 29

•

7 years ago

mozreview-review

Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

https://reviewboard.mozilla.org/r/181174/#review186934

::: layout/style/crashtests/1400926.html:2
(Diff revision 4)
> +<!DOCTYPE html>
> +<html>

nit: No need for `<html>`, specially if there isn't a closing tag.

::: servo/components/style/properties/declaration_block.rs:117
(Diff revision 4)
>      }
>  }
>  
> +/// Iterator over `PropertyDeclaration` for Importance::Normal.
> +pub struct NormalDeclarationIterator<'a> {
> +    iter: Zip<Iter<'a, PropertyDeclaration>, smallbitvec::Iter<'a>>,

I think this would still be nicer just wrapping `DeclarationImportanceIterator`, any reason that's not done?

Anyway, looks ok.

Attachment #8909658 - Flags: review?(emilio) → review+

Assignee

Comment 30

•

7 years ago

Attached file Servo PR — Details

Comment hidden (mozreview-request)

Comment on attachment 8909658 [details]
Bug 1400926 - Filter out !important property in keyframes for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181174/diff/4-5/

Assignee

Updated

•

7 years ago

Attachment #8910066 - Attachment is obsolete: true

Assignee

Comment 32

•

7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bcf1ead034e29c5c53b8d20f87f6e9a9975fbb3

Comment 33

•

7 years ago

https://hg.mozilla.org/integration/autoland/rev/89b1fac5342d

NI Hiro to land the testcase.

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

Resolution: --- → FIXED

Updated

•

7 years ago

Flags: needinfo?(hikezoe)