Preserve invalid selectors in :is() and :where() [chromium/src : main]
Byungwoo Lee (Gerrit)
Byungwoo Lee added 1 comment![]( "Open in Gerrit")
Hello Rune,
This is the CL preserving invalid selectors in :is and :where().
Could you please take a look? Thank you!
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
sesse@ had some relevant questions here:
https://github.com/w3c/csswg-drafts/issues/8356#issuecomment-1782860394
Additionally, what about case-sensitivity?
The issues is still marked as NeedsEdits. This is still not covered by the spec?
Related details
- Byungwoo Lee
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
kPseudoUnparsedContainingNesting,Why do these need to be two pseudo ids? Isn't different CSSNestingType enough?
Byungwoo Lee (Gerrit)
Byungwoo Lee added 3 comments![]( "Open in Gerrit")
sesse@ had some relevant questions here:
https://github.com/w3c/csswg-drafts/issues/8356#issuecomment-1782860394
Additionally, what about case-sensitivity?
The issues is still marked as NeedsEdits. This is still not covered by the spec?
I posted a comment to the issue (https://github.com/w3c/csswg-drafts/issues/8356#issuecomment-5042919620), and updated the CL accordingly.
Additionally, what about case-sensitivity?
I cannot clearly understand this question. Could you please share some more context or some examples?
Thanks for the review! I addressed and replied the comments. Could you please take a look?
Why do these need to be two pseudo ids? Isn't different CSSNestingType enough?
Yes it is correct. Using `kPseudoUnparsed` with `kNone` nesting type is enough. I fixed this. Thank you!
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
Byungwoo Leesesse@ had some relevant questions here:
https://github.com/w3c/csswg-drafts/issues/8356#issuecomment-1782860394
Additionally, what about case-sensitivity?
The issues is still marked as NeedsEdits. This is still not covered by the spec?
I posted a comment to the issue (https://github.com/w3c/csswg-drafts/issues/8356#issuecomment-5042919620), and updated the CL accordingly.
Additionally, what about case-sensitivity?
I cannot clearly understand this question. Could you please share some more context or some examples?
For instance, the first sub-selector here is invalid, but all simple selectors are known:
```
:is(::before::part(my-part):HOVER , div) {}
```
Do we lower-case :HOVER in the serialization?
Also, do we strip trailing white-space?
Related details
- Byungwoo Lee
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Byungwoo Lee (Gerrit)
Byungwoo Lee voted and added 1 comment![]( "Open in Gerrit")
Votes added by Byungwoo Lee
| Commit-Queue | +0 |
Byungwoo Leesesse@ had some relevant questions here:
https://github.com/w3c/csswg-drafts/issues/8356#issuecomment-1782860394
Additionally, what about case-sensitivity?
The issues is still marked as NeedsEdits. This is still not covered by the spec?
Rune LillesveenI posted a comment to the issue (https://github.com/w3c/csswg-drafts/issues/8356#issuecomment-5042919620), and updated the CL accordingly.
Additionally, what about case-sensitivity?
I cannot clearly understand this question. Could you please share some more context or some examples?
For instance, the first sub-selector here is invalid, but all simple selectors are known:
```
:is(::before::part(my-part):HOVER , div) {}
```Do we lower-case :HOVER in the serialization?
Also, do we strip trailing white-space?
Thank you for the example!
do we strip trailing white-space?
Yes. Patchset 8 didn't do this, but I have uploaded Patchset 9 to strip trailing whitespaces. AFAIK, trailing spaces are just ignorable whitespaces since it doesn't act as a descendant combinator or other selector token. So, stripping them is looks reasonable.
Thank you for pointing this!
Do we lower-case :HOVER in the serialization?
No we do not lowercase :HOVER in this scenario.
I think that trying to selectively lowercase tokens like :HOVER inside an invalid selector introduces unnecessary complexity and parsing overhead since we exit parsing early when we detect failure. Consuming following tokens and handling edge cases looks not necessary only for serialization.
I also added the case to the unittest.
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 5 comments![]( "Open in Gerrit")
This needs to be specified in the spec before shipping, right?
}I don't understand what's going on here. What this invalid_list and subpos handling for?
SerializeInvalidSelectorsInForgivingSelectorListEnabled()) {I'm a bit worried this codepath now goes untested (since this is an experimental feature and we ship something else). It's different enough from the previous implementation.
bool ends_with_empty_selector = true;This needs documentation. It's really hard to understand what's going on in this function and what these variables mean.
ParseSelector(":is(::before:HOVER , .a)");Could these be wpt tests instead to make sure browsers align on this behavior?
Related details
- Byungwoo Lee
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Byungwoo Lee (Gerrit)
Byungwoo Lee added 5 comments![]( "Open in Gerrit")
Yes right. I'll follow up the csswg issue.
I don't understand what's going on here. What this invalid_list and subpos handling for?
Ah, right. this looks confusing. This is for marking invalid list, and I think I can do that later by iterating consumed selectors. Will fix it. Thank you!
SerializeInvalidSelectorsInForgivingSelectorListEnabled()) {I'm a bit worried this codepath now goes untested (since this is an experimental feature and we ship something else). It's different enough from the previous implementation.
I think that the unit test I added covers all code paths including serialization itself, feature variation (enabled/disabled), selector list iteration (normal iteration, new iteration including unparsed invalid), and invalid list marking.
I can see that this path is already covered by the test (from coverage report and manual test with some logs)
bool ends_with_empty_selector = true;This needs documentation. It's really hard to understand what's going on in this function and what these variables mean.
Sorry for the confusion. I'll remove the position and invalid_list to make the code more clear, and add a comment for the `ends_with_empty_selector`.
FYI, `ends_with_empty_selector` represents that the selector list is empty text (e.g. `:is()`) or ends with comma (e.g. `:is(.a,)`). For both cases, we need to add unparsed-invalid for the empty text.
ParseSelector(":is(::before:HOVER , .a)");Could these be wpt tests instead to make sure browsers align on this behavior?
Yes, of course. I'll add it.
Byungwoo Lee (Gerrit)
Byungwoo Lee added 4 comments![]( "Open in Gerrit")
Addressed some of the comments, and added new changes about CSSSelector::Empty() condition. (I updated the commit message as well).
Could you please take another look? Thank you for your review!
Byungwoo LeeI don't understand what's going on here. What this invalid_list and subpos handling for?
Ah, right. this looks confusing. This is for marking invalid list, and I think I can do that later by iterating consumed selectors. Will fix it. Thank you!
I made `MarkAsInvalidListIfNeeded()` method and removed those.
Byungwoo LeeThis needs documentation. It's really hard to understand what's going on in this function and what these variables mean.
Sorry for the confusion. I'll remove the position and invalid_list to make the code more clear, and add a comment for the `ends_with_empty_selector`.
FYI, `ends_with_empty_selector` represents that the selector list is empty text (e.g. `:is()`) or ends with comma (e.g. `:is(.a,)`). For both cases, we need to add unparsed-invalid for the empty text.
Renamed the variable to `has_trailing_empty_argument` and added a comment.
Byungwoo LeeCould these be wpt tests instead to make sure browsers align on this behavior?
Yes, of course. I'll add it.
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen voted and added 3 comments![]( "Open in Gerrit")
Votes added by Rune Lillesveen
| Code-Review | +1 |
Acknowledged
lgtm
SerializeInvalidSelectorsInForgivingSelectorListEnabled()) {I'm a bit worried this codepath now goes untested (since this is an experimental feature and we ship something else). It's different enough from the previous implementation.
I think that the unit test I added covers all code paths including serialization itself, feature variation (enabled/disabled), selector list iteration (normal iteration, new iteration including unparsed invalid), and invalid list marking.
I can see that this path is already covered by the test (from coverage report and manual test with some logs)
- Byungwoo Lee
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/61514.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Byungwoo Lee
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Byungwoo Lee (Gerrit)
Byungwoo Lee voted and added 1 comment![]( "Open in Gerrit")
Votes added by Byungwoo Lee
| Commit-Queue | +2 |
1 comment
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |