Closed Bug 1466136 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file)

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.
Bug 1466008, that is :)
Depends on: 1466008
Summary: Do not look at the rule type when parsing property ids. → Do not look at the rule type when parsing property ids from values.
I think as of right now it's not observable... But sure the value parsing bits look fishy...
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 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`.
Attachment #8982557 - Flags: review?(xidorn+moz)
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.
Attachment #8982557 - Flags: review?(hikezoe)
(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.
(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
 }
Flags: needinfo?(bbirtles)
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.
Thank you, dbaron!  Filed the spec issue now. https://github.com/w3c/csswg-drafts/issues/2736
Flags: needinfo?(bbirtles)
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.
Attachment #8982557 - Flags: review?(hikezoe) → review+
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?
(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.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1c44c3fe871
Don't look at the rule type from value parsing. r=hiro
https://hg.mozilla.org/mozilla-central/rev/b1c44c3fe871
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: