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-typed-om] Clarify list-valued properties #823

Open
andruud opened this issue Oct 8, 2018 · 7 comments
Open

[css-typed-om] Clarify list-valued properties #823

andruud opened this issue Oct 8, 2018 · 7 comments

Comments

@andruud
Copy link
Member

andruud commented Oct 8, 2018

If a property optionally takes a non-list as a value, is it a list-valued property?

Examples:

  1. counter-reset
  2. transform
  3. content
  4. Custom property with syntax none | <length>+

If such properties are list-valued properties, what should happen when you do e.g. e.attributeStyleMap.append('counter-reset', 'none')?

@tabatkins
Copy link
Member

We haven't figured out the answer to this. :( I suspect this needs to be resolved by separating out the top-level productions of a property grammar into "list-valued" and "single-valued" categories, and throwing an error if you ever try to put a single-valued production into a list-valued mode. (Such as starting with "transform:none" and then appending a value, or vice versa.)

Maybe we special-case list-based interaction with the "empty" values that some properties, like transform, have, so you can start from that value and append list-valued values and have it work "as expected"?

@andruud
Copy link
Member Author

andruud commented Oct 9, 2018

Describing list-valuedness of productions rather than properties makes much more sense!

Re. special-casery of "empty" values, something more general might be needed. Custom properties may have custom "empty" values, e.g. omitted | <number>+, and it's possible to register a property with several different incompatible lists: <length>+ | <url>+. Perhaps append should behave as set whenever you are trying to append something that's incompatible with the current value.

Sidenote: the <length>+ | <url>+-case means that we have to reify the current value to check whether the append is allowed. (We may be attempting to append a list of urls to an existing list of lengths, or vice-versa).

@tabatkins
Copy link
Member

Perhaps append should behave as set whenever you are trying to append something that's incompatible with the current value.

This is a good point. I'll think on this, but it sounds promising.

Sidenote: the <length>+ | <url>+-case means that we have to reify the current value to check whether the append is allowed. (We may be attempting to append a list of urls to an existing list of lengths, or vice-versa).

Well, not reify, right? That's the process of turning the value into a JS object. This can be checked purely with the internal values, right?

@tabatkins
Copy link
Member

Actually, I think what I want is:

  • grammars have their top-level productions (the things separated by |) classified as list-valued or single-valued. (A single TypedOM object always represents either an entire single-valued term, or one iteration of a list-valued term.)
  • you can't ever append() a value that matches a single-valued production. This throws an error.
  • If you append() a value that's a list-valued iteration:
    • if the existing values are of the same production, great, it appends as normal.
    • if the existing value is of a single-valued production, it replaces the existing value silently. (This magically handles cases like appending to none, but also applies to any other single keywords/etc.)
    • if the existing values are of a different list-valued production, maybe that's an error? Or maybe it should replace, I'm unsure.

@andruud
Copy link
Member Author

andruud commented Oct 12, 2018

Well, not reify, right? That's the process of turning the value into a JS object. This can be checked purely with the internal values, right?

Right. (To figure out compatibility between existing and incoming values, we do have to create JS objects internally in Blink at the moment, but you're right: it's an implementation detail).

you can't ever append() a value that matches a single-valued production. This throws an error.

Does this mean the following is an error?

// Given <length> | <length-percentage>+
e.attributeStyleMap.set('--x', '1%');
e.attributeStyleMap.append('--x', '1px');

@tabatkins
Copy link
Member

Ah, bad phrasing on my part. To handle ambiguous things, I guess need to check it against the existing list-valued things. That is, verify that what's already in the property matches one of the list-valued iterations, and that the thing you're appending matches one of the same list-valued iterations.

@css-meeting-bot
Copy link
Member

The Houdini Task Force just discussed Clarify list-valued properties.

The full IRC log of that discussion <TabAtkins> Topic: Clarify list-valued properties
<TabAtkins> github: https://github.com//issues/823
<heycam> TabAtkins: this was brought up by one of our engineers
<heycam> ... I was hoping at the start this issue wouldn't occur. I was hoping to split props into list values and single values
<heycam> ... every prop would be one or the other
<heycam> ... anders pointed out we have several props that at minimum have.a neutral value, but sometimes have more than that
<heycam> ... and the list value / single value split is not enforced in custom props either
<heycam> ... you can always define [ length+ | number+ ]
<heycam> ... not handling that well
<heycam> ... so rather than declare props to be list valued or single valued, declare top level productions as being list valued or single valued in a given prop
<heycam> ... e.g. content
<heycam> ... some of the productions will be single valued, and they can't be used with the append() methods
<heycam> ... some are multi-valued, and can be used with the append() methods
<heycam> ... and if you do the wrong one, and you call styleMap.append("counter-reset", "none")
<heycam> ... I believe we'll throw in that case
<heycam> ... one thing I'm not sure is how to handle the case where it's currently a single value, and you append a list production
<heycam> ... e.g. it's currently set to none, and you want to append real values
<heycam> ... the most common case is none, I'd like that to be easier to handle
<heycam> ... so I'd like to make that silently drop the single value, and turning it into a multi-value production instead
<heycam> ... not sure about this, may be more consistent to throw here
<heycam> ... I believe this design in general, is the way to go to handle all possible props properly
<heycam> ... it's not a significant complexity increase
<heycam> ... it'll be a bit more work continuing we need to make sure they're properly tracked
<dbaron> heycam: what if we change a property from single-value length to multi-value length?
<heycam> TabAtkins: in that case the original length would be classified as a multi value
<dbaron> heycam: so in the updated property that would be defined as a multi-value thing
<heycam> TabAtkins: so let me know if you have opinions on this
<heycam> ... otherwise I'll resolve it like this and edit it in
<heycam> fremy: how you define that syntax, how would you write down your decl to say this is single valued or list valued?
<heycam> TabAtkins: CSS in the specs will define this
<heycam> ... don't have to do that in the API side
<heycam> fremy: for custom properties
<heycam> TabAtkins: right now, custom props are either a single value or there's a + in there
<heycam> ... but once we had the # one, those will be multi-valued

@astearns astearns removed the Agenda+ label Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants