Support processing instructions in the HTML parser [chromium/src : main]
Noam Rosenthal (Gerrit)
Message from Noam Rosenthal![]( "Open in Gerrit")
Set Ready For Review
Related details
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Jägenstedt (Gerrit)
Philip Jägenstedt added 5 comments![]( "Open in Gerrit")
Currently trying to figure out how to handle xml* and lit$ targets without unbounded buffering of both original + lowercased data. But I think we can land something and then update it.
// TODO(nrosenthal): allow the full unicode range of ID_Start?Consensus says no: https://github.com/whatwg/html/pull/12118#issuecomment-3847775588
token_.BeginProcessingInstruction();I made an editorial change to the spec to not create a PI token that we might later discard, which I also think made the handling of "xml" and "xml-stylesheet" easier to follow. This step should just be to clear the temp buffer and reconsume (below).
However, I now see that both spec and implementation only use the temp buffer to compare to the lowercase string "script", and doesn't use the buffer to "back up" and get the original characters as I've attempted.
I'm leaning towards changing the spec again to not use the temp buffer in this novel way, WDYT?
ParseError();Does it matter what character we're at when this is called, does this end up pointing to a specific location in DevTools or something?
I ask because the right location to point to for the "xml" and "xml-stylesheet" cases are probably the first "x" but we're way past that when we can see the error.
token_.Clear();I see that `token_.Clear()` is never called outside of HTMLTokenizer::Reset() before this change, just like the spec never said "discard the token".
Related details
- Noam Rosenthal
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. |
Philip Jägenstedt (Gerrit)
Philip Jägenstedt added 6 comments![]( "Open in Gerrit")
EXPECT_EQ("<?foo ba|r ?>", SetAndGetSelectionText("<?foo ba|r ?>"))I guess this is in an XML document or https://chromium-review.googlesource.com/c/chromium/src/+/7531667 would have failed this test.
String processing_instruction_data_;Why can't this reuse data_? We don't need to use both at the same time, right?
TEST_P(HTMLDocumentParserTest, ProcessingInstructionNoQuestionMark) {Do you think these tests should be kept fairly minimal, or what goes here vs. WPT? Do these tests get the code coverage to 100%?
processing_instruction_test_equivalent("<?hey there?>", "<?hey there>");A few more processing_instruction_test_equivalent would be:
`<?hey?there>` equivalent to `<?hey ?there>` (with > or ?> as the closing syntax, doesn't matter)
`<?HEY THERE>` equivalent to `<?hey THERE>`.
processing_instruction_test("<?something ? >", [Can you also add a more basic trailing whitespace test like `<?something a >` to show that all trailing whitespace is preserved, not just following `?`?
"price$value",Throw in lit$123456789 in a compat category?
Related details
- Noam Rosenthal
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. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 10 comments![]( "Open in Gerrit")
EXPECT_EQ("<?foo ba|r ?>", SetAndGetSelectionText("<?foo ba|r ?>"))I guess this is in an XML document or https://chromium-review.googlesource.com/c/chromium/src/+/7531667 would have failed this test.
Acknowledged
String processing_instruction_data_;Why can't this reuse data_? We don't need to use both at the same time, right?
data_ is used for the target, as we can't use "name" since it has all kinds of special rules.
TEST_P(HTMLDocumentParserTest, ProcessingInstructionNoQuestionMark) {Do you think these tests should be kept fairly minimal, or what goes here vs. WPT? Do these tests get the code coverage to 100%?
Agreed, it was more about duplicating what what already there. Kept only one test for this.
// TODO(nrosenthal): allow the full unicode range of ID_Start?Consensus says no: https://github.com/whatwg/html/pull/12118#issuecomment-3847775588
Acknowledged
token_.BeginProcessingInstruction();I made an editorial change to the spec to not create a PI token that we might later discard, which I also think made the handling of "xml" and "xml-stylesheet" easier to follow. This step should just be to clear the temp buffer and reconsume (below).
However, I now see that both spec and implementation only use the temp buffer to compare to the lowercase string "script", and doesn't use the buffer to "back up" and get the original characters as I've attempted.
I'm leaning towards changing the spec again to not use the temp buffer in this novel way, WDYT?
Seems like that's what you've done?
ParseError();Does it matter what character we're at when this is called, does this end up pointing to a specific location in DevTools or something?
I ask because the right location to point to for the "xml" and "xml-stylesheet" cases are probably the first "x" but we're way past that when we can see the error.
I don't think we do anything with ParseError.
token_.Clear();Noam RosenthalI see that `token_.Clear()` is never called outside of HTMLTokenizer::Reset() before this change, just like the spec never said "discard the token".
Yea but without that we have an initialized token and we call "BeginComment" and assert. Should I not have initialized the "ProcessingInstruction" state until we know we have a valid target?
processing_instruction_test_equivalent("<?hey there?>", "<?hey there>");A few more processing_instruction_test_equivalent would be:
`<?hey?there>` equivalent to `<?hey ?there>` (with > or ?> as the closing syntax, doesn't matter)
`<?HEY THERE>` equivalent to `<?hey THERE>`.
Done the first.
I think the second one is no longer valid?
Can you also add a more basic trailing whitespace test like `<?something a >` to show that all trailing whitespace is preserved, not just following `?`?
Done
Throw in lit$123456789 in a compat category?
- Philip Jägenstedt
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. |
Philip Jägenstedt (Gerrit)
Philip Jägenstedt added 5 comments![]( "Open in Gerrit")
String processing_instruction_data_;Noam RosenthalWhy can't this reuse data_? We don't need to use both at the same time, right?
data_ is used for the target, as we can't use "name" since it has all kinds of special rules.
I see, the name confused me. Will we still need this if we create a PI token early and just add to its target data?
token_.BeginProcessingInstruction();Noam RosenthalI made an editorial change to the spec to not create a PI token that we might later discard, which I also think made the handling of "xml" and "xml-stylesheet" easier to follow. This step should just be to clear the temp buffer and reconsume (below).
However, I now see that both spec and implementation only use the temp buffer to compare to the lowercase string "script", and doesn't use the buffer to "back up" and get the original characters as I've attempted.
I'm leaning towards changing the spec again to not use the temp buffer in this novel way, WDYT?
Seems like that's what you've done?
Yes, Henri thought we should just make target case-sensitive, so I did that. Now the spec creates a PI token as soon as seeing <? and if there's an error the target is read back and turned into a comment token instead. There's no observable difference between this and keeping a buffer until we know what token type it will be, but it made things more readable IMHO.
ParseError();Noam RosenthalDoes it matter what character we're at when this is called, does this end up pointing to a specific location in DevTools or something?
I ask because the right location to point to for the "xml" and "xml-stylesheet" cases are probably the first "x" but we're way past that when we can see the error.
I don't think we do anything with ParseError.
That's what I thought.
token_.Clear();Noam RosenthalI see that `token_.Clear()` is never called outside of HTMLTokenizer::Reset() before this change, just like the spec never said "discard the token".
Yea but without that we have an initialized token and we call "BeginComment" and assert. Should I not have initialized the "ProcessingInstruction" state until we know we have a valid target?
That's one option, but in the end I made the spec throw away a token instead, just like you're doing here.
processing_instruction_test_equivalent("<?hey there?>", "<?hey there>");Noam RosenthalA few more processing_instruction_test_equivalent would be:
`<?hey?there>` equivalent to `<?hey ?there>` (with > or ?> as the closing syntax, doesn't matter)
`<?HEY THERE>` equivalent to `<?hey THERE>`.
Done the first.
I think the second one is no longer valid?
- Noam Rosenthal
- Philip Jägenstedt
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. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 2 comments![]( "Open in Gerrit")
String processing_instruction_data_;Noam RosenthalWhy can't this reuse data_? We don't need to use both at the same time, right?
Philip Jägenstedtdata_ is used for the target, as we can't use "name" since it has all kinds of special rules.
I see, the name confused me. Will we still need this if we create a PI token early and just add to its target data?
This is AtomicHTMLToken, it's only created when we emit the valid token to the parser.
I changed it to "processing_instruction_target_" instead, is that clearer?
token_.BeginProcessingInstruction();Noam RosenthalI made an editorial change to the spec to not create a PI token that we might later discard, which I also think made the handling of "xml" and "xml-stylesheet" easier to follow. This step should just be to clear the temp buffer and reconsume (below).
However, I now see that both spec and implementation only use the temp buffer to compare to the lowercase string "script", and doesn't use the buffer to "back up" and get the original characters as I've attempted.
I'm leaning towards changing the spec again to not use the temp buffer in this novel way, WDYT?
Philip JägenstedtSeems like that's what you've done?
Yes, Henri thought we should just make target case-sensitive, so I did that. Now the spec creates a PI token as soon as seeing <? and if there's an error the target is read back and turned into a comment token instead. There's no observable difference between this and keeping a buffer until we know what token type it will be, but it made things more readable IMHO.
- Philip Jägenstedt
- Philip Jägenstedt
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. |
Philip Jägenstedt (Gerrit)
Philip Jägenstedt added 1 comment![]( "Open in Gerrit")
String processing_instruction_data_;Noam RosenthalWhy can't this reuse data_? We don't need to use both at the same time, right?
Philip Jägenstedtdata_ is used for the target, as we can't use "name" since it has all kinds of special rules.
Noam RosenthalI see, the name confused me. Will we still need this if we create a PI token early and just add to its target data?
This is AtomicHTMLToken, it's only created when we emit the valid token to the parser.
I changed it to "processing_instruction_target_" instead, is that clearer?
Yep, that's clear. Just checking, now that the target can only be alphanumeric and hyphens, does name_ still have special rules that prevent reuse?
Related details
- Noam Rosenthal
- Philip Jägenstedt
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. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
String processing_instruction_data_;Noam RosenthalWhy can't this reuse data_? We don't need to use both at the same time, right?
Philip Jägenstedtdata_ is used for the target, as we can't use "name" since it has all kinds of special rules.
Noam RosenthalI see, the name confused me. Will we still need this if we create a PI token early and just add to its target data?
Philip JägenstedtThis is AtomicHTMLToken, it's only created when we emit the valid token to the parser.
I changed it to "processing_instruction_target_" instead, is that clearer?
Yep, that's clear. Just checking, now that the target can only be alphanumeric and hyphens, does name_ still have special rules that prevent reuse?
- Philip Jägenstedt
- Philip Jägenstedt
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. |
Philip Jägenstedt (Gerrit)
Philip Jägenstedt added 1 comment![]( "Open in Gerrit")
String processing_instruction_data_;Noam RosenthalWhy can't this reuse data_? We don't need to use both at the same time, right?
Philip Jägenstedtdata_ is used for the target, as we can't use "name" since it has all kinds of special rules.
Noam RosenthalI see, the name confused me. Will we still need this if we create a PI token early and just add to its target data?
Philip JägenstedtThis is AtomicHTMLToken, it's only created when we emit the valid token to the parser.
I changed it to "processing_instruction_target_" instead, is that clearer?
Noam RosenthalYep, that's clear. Just checking, now that the target can only be alphanumeric and hyphens, does name_ still have special rules that prevent reuse?
Yea, it needs to be an HTML element name.
- Noam Rosenthal
- Philip Jägenstedt
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. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
String processing_instruction_data_;Noam RosenthalWhy can't this reuse data_? We don't need to use both at the same time, right?
Philip Jägenstedtdata_ is used for the target, as we can't use "name" since it has all kinds of special rules.
Noam RosenthalI see, the name confused me. Will we still need this if we create a PI token early and just add to its target data?
Philip JägenstedtThis is AtomicHTMLToken, it's only created when we emit the valid token to the parser.
I changed it to "processing_instruction_target_" instead, is that clearer?
Noam RosenthalYep, that's clear. Just checking, now that the target can only be alphanumeric and hyphens, does name_ still have special rules that prevent reuse?
Philip JägenstedtYea, it needs to be an HTML element name.
Is something else used for <my-button> then?
It gets the name "unknown" and then an additional string of "my-button". I find it a bit odd. Lots of asserts... we can use it awkwardly and say that it's an "unknown" with the target as the "real" name.
Related details
- Philip Jägenstedt
- Philip Jägenstedt
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. |
Philip Jägenstedt (Gerrit)
Philip Jägenstedt voted and added 2 comments![]( "Open in Gerrit")
Votes added by Philip Jägenstedt
| Code-Review | +1 |
2 comments
String processing_instruction_data_;Noam RosenthalWhy can't this reuse data_? We don't need to use both at the same time, right?
Philip Jägenstedtdata_ is used for the target, as we can't use "name" since it has all kinds of special rules.
Noam RosenthalI see, the name confused me. Will we still need this if we create a PI token early and just add to its target data?
Philip JägenstedtThis is AtomicHTMLToken, it's only created when we emit the valid token to the parser.
I changed it to "processing_instruction_target_" instead, is that clearer?
Noam RosenthalYep, that's clear. Just checking, now that the target can only be alphanumeric and hyphens, does name_ still have special rules that prevent reuse?
Philip JägenstedtYea, it needs to be an HTML element name.
Noam RosenthalIs something else used for <my-button> then?
It gets the name "unknown" and then an additional string of "my-button". I find it a bit odd. Lots of asserts... we can use it awkwardly and say that it's an "unknown" with the target as the "real" name.
OK, let's not 😊
const AtomicString target_lower = target.AsAtomicString().LowerASCII();This works, but can't we use a ASCII case-insensitive compare instead of lowercasing the target?
Related details
- Noam Rosenthal
- Philip Jägenstedt
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. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
const AtomicString target_lower = target.AsAtomicString().LowerASCII();This works, but can't we use a ASCII case-insensitive compare instead of lowercasing the target?
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
- Dominic Farolino
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
Seems like there are tons of unrelated indentation/styling changes to VTS, can those be reverted?
Related details
- Noam Rosenthal
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
Seems like there are tons of unrelated indentation/styling changes to VTS, can those be reverted?
- Dominic Farolino
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Code-Review | +1 |
1 comment
VTS lgtm; cc'ing kouhei@ (non-blocking) since he's usually interested in this kind of change.
Related details
- Noam Rosenthal
- Philip Jägenstedt
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/57716.
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
- Noam Rosenthal
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Commit-Queue+2![]( "Open in Gerrit")
- Philip Jägenstedt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Support processing instructions in the HTML parser
The parser now recognizes <?target data> as a ProcessingInstruction and
adds it to the DOM instead of a bogus comment.
As per spec PR:
- xml/xml-stylesheet are blocklisted, and stay a bogus comment.
We can add more of these if there are compat issues.
- A PI can appear wherever a comment appears.
- ?> at the end ignores the ?
Currently in this CL, PI targets are constrained to
/^[A-Za-z][A-Za-z0-9-]*$/.
Added a VTS that keeps current behavior, so that we don't lose some of
the existing html5lib tests while this is in development.
See spec PR: https://github.com/whatwg/html/pull/12118
- M third_party/blink/renderer/core/editing/testing/selection_sample_test.cc
- M third_party/blink/renderer/core/html/html_view_source_document.cc
- M third_party/blink/renderer/core/html/parser/atomic_html_token.cc
- M third_party/blink/renderer/core/html/parser/atomic_html_token.h
- M third_party/blink/renderer/core/html/parser/html_construction_site.cc
- M third_party/blink/renderer/core/html/parser/html_construction_site.h
- M third_party/blink/renderer/core/html/parser/html_document_parser_test.cc
- M third_party/blink/renderer/core/html/parser/html_token.h
- M third_party/blink/renderer/core/html/parser/html_tokenizer.cc
- M third_party/blink/renderer/core/html/parser/html_tokenizer.h
- M third_party/blink/renderer/core/html/parser/html_tokenizer_test.cc
- M third_party/blink/renderer/core/html/parser/html_tree_builder.cc
- M third_party/blink/renderer/core/html/parser/html_tree_builder.h
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/VirtualTestSuites
- A third_party/blink/web_tests/external/wpt/dom/nodes/MutationObserver-characterData-expected.txt
- A third_party/blink/web_tests/external/wpt/html/syntax/parsing/parse-processing-instruction.tentative.html
- M third_party/blink/web_tests/fast/xpath/xpath-functional-test-expected.txt
- A third_party/blink/web_tests/virtual/parse-processing-instructions-as-bogus-comments/README.md
Code-Review: +1 by Dominic Farolino, +1 by Philip Jägenstedt
| 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")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57716
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |