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-shadow-parts] decide on forwarding micro-syntax for partmap #2411

Closed
fergald opened this issue Mar 7, 2018 · 31 comments
Closed

[css-shadow-parts] decide on forwarding micro-syntax for partmap #2411

fergald opened this issue Mar 7, 2018 · 31 comments

Comments

@fergald
Copy link
Contributor

fergald commented Mar 7, 2018

As discussed at Web Components F2F (2018-03, Tokyo), update spec, following on from splitting part= and partmap= finalize forwarding syntax. Discussion in TAG issue proposes e.g.

partmap="a b, c d, e"

vs

partmap="a => b, c => d, e"

("e" is included here as an example of forwarding without renaming).

@fergald
Copy link
Contributor Author

fergald commented Mar 26, 2018

@tabatkins @rniwa

I was thinking a bit more about this. The spec allows

a => b, a => c, ...

so should we allow

a => b c, ...

? In which case, I think the => becomes more helpful and

a b c, ...

really seems hard for humans.

@rniwa
Copy link

rniwa commented Mar 26, 2018

What does a => b, a => c mean in this context? What concrete use cases would need to do that?

@fergald
Copy link
Contributor Author

fergald commented Mar 26, 2018

The concrete use case that I can think of is migrating to a new part-name interface and implementing both the old and the new interface for as long as needed, e.g.

#shadow

c-e1/ce-2/a can be styled using the old naming scheme "b" and also the new naming scheme, "c"

While I'm at it, my instinctual interpretation of "a=>b, a =>c" is really that c-e1::part(a) would match against
some inner parts b and c. The fact that it goes the opposite direction keeps surprising me. If we reversed them then "a=>b, a=>c" would signify grouping 2 inner parts under the same name which seems like a more common operation that might be more worth shortening to "a => b c".

@rniwa
Copy link

rniwa commented Mar 26, 2018

By migrating to a new part-name interface, you mean from v0 API chrome supported? I don't think we'd need a short-hand for that since that's most likely taken care of by some sort of a library. It would be pretty crazy for each component author to keep around v0/v1 syntax mixed together.

The fact you're confused about the direction of the arrow is yet another reason not to have the arrow. Like I said during F2F, the arrow doesn't have an inherent semantics here, and won't add any meaning beyond the order of tokens would convey.

@fergald
Copy link
Contributor Author

fergald commented Mar 26, 2018

By migrating, I mean, I defined a custom element with a part name interface (I think of part names as the styling interface for a CE) and then I realize later that my interface is bad and I would like to migrate to a better one but I don't want to force all my users to migrate immediately. Alternatively, I would like to support 2 interfaces, maybe to ease dropping my CE in as a replacement for some other CE. So I present inner parts under multiple names.

That said, I think that's a valid but minority use case, I expect forwarding both b and c as a would be more common.

To be clear, I'm not confused by the arrow, I just find the order confusing. I expect the first thing to the thing which is being created/defined (a part name that will be visible to the outside) and the second to be the definition. I think I would prefer that regardless of whether it's "a=>b", "a=b" or just "a b". Then, the partmap attribute just looks a lot like a serialized dictionary and maybe "a: b, ..." and "a : b c, ..." would actually be most natural since it's almost JSON.

So 3 things
1 Decide on "a b", "a=>b", "a: b" and I don't have a strong opinion on that although I think I would prefer to have something beyond whitespace and comma in there.
2 Unless I'm missing an argument for "b=>a", I'd propose making it "a=>b" to add «[ a → partMap[b] ]» to the part map.
3 Allow "a => b c" to add «[ a → partMap[b] ]» and «[ a → partMap[c] ]» to the part map.

@rniwa
Copy link

rniwa commented Mar 26, 2018

To be clear, I'm not confused by the arrow, I just find the order confusing.

If the arrow doesn't tell you the ordering, what the heck is the point of having the arrow at all?

I'd object to using => unless there is a strong reason we ought to (and I see none) because it would make the parsing harder and adds unnecessary micro-syntax.

@tabatkins
Copy link
Member

