Skip to content

Conversation

@andruud
Copy link
Member

@andruud andruud commented Dec 12, 2018

This adds basic support for @property, without solving any of the potentially complicated issues, like #845 and #846.

Resolves #137, at least partially.

@heycam
Copy link
Contributor

heycam commented Jun 13, 2019

@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...

@dbaron dbaron requested a review from tabatkins June 13, 2019 22:59
@astearns
Copy link
Member

I took a look, but @tabatkins should review.

@emilio
Copy link
Contributor

emilio commented Jun 14, 2019

What happens when you dynamically remove an @property rule? What about multiple @property rules with the same name?

@andruud
Copy link
Member Author

andruud commented Jun 14, 2019

Thank you @heycam, @astearns & @emilio.

What happens when you dynamically remove an @property rule?

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.

What about multiple @property rules with the same name?

Yeah, this is interesting (since CSS.registerProperty throws if you try to re-register). I assume we want the last one to apply for @property, even if that's the opposite behavior of CSS.registerProperty). WDYT?

@emilio
Copy link
Contributor

emilio commented Jun 15, 2019

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 @keyframes so it seems fair.

@emilio
Copy link
Contributor

emilio commented Jun 16, 2019

Also, I'd note that with the decision we took in #880, it means that setProperty and CSS.supports may need to update style data (I think the Blink concept is doing an active style update?).

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.

@andruud
Copy link
Member Author

andruud commented Jun 16, 2019

@emilio Please excuse my ignorance, but why would setProperty/CSS.supports need to update style data? (And what is "style data" anyway?)

@andruud
Copy link
Member Author

andruud commented Jun 16, 2019

@emilio Oh, never mind, I see what you mean ... hmm ...

@emilio
Copy link
Contributor

emilio commented Jun 17, 2019

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 setProperty call (since there may be new @property rules for example that you still haven't processed), which I think is really bad, not only for the unexpected, potentially big amount of work that that requires, but also because setProperty at least is a way to potentially dirty this data. Of course you can try to be smarter about invalidating and updating it, but I think it's not great even with that.

This adds basic support for @Property, without solving any of the
potentially complicated issues, like w3c#845 and w3c#846.

Resolves w3c#137, at least partially.
Name: syntax
Value: <<string>>
For: @property
Initial: "*"
Copy link
Contributor

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.

Copy link
Member Author

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 "*".

Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

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!

@tabatkins tabatkins merged commit a5cd596 into w3c:master Jul 22, 2019
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.
Copy link
Member Author

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! 😱

Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[css-properties-values-api] Allow custom property descriptors with a CSS @-rule

6 participants