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)
Core
CSS Parsing and Computation
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.
Assignee | ||
Updated•6 years ago
|
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.
Assignee | ||
Comment 2•6 years ago
|
||
I think as of right now it's not observable... But sure the value parsing bits look fishy...
Assignee | ||
Comment 3•6 years ago
|
||
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 hidden (mozreview-request) |
Comment 5•6 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 6•6 years ago
|
||
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)
Assignee | ||
Comment 7•6 years ago
|
||
(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•6 years ago
|
||
(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.
Comment 10•6 years ago
|
||
Thank you, dbaron! Filed the spec issue now. https://github.com/w3c/csswg-drafts/issues/2736
Flags: needinfo?(bbirtles)
Comment 11•6 years ago
|
||
mozreview-review |
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 12•6 years ago
|
||
mozreview-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?
Assignee | ||
Comment 13•6 years ago
|
||
(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•6 years ago
|
||
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
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b1c44c3fe871
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•