Again, the parsing isn't any harder. This is seriously a non-issue, parsing-wise. Whether or not to use any sort of separator token is purely a matter of making it easier to use/understand. "We don't need it" is a reasonable argument against; "this is hard to parse" is not.

The reasoning behind the arrow is that it tells you the direction of the forwarding/name change. In a => b, it indicates that the part named "a" becomes the part named "b" in the outer scope. However, I understand that in maps the direction is usually the other way; a: b would indicate that in your partmap, the part named a is equivalent to the subpart b.

I personally find a b, c d completely opaque, on the other hand. There's zero indication of what's happening there.

Perhaps a as b, c as d? The English connector suggests the meaning a little bit more, and has a clearer direction of association.

What concrete use cases would need to do that?

A part, in general, can be given multiple names, and multiple parts can share the same name. (The semantics of a part-name are equivalent to a class.) Exposing the same functionality in the part forwarding is thus necessary; it's what you could do if you were directly annotating the sub-parts as your own children.

@rniwa
Copy link

rniwa commented Mar 26, 2018

Again, the parsing isn't any harder. This is seriously a non-issue, parsing-wise. Whether or not to use any sort of separator token is purely a matter of making it easier to use/understand. "We don't need it" is a reasonable argument against; "this is hard to parse" is not.

We can continue to agree to disagree on that point.

Perhaps a as b, c as d? The English connector suggests the meaning a little bit more, and has a clearer direction of association.

That has the same semantic ambiguity. One can't tell whether a (in a subcomponent) is exposed as b in the host component, or b in the host component is mapped as b in a subcomponent.

@tabatkins
Copy link
Member

We can continue to agree to disagree on that point.

We really can't. I know how parsing code works; I've written bucket-loads of it. It's an insignificant addition to parsing.

That has the same semantic ambiguity.

Hmm, true.

@rniwa
Copy link

rniwa commented Mar 26, 2018

We can continue to agree to disagree on that point.
We really can't. I know how parsing code works; I've written bucket-loads of it. It's an insignificant addition to parsing.

Are you implying that I don't understand parsing? Like you, I've written many parsers in C/C++, and in particular, I'm very familiar with how DOMTokenList's parser works, and adding this random syntax would make that implementation significantly slower. Having any kind of multi-character token like => is highly undesirable in implementing a high performance parser.

Anyhow, another thing we need to decide is how this attribute is represented in DOM. Presumably, we'd want to add element.partMap which doesn't simply return a string. If we're using DOMTokenList, then we'd hit the aforementioned performance issue. If we're using something else, then we better have a good argument for introducing a yet another DOM class, and design it along with the format for the content attribute.

@tabatkins
Copy link
Member

"It would be good for this be high-performance, and the extra effort involved in parsing the separate token would interfere with that" is a reasonable argument! It's different from "the separator token is hard to parse", tho, which is false. ^_^

@rniwa
Copy link

rniwa commented Mar 26, 2018

When I say "the separator token is hard to parse" is synonymous to not being able to implement this without regressing perf since we wouldn't implement any feature if it interferes with perf.

Also, having to just do value.split(/\s*,\s*/).map((t) => t.split(/\s+/)) versus having to deal with => because now you'd have to deal with more complicate error handling as just having = and >, etc... is a significant difference in the complexity.

@tabatkins
Copy link
Member

Like I said, I'm fine with "it's slower to parse". That's a reasonable argument against having such a separator.

@fergald
Copy link
Contributor Author

fergald commented Mar 27, 2018

If the arrow doesn't tell you the ordering, what the heck is the point of having the arrow at all?

There's 2 thing heres, a visual separator and the ordering.

Ordering first. The spec talks about the part map. Developers will have this map as a their mental model but in the current spec, have "a => b" meaning partmap["b"] = "a". This tripped me up first time I tried I tried to produce an example and I suspect it trips others up, it's basically "value => key". I would argue we should have "key => value"

