-
Notifications
You must be signed in to change notification settings - Fork 142
[css-properties-values-api] Add @property. #847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@tabatkins @astearns @therealglazou could one of you review and merge this PR if it looks good? Since @andruud is beginning implementation it would be preferable to have this in the spec, so that others can look at it... |
|
I took a look, but @tabatkins should review. |
|
What happens when you dynamically remove an |
|
Thank you @heycam, @astearns & @emilio.
Then I'd say the property isn't registered anymore. There is a note saying we might add a way to unregister a property ... I guess this does that.
Yeah, this is interesting (since CSS.registerProperty throws if you try to re-register). I assume we want the last one to apply for |
|
Seems ok, though kind of unfortunate... In general having two ways of doing the same is not great IMHO. But "last wins" seems consistent with |
|
Also, I'd note that with the decision we took in #880, it means that I think that's terrible, fwiw, and if I'd have known there were plans to introduce a declarative way of defining these I'd have objected even more strongly to that. |
|
@emilio Please excuse my ignorance, but why would setProperty/CSS.supports need to update style data? (And what is "style data" anyway?) |
|
@emilio Oh, never mind, I see what you mean ... hmm ... |
|
Yeah, sorry, style data is a Servo-ism, it means the "processed" data out of the parsed stylesheets. This is why I used the "active style" concept which is a WebKit-ism :) But yeah, it basically means that you need to look at whether the processed data from stylesheets is dirty and update it from a |
| Name: syntax | ||
| Value: <<string>> | ||
| For: @property | ||
| Initial: "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* is wrong syntax, since means zero or more occurrences multiplier with missed term before it. Looks like it should be Nothing like for other descriptors if it's a required property, or "<declaration-value>" if it will become optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not *, it's "*".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I mean wrong syntax for a custom property value (inside quotes used specific grammar). So, * is not valid for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This accepts a <string>, hence "*" is valid in that sense.
What is or isn't a valid syntax descriptor is determined by 3.4.2. Consume a syntax descriptor, not css3-values (directly).
In other words, a single * alone is a special snowflake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thank you for clarification!
| Declarations that contain other descriptors not defined here, must be ignored to | ||
| maintain forwards compatiblity. | ||
| If an ''@property'' rule contains any unknown descriptors, | ||
| the entire rule is invalid and must be ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tabatkins Ouch, my forwards compat! 😱
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I misread what you'd wrote there!
This adds basic support for
@property, without solving any of the potentially complicated issues, like #845 and #846.Resolves #137, at least partially.