Skip to content
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

[css-properties-values-api] Generally ignore syntax in APIs. #912

Merged
merged 2 commits into from
Jul 22, 2019

Conversation

andruud
Copy link
Member

@andruud andruud commented Jul 2, 2019

Resolves #902.

@andruud
Copy link
Member Author

andruud commented Jul 2, 2019

😿 (but also 🎉)

@emilio, can you take a look at this and verify that this is what we want?

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not that familiar with the reification stuff, but it looks sane to me. Thanks for doing this :)

css-properties-values-api/Overview.bs Outdated Show resolved Hide resolved
@tabatkins
Copy link
Member

Hmmm, losing the reification behavior sucks. :( It means there's no way to interact with, say, a "<length>"-registered property as a CSSUnitValue, unless you go parse it yourself with your own knowledge.

Is there any way we can save this? It'll require flushing style data and be racy with stylesheets, unfortunately.

Even if we don't think it's reasonable to salvage .get()'s reification behavior, I'd want some way to say "reify this as --foo property specifies". But anything we add will also be racy. We can't even rely on, say, using a promise that doesn't resolve until the property is registered, since there might be multiple registrations.

@andruud
Copy link
Member Author

andruud commented Jul 5, 2019

losing the reification behavior sucks. :(

From the author's perspective, yeah 😿. But a <length>-registered property being CSSUnparsedValue at parse-time/specified-value-"time" is actually the truth. Since the parser is producing a typeless token stream, I think it's appropriate that we reify accordingly. We probably shouldn't try to "fix" this problem in the wrong place, instead we should try to change what is true in the parser. (Though this is probably also hard, and there's that resolution that says we can't).

I can't see any good way to solve everything in this case. :(

One related thought is that we could allow any object-value through Typed OM (regardless of registration status), and in general allow custom properties to hold any value (not just token streams).

// (Don't know or care if --x is registered or not)
elm.attributeStyleMap.set('--x', CSS.px(10));
elm.attributeStyleMap.get('--x'); // Returns CSSUnitValue

In other words, the underlying value created from CSS.px(10) retains its type such that we can reify the same type back. If --x turns out to be registered with a syntax that doesn't match, it's invalid at computed value time. If it's not registered, it substitutes as the tokenized serialization of the value. Note that this would apply to css-variables, not css-properties-values-api. At least this gives us some minimal typeness for specified styles.

@andruud
Copy link
Member Author

andruud commented Jul 16, 2019

@tabatkins We will probably need to think outside the box we've created for ourselves to proceed, so here goes:

We could make @property a thing which takes effect immediately during parsing:

@property --x {
  syntax: "<length>";
  inherits: false;
  initial-value: 0px;
}

--x: 10px; /* Actually parsed as <length> parse-time */

Let's invent a [[declaredPropertySet]] slot on the associated Document. This slot is like [[registeredPropertySet]], except @property properties go in [[declaredPropertySet]] and CSS.registerProperty properties go in [[registeredPropertySet]].

  1. We may query [[registeredPropertySet]] during setProperty, specified value reification in Typed OM (and so forth), but not the [[declaredPropertySet]] slot. The [[declaredPropertySet]] slot may be queried (if necessary) by computed-value APIs ONLY.
    • This means we can continue to reify specified values according to what's registered with CSS.registerProperty. I don't think we actually should, as we'd be misrepresenting the truth in the CSS declaration block.
  2. @property rules take effect immediately in the parser, which means that the parser can actually parse subsequent declarations according to the syntax right away.
  3. Since we parse real values right away, they exist natively in the parsed representation, and hence specified values reify with proper types automatically.
  4. @supports can now detect whether a given @property rule worked. (Yay, fallback).
  5. We can reject declarations parse-time.
  6. Properties declared with @property still substitute as their computed values. This means that previously parsed custom properties must be reinterpreted computed-value time, if a property is re-registered:
@property --x {
  syntax: "<length>";
  inherits: false;
  initial-value: 0px;
}

--x: 10px; /* <length> */

@property --x {
  syntax: "<color>";
  inherits: false;
  initial-value: red;
}

/* --x is still a <length>, but is now invalid at computed-value time */
  1. If a property is also registered with CSS.registerProperty, that registration wins, and any value parsed as a result of @property rules are reinterpreted computed-value time.

Open questions:

  1. How to determine whether a property inherits if multiple @property rules exist and disagree.
  2. Which types do we accept (via .set() etc) in TypedOM for @property-registered properties? We can't look at the at the [[declaredPropertySet]] slot. We could accept anything (see previous comment). Or we could invent something new, like associating the [[declaredPropertySet]] with the StylePropertyMap/'CSS declaration block' instead of the Document. That way, the information we need is always locally available whenever something is parsed.

@emilio
Copy link
Contributor

emilio commented Jul 16, 2019

We could make @Property a thing which takes effect immediately during parsing:

That sounds not great... What happens if we removeRule the @property rule? Do we need to go find all the following declarations for that property and re-parse them as unparsed values? From which text, if we've parsed them as <length> previously?

@andruud
Copy link
Member Author

andruud commented Jul 16, 2019

That sounds not great...

OK.

What happens if we removeRule the @Property rule? Do we need to go find all the following declarations for that property and re-parse them as unparsed values?

Yes, no, maybe. I don't know yet.

From which text, if we've parsed them as previously?

Note that custom properties must already preserve their exact input for serialization. Naturally this isn't spec'd anywhere. (I'm not implying that this automatically makes this @property behavior work).

@emilio
Copy link
Contributor

emilio commented Jul 16, 2019

Sorry, to be clear I don't try to be harsh, just trying to point out stuff that probably makes something hard. I very much appreciate the work y'all do ^.^

@andruud
Copy link
Member Author

andruud commented Jul 16, 2019

^.^

😹

I had some ideas for how to deal with deleteRule which could avoid reparsing, but it doesn't matter, since insertRule is even worse. I guess we'd have to reparse. Meh.

@andruud
Copy link
Member Author

andruud commented Jul 16, 2019

Or perhaps not allow insertRule / deleteRule operations such that it would affect values that are already parsed. :P

@emilio
Copy link
Contributor

emilio commented Jul 16, 2019

So my issue with reparsing is that it's not only reparsing already-parsed stuff, but also potentially bringing back to life declarations that were overridden.

I think "operations such that it would affect values that are already parsed" is also pretty hard to detect for the same reason.

So for example, for the case where you have a length property called --foo, I assume that, if it's registered before that:

div {
  --foo: 10px; /* This one parses. */
  --foo: red; /* Gets dropped at parse-time, I guess? */
}

If then I deleteRule the @property rule, I need to brin the --foo: red back from nowhere. Unless I'm misunderstanding the proposal.

(edit: use css comments, :))

@andruud
Copy link
Member Author

andruud commented Jul 17, 2019

potentially bringing back to life declarations that were overridden [...] from nowhere

Yeah, I know ... we can't do that.

I think "operations such that it would affect values that are already parsed" is also pretty hard to detect for the same reason.

It could be more defensive, like "insertions/deletions of @property rules affecting a custom property that has been used anywhere, once, ever". This specifically might not be what we want ... but I think if we want these properties to behave natively ( would be nice, right? :-) ), we'd need to define the properties outside the "regular author scope" of CSS, or limit how/when @property rules appear (which can have almost the same effect).

@tabatkins
Copy link
Member

I'm ready to merge this; any additional concerns, Anders or Emilio?

The end result is just that we can't reify according to a grammar until computed-value time; any specified values show as CSSUnparsedValue like an unregistered property, or "*"-grammared registered property. And setting a property pays no attention to grammar in any API.

@andruud
Copy link
Member Author

andruud commented Jul 22, 2019

👍 / 😢 (Mixed feelings, but this is probably for the better).

any additional concerns

Just that it annoys me that the path forward to mitigating the TypedOM situation is so unclear right now. Unless you have some new insight to share, Tab?

any API

Don't forget to also undo the change for two-param CSS.supports!

@tabatkins
Copy link
Member

Hey, so long as computed values still reify well, I'm happy enough. The losses are worth gaining @property.

@tabatkins tabatkins merged commit 75ec921 into w3c:master Jul 22, 2019
@tabatkins
Copy link
Member

tabatkins commented Jul 22, 2019

Don't forget to also undo the change for two-param CSS.supports!

Done. (In w3c/csswg-drafts@47269f7)

@emilio
Copy link
Contributor

emilio commented Jul 23, 2019

Hey, so long as computed values still reify well, I'm happy enough. The losses are worth gaining @Property.

Yeah, I agree with this sentiment. Thanks for bearing with me y'all :)

@andruud andruud deleted the no_invalidate branch July 24, 2019 11:24
aarongable pushed a commit to chromium/chromium that referenced this pull request Jul 28, 2019
With recent spec issues finally resolved [1], and corresponding adjustments
made to Blink and WPT [2][3][4], we can enable this again.

[1] w3c/css-houdini-drafts#912
[2] https://crrev.com/88ae629e1ed425385c46369e912ed96baac6c4d9
[3] https://crrev.com/6ce684bfcdfdebf69bd50e5f3e51f48078eb43c8
[4] https://crrev.com/4aa028f816fb7e87194510906d687a7aee26652a

I2S: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/QjcyhPZ_sNI/SuB-GuNPDAAJ
Bug: 641877
Change-Id: I90874324600cbe268a6c0f2af9b2c772d7e1754a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1718566
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#681669}
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] @property vs setProperty
3 participants