The second thing is whether it should be "key => value" or just "key value" or "key value value value" (I don't care about "=>" vs ":" but I'm going to stick with "=>" for examples). I'd argue that from a developer ergonomics POV, "key value1 value2 value2" is bad. It looks like a uniform list of tokens, there's is nothing to distinguish key from values. It's unclear that "a b" is different from "b a" and (if we allow it) that "a b c" and "b a c" have completely different meanings but that "a c b" and "a b c" are identical. "a => b c" makes it very clear that "a" is the key and "b" and "c" are a set of values.

So the question is not whether there is a parsing cost but whether it is worth the ergonomic benefit. If we used "a: b c" (so single-char instead of multi-char) it's hard to imagine that the parsing difference is going to be significant. Would it be crazy to make the : optional for those who want it?

@fergald
Copy link
Contributor Author

fergald commented Mar 27, 2018

Anyhow, another thing we need to decide is how this attribute is represented in DOM. Presumably, we'd want to add element.partMap which doesn't simply return a string. If we're using DOMTokenList, then we'd hit the aforementioned performance issue. If we're using something else, then we better have a good argument for introducing a yet another DOM class, and design it along with the format for the content attribute.

I don't see how we could use DOMTokenList since this is a map. Here is a proposed change to the spec to add IDL. Very happy to have comments on that.

@rniwa
Copy link

rniwa commented Mar 27, 2018

I don't think we should add a shorthand for mapping multiple parts in a subcomponent to a single part. I also disagree that having => would improve the developer ergonomics because of the semantics ambiguity.

@fergald
Copy link
Contributor Author

fergald commented Mar 27, 2018

Happy to leave out the shorthand, although if we find people are doing it often, we should consider it.

What do you mean by "semantics ambiguity"?

Also do you have an opinion on whether "a b" should mean

  • partmap["a"].add("b")
  • partmap["b"].add("a")

@rniwa
Copy link

rniwa commented Mar 27, 2018

I'm not certain either API makes sense. What you're adding is a relationship so partmap.add({part: 'a', subpart: 'b'}) or partmap.add('a', 'b') makes more sense to me. Similarly, partmap.remove({part: 'a'}) or partmap.remove('a', 'b').

In your model, how do you delete a part in the map? delete partmap['a'] or partmap.remove('a')? If it's latter, do we require developers to explicitly add a part by partmap.add('a') before accessing partmap['a']?

@fergald
Copy link
Contributor Author

fergald commented Mar 27, 2018

In my model the partmap is a dictionary where the values are Set()s of part names. So deleting would be partmap['a'].delete('b') and to remove "a" entirely, delete partmap['a'] (although partmap['a'].clear() would have the same effect).

What's the tradeoff between a dictionary of sets and something with an explicit API. Are there operations that dictionaries/sets allow that we wouldn't want or conversely are there operations dictionaries/sets don't allow that we would want and couldn't add later without breaking compatibility? I don't really want to write up an API for something that behaves essentially as a dictionary of sets if I can avoid it.

@rniwa
Copy link

rniwa commented Mar 27, 2018

Well, browser engines can't observe changes to a dictionary so your proposal as currently stands will make partmap readonly, and any changes to it will be discarded. It's even weirder because it has [SameObject] on it. It means the browser engine has to update the dictionary continuously? I don't think it works because then author scripts can override property descriptors of a dictionary, and all sorts of insanity ensues.

@fergald
Copy link
Contributor Author

fergald commented Mar 27, 2018

The programmatic API is not blocking anything right now, can we move discussion of that to #2414 and get back to the forwarding syntax here?

Do you have an opinion on whether

To expose the span for styling, should that be partmap="a b" or partmap="b a"? I would suggest that given that the model in the spec and the name is of a map, it seems like the key ("a") should come first.

@rniwa
Copy link

rniwa commented Mar 27, 2018

The programmatic API is not blocking anything right now, can we move discussion of that to #2414 and get back to the forwarding syntax here?

I don't think we can reach a consensus on content attribute syntax without coming to a consensus on IDL attribute. I certainly wouldn't support any move to reach a consensus without also having a concrete proposal & a rough consensus on IDL attribute since these things are interdependent, and the past attempts to define one without the other usually resulted in pretty terrible outcomes.

My preference here is to subclass DOMTokenList and overload add and remove with a variant which takes multiple arguments and/or a dictionary. That would make the whole thing work & consistent with the rest of the DOM.

Perhaps the problem here is the content attribute name itself. Maybe we can come up with a name which makes the semantics of what maps to what clear. e.g. if it were partretargeting="a b" or partdelegates="a b", or partforwarding="a b" (this one is a bit ambiguous), it becomes self evident that a refers to a part of the current tree, and b refers to a part inside the element on which this attribute is specified. I'm sure someone can come up with an even better name though.

@fergald
Copy link
Contributor Author

fergald commented Mar 28, 2018

I think there is some miscommunication here

Well, browser engines can't observe changes to a dictionary so your proposal as currently stands will make partmap readonly, and any changes to it will be discarded. It's even weirder because it has [SameObject] on it. It means the browser engine has to update the dictionary continuously? I don't think it works because then author scripts can override property descriptors of a dictionary, and all sorts of insanity ensues.

There is a model described in the spec which maps part names to elements. This mapping of names to elements is conceptual only, it's not what devs will interact with and it's not how blink will implement this either. No one is proposing to expose this model to devs. This would require dynamic updates and a whole load of other problems but it is not relevant, except as a way of specifying the behaviour of the feature.

The model that devs need to consider is one that maps, let's call them, "outer" part names to "inner" part names and it is simply a map from string to set-of-strings. It does not change dynamically.

If we need to create an new class because browsers can't observe changes to a dictionary, that's fine.

My preference here is to subclass DOMTokenList and overload add and remove with a variant which takes multiple arguments and/or a dictionary. That would make the whole thing work & consistent with the rest of the DOM.

I don't think it's useful to base a map on DOMTokenList. It's not a list of tokens, it's a comma separated list of pairs of tokens. I think we would have to override every method of DOMTokenList.

Having the values of the map be DOMTokenList seems reasonable. Since we hope to support things like "partname-*", I'm not sure if that really fits with DOMTokenList but it looks like space is the only character not allow, so actually this seems fine.

Here's what I propose - I have a few pull requests out for the spec. I will assemble them and add some more stuff (a clear dev-facing model for the part map) and publish that somewhere and get agreement on the shape of that.

I'm happy to VC about this too to clarify any points but maybe that's best left until I publish an updated version.

@rniwa
Copy link

rniwa commented Mar 28, 2018

I don't think it's helpful to have pull requests until we can agree on the basic syntax & IDL attribute first. It's a lot harder to judge the proposal based on a PR compared to looking at a snippet of code.

@fergald
Copy link
Contributor Author

fergald commented Mar 28, 2018 via email

@fergald
Copy link
Contributor Author

fergald commented Mar 31, 2018

https://fergald.github.io/csswg-drafts/rendered/merged/css-shadow-parts-1/Overview.html

contains a rendered version of spec with the following changes:

  • describes:
    • part name list
    • part name map
    • part element map (previously in the spec, the other 2 were not)
  • attributes are no longer described as modified part element map, instead they modify part name list and part name map and I include an algorithm for (re)calculating part element map from these 2 and the current DOM
  • tries to make it clear that part element map is for spec purpose only
  • IDL for partList and partMap. I imagine this still needs work but I don't know what, so please be specific about changes you'd like to see to this

@rniwa
Copy link

rniwa commented Apr 1, 2018

Again, I don't think we should have => in the map. Also, we should distinguish content attribute and IDL attribute explicitly. It's totally unclear which one that spec is talking about.

@fergald
Copy link
Contributor Author

fergald commented Apr 1, 2018 via email

@rniwa
Copy link

rniwa commented Apr 1, 2018

See, for example, the definition of style attribute.

@fergald
Copy link
Contributor Author

fergald commented Sep 27, 2018

For the purposes of shipping a first version of this I think comma-separated token/token-pair is fine. So I'm going to close this issue.

@fergald
Copy link
Contributor Author

fergald commented Oct 2, 2018

Based on discussions in #2368, settling on "foo1 : bar1, foo2".

@fergald fergald closed this as completed Nov 8, 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

3 participants