1466136 - Do not look at the rule type when parsing property ids from values.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez [:emilio]]

In bug XXX I had to land a followup:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/24447bd95fbd88c310b93e096f476873364a7ab1

Which did set the rule type in the custom properties code, since now it's used by will-change. But looks to me like using it is slightly bogus, and will cause fun stuff to happen when using properties in @keyframe blocks and such.

We should probably ignore the rule type there.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Bug 1466008, that is :)

Comment 2

[Emilio Cobos Álvarez [:emilio]]

I think as of right now it's not observable... But sure the value parsing bits look fishy...

Comment 3

[Emilio Cobos Álvarez [:emilio]]

I'd use parse_enabled_for_all_content. That may be enough for will-change, but I also wanted to use PropertyId::parse in transition-property to fix bug 1419695.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1466136: Don't look at the rule type from value parsing.

This would cause properties to change the value semantics between, e.g.,
@keyframes and non-@keyframes, which would be observable.

It happens not to be observable since the animation-* and transition-*
properties are not allowed in @keyframes, nor have bits in `contain`, and none
of the two properties are allowed in @page. But I think it's the right thing to
do.

This still causes a quirk like a property value in chrome / user origins being
potentially different if the value is specified via CSS var functions. But I
think that is fine.

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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8982557 [details]
Bug 1466136: Don't look at the rule type from value parsing.

https://reviewboard.mozilla.org/r/248546/#review254936

I'm not very convined that this is the right thing to do.

You can probably try convincing birtles or hiro. If they think it's the right thing to do, I wouldn't be against.

::: commit-message-dc2f0:6
(Diff revision 1)
> +It happens not to be observable since the animation-* and transition-*
> +properties are not allowed in @keyframes, nor have bits in `contain`, and none
> +of the two properties are allowed in @page. But I think it's the right thing to
> +do.

It seems to me `transition-*` are allowed in `@keyframes`.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8982557 [details]
Bug 1466136: Don't look at the rule type from value parsing.

Hiro, mind taking a look at this?

This makes it so that transition-property / will-change has the same semantics in and outside of @keyframes blocks. I think it's the right thing to do, and was the behavior we had until very recently.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #5)
> Comment on attachment 8982557 [details]
> Bug 1466136: Don't look at the rule type from value parsing.
> 
> https://reviewboard.mozilla.org/r/248546/#review254936
> 
> I'm not very convined that this is the right thing to do.
> 
> You can probably try convincing birtles or hiro. If they think it's the
> right thing to do, I wouldn't be against.
> 
> ::: commit-message-dc2f0:6
> (Diff revision 1)
> > +It happens not to be observable since the animation-* and transition-*
> > +properties are not allowed in @keyframes, nor have bits in `contain`, and none
> > +of the two properties are allowed in @page. But I think it's the right thing to
> > +do.
> 
> It seems to me `transition-*` are allowed in `@keyframes`.

Well, sure, but the animation properties are not transitionable, so I think it's still not observable.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #5)
> ::: commit-message-dc2f0:6
> (Diff revision 1)
> > +It happens not to be observable since the animation-* and transition-*
> > +properties are not allowed in @keyframes, nor have bits in `contain`, and none
> > +of the two properties are allowed in @page. But I think it's the right thing to
> > +do.
> 
> It seems to me `transition-*` are allowed in `@keyframes`.

Yes, right.  The CSS animations spec doesn't mention it at all, whereas the web animation spec does clearly mention it in '6.6.3. Processing a keyframes argument'.

From https://drafts.csswg.org/web-animations-1/#processing-a-keyframes-argument

 2. Build up a list of animatable properties as follows:

   1. Let animatable properties be a list of property names (including shorthand properties that have longhand sub-properties that are animatable) that can be animated by the implementation.

But this is for setKeyframes() and animate() method, not for @keyframes.  So if we did implement precisely CSS animations keyframes handling what the spec defines, this bug could be observable by getKeyframes() method (with/without the patch).

Brian, the CSS animation spec just mentions that properties with !important and animation-XX (other than animation-timing-function) are ignored in @keyframes.  Should we raise a spec issue or fix our implementation?  Note that in such cases chrome does return objects that doesn't include CSS property name there something like this;

 {
   composite: null
   computedOffset: 0
   easing: "ease"
   offset: 0
 }

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think at one point the CSS WG agreed that transition-* wasn't allowed in keyframes -- although I'm not sure -- and if it's not in the spec, I'm not sure if it's because of editing error or because there was a later agreement to change it.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Thank you, dbaron!  Filed the spec issue now. https://github.com/w3c/csswg-drafts/issues/2736

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8982557 [details]
Bug 1466136: Don't look at the rule type from value parsing.

https://reviewboard.mozilla.org/r/248546/#review255284

Actually this looks a bit error-prone to me, but I am OK.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8982557 [details]
Bug 1466136: Don't look at the rule type from value parsing.

https://reviewboard.mozilla.org/r/248546/#review255286

::: servo/components/style/values/specified/box.rs:751
(Diff revision 1)
>              TransitionProperty::Unsupported(ref i) => i.to_css(dest),
>          }
>      }
>  }
>  
>  impl Parse for TransitionProperty {

On my local repo, this specified/box.rs doesn't have impl Parse for TransitionProperty, where did it come from?

Comment 13

[Emilio Cobos Álvarez [:emilio]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> Comment on attachment 8982557 [details]
> Bug 1466136: Don't look at the rule type from value parsing.
> 
> https://reviewboard.mozilla.org/r/248546/#review255286
> 
> ::: servo/components/style/values/specified/box.rs:751
> (Diff revision 1)
> >              TransitionProperty::Unsupported(ref i) => i.to_css(dest),
> >          }
> >      }
> >  }
> >  
> >  impl Parse for TransitionProperty {
> 
> On my local repo, this specified/box.rs doesn't have impl Parse for
> TransitionProperty, where did it come from?

Bug 1419695.

To be clear, the issue this fixes is the property _values_ having different semantics in different rules, which I think it's not something we want to introduce...

And it is indeed somewhat error prone, but there's the nice thing that test_value_storage.html catches, given the lack of rule_type in the parser for custom properties.

I'm also not in love with this, but lacking a reason to really introduce this distinction, I think this is the right thing to do.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1c44c3fe871
Don't look at the rule type from value parsing. r=hiro

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b1c44c3fe871