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] fully specify the parser for the forwarding micro-syntax #2412

Open
fergald opened this issue Mar 7, 2018 · 15 comments
Open

Comments

@fergald
Copy link
Contributor

fergald commented Mar 7, 2018

As discussed at Web Components F2F (2018-03, Tokyo) and in TAG issue update spec to include a fully specified parser.

@rniwa
Copy link

rniwa commented Jul 6, 2018

Isn't this a duplicate of #2411 ?

@fergald
Copy link
Contributor Author

fergald commented Jul 9, 2018

Not exactly. This one is for me to add a section to the spec with a grammar, it's just not worth doing until we have clearer agreement on #2411

@fergald
Copy link
Contributor Author

fergald commented Nov 9, 2018

13add36 adds a fully specified parser. I'd appreciate people sanity checking it. A couple of points:

  • I chose strict over lenient, e.g. "foo: bar buz" will result in nothing at all. An alternative would be to make "foo:bar buz" give you the same result as "foo: bar" and just ignore the buz. I don't have a strong opinion on which it should be, my preference is strictness but browsers tend to be very lenient. The downside of being lenient is that if we decide to assign a meaning to "foo: bar buz" in the future, bad things may happen to pages that were working only because of lenience...
  • I followed the parser specs in the HTML spec. There are certain things defined in there like "collect a sequence of code points" which aren't defined in this spec. I'm not sure what the correct approach is to fix this, just reference the HTML doc?

@tabatkins

@annevk
Copy link
Member

annevk commented Nov 9, 2018

Yes, you want to define this in terms of https://infra.spec.whatwg.org/ (Infra, not HTML). Since you're using Bikeshed you need to use <a> to get them to link, not <span>.

Strict is good, but it seems you're not totally strict? Validity requires no whitespace, but you are stripping whitespace.

You need to be clearer about what "space characters" means. I suspect you want to reference the ASCII whitespace definition.

@fergald
Copy link
Contributor Author

fergald commented Nov 9, 2018

I think strict for everything except inter-token whitespace is what I wanted and matches most (all?) of the parsers in the HTML spec. I will update the links. Thanks.

@annevk
Copy link
Member

annevk commented Nov 9, 2018

Yeah, but then you should incorporate that in the definition of what's valid and allow ASCII whitespace there.

@annevk
Copy link
Member

annevk commented Nov 9, 2018

By the way, does this attribute only apply to HTML elements or to all elements? I guess only to HTML elements since shadow trees are restricted to them?

@annevk
Copy link
Member

annevk commented Nov 9, 2018

Sorry for all the short thoughts, one other thing I think we want here is to eventually move this attribute definition into the HTML Standard.

@fergald
Copy link
Contributor Author

fergald commented Nov 9, 2018

@annevk

Yeah, but then you should incorporate that in the definition of what's valid and allow ASCII whitespace there.

Makes total sense to me but e.g. look at Lists of floating-point numbers
where the "valid" definition eplicitly rules out whitespace and the parser copes with it. I assumed that the "valid" one was actually specifying the canonical/serialized verison rather than the only valid format (since other examples in the spec are similarly divergent). I'm happy to go either way, any advice appreciated.

@annevk
Copy link
Member

annevk commented Nov 9, 2018

Valid is what is conforming for developers to write and what a validator would complain about. It has no effect on serialization (or parsing). I think HTML varies in where whitespace is allowed and examples in the HTML standard vary as to whether they are valid or not (there's some invalid examples to make a point). Here it probably makes sense to allow whitespace given the examples written thus far.

@fergald
Copy link
Contributor Author

fergald commented Nov 9, 2018

Yeah, I'll change this spec to clearly allow whitespace.

But just on the topic of the HTML, I'm not talking about examples, the spec itself is contradictory from one paragraph to the next E.g.

A valid list of floating-point numbers is a number of valid floating-point numbers separated by U+002C COMMA characters, with no other characters (e.g. no ASCII whitespace). In addition, there might be restrictions on the number of floating-point numbers that can be given, or on the range of values allowed.

but then

  1. Collect a sequence of code points that are ASCII whitespace, U+002C COMMA, or U+003B SEMICOLON characters from input given position. This skips past any leading delimiters.
    and

5.6 Collect a sequence of code points that are ASCII whitespace, U+002C COMMA, or U+003B SEMICOLON characters from input given position. This skips past the delimiter.

So the parser for list of floats does not match the definition of "valid". Maybe I just picked a bad example to base my one on.

@tabatkins
Copy link
Member

Yeah, what you call "valid" is just "what should a validator check to see if it should complain about this?". It doesn't necessarily have any connection to what's actually accepted; there's often a lot of constraints there that make the set of accepted things much wider than the set of valid things.

In this case, I think it's perfectly reasonable and good to have whitespace in your part mapping; I would be annoyed if a validator complained at me for it. So it should be included in the definition of validity.

(Reviewing the spec now.)

@tabatkins
Copy link
Member

K, and reviewed. Lots of comments on the commit itself.

@fergald
Copy link
Contributor Author

fergald commented Nov 12, 2018

@tabatkins Thanks. a0ec26c addresses lots of your comments.

Still to do

  • rewrite in terms of split a string input on commas
  • better definitions of "valid"

@fergald
Copy link
Contributor Author

fergald commented Nov 13, 2018

One other commit that I didn't tag is

b7893dc
[css-shadow-parts-1] fix auto-select for list refs]

PTAL

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