Fix: Mis-nested form tags inside template parsed incorrectly [chromium/src : main]
Haoran Tang (Gerrit)
Haoran Tang added 1 comment![]( "Open in Gerrit")
Hi reviewers, I fixed the issue by adjusting the parsing for mis-nested `<form>` tags in `<template>`, based on [HTML5 specs](https://html.spec.whatwg.org/multipage/parsing.html#parsing-main-inbody).
I do have a couple of questions:
1. Is `fast/parser/misnested-form-tags-in-template.html` the right location for this test?
2. How can I configure this test to be text-only? It currently fails with `-expected.png was missing`.
I'm new to chromium, apologies for any mistakes i made.
Related details
- Kouhei Ueno
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haoran Tang (Gerrit)
Haoran Tang voted Commit-Queue+1![]( "Open in Gerrit")
- Haoran Tang
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 6 comments![]( "Open in Gerrit")
Thanks for writing what looks like your first patch to Chromium! 😊
aligning behavior with HTML5 parsing spec.It would help to link directly to the relevant section of the HTML spec in this comment also. For this section, it's a bit hard to link to, but you should be able to use this:
Fix: Mis-nested form tags inside template
parsed incorrectly
Ensures that form end tags inside templates
forcibly close any inner tags,
aligning behavior with HTML5 parsing spec.nit: please line wrap at 72 characters.
Haoran Tang <haoran.tan...@gmail.com>Note to self: I verified that this user has signed the CLA.
if (!tree_.OpenElements()->InScope(tag)) {
ParseError(token);
return;
}
tree_.GenerateImpliedEndTags();
if (!tree_.CurrentStackItem()->MatchesHTMLTag(tag)) {
ParseError(token);
}
tree_.OpenElements()->PopUntilPopped(tag);This looks good. But for safety, it's always a good idea to flag-guard any such change, in case of emergency. You can use this as an example to follow:
Thank you for writing a test. But this should be a WPT, rather than an (internal-only) web_test. I think this is probably the right place to put that:
and you could use something like this one as an example to follow:
Related details
- Haoran Tang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haoran Tang (Gerrit)
Haoran Tang abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haoran Tang (Gerrit)
Haoran Tang added 4 comments![]( "Open in Gerrit")
Fix: Mis-nested form tags inside template
parsed incorrectly
Ensures that form end tags inside templates
forcibly close any inner tags,
aligning behavior with HTML5 parsing spec.nit: please line wrap at 72 characters.
Done
aligning behavior with HTML5 parsing spec.It would help to link directly to the relevant section of the HTML spec in this comment also. For this section, it's a bit hard to link to, but you should be able to use this:
is there a preferred way to wrap long URLs in the commit message
if (!tree_.OpenElements()->InScope(tag)) {
ParseError(token);
return;
}
tree_.GenerateImpliedEndTags();
if (!tree_.CurrentStackItem()->MatchesHTMLTag(tag)) {
ParseError(token);
}
tree_.OpenElements()->PopUntilPopped(tag);This looks good. But for safety, it's always a good idea to flag-guard any such change, in case of emergency. You can use this as an example to follow:
Done
Thank you for writing a test. But this should be a WPT, rather than an (internal-only) web_test. I think this is probably the right place to put that:
and you could use something like this one as an example to follow:
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haoran Tang (Gerrit)
Haoran Tang added 1 comment![]( "Open in Gerrit")
aligning behavior with HTML5 parsing spec.Haoran TangIt would help to link directly to the relevant section of the HTML spec in this comment also. For this section, it's a bit hard to link to, but you should be able to use this:
is there a preferred way to wrap long URLs in the commit message
Done
Related details
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 8 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Commit-Queue | +1 |
8 comments
Thanks for adding the flag and moving the test! Just a few smaller comments now and then I think this is ok to land.
name: "CorrectTemplateFormParsing",I think to pass the tests, this needs to be in the correct position, alphabetically.
<html>
<head>nit: it's better if you don't include non-essential tags in the test. So you can generally skip `<html>`, `<head>`, `<body>`, etc.
<title>Template Form Parsing Test</title>Feel free, if you like, to add an `author` <link> tag, like this one:
<h1>Template Form Parsing Test</h1>You don't need this
<div id="log" style="display:block"></div>You don't need this either.
<div id="testTemplateContainer">This is ok, but easier would be:
```
<div>
A<template><br>BC<form>D<div>E</form>F</div>G</template>H
</div>
const wrapper = document.querySelector('div');
```
Related details
- Haoran Tang
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haoran Tang (Gerrit)
Haoran Tang added 7 comments![]( "Open in Gerrit")
I think to pass the tests, this needs to be in the correct position, alphabetically.
Done
nit: it's better if you don't include non-essential tags in the test. So you can generally skip `<html>`, `<head>`, `<body>`, etc.
Done
Feel free, if you like, to add an `author` <link> tag, like this one:
Done
You don't need this
Done
You don't need this either.
Done
You don't need this
Done
This is ok, but easier would be:
```
<div>
A<template><br>BC<form>D<div>E</form>F</div>G</template>H
</div>const wrapper = document.querySelector('div');
```
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haoran Tang (Gerrit)
Haoran Tang added 1 comment![]( "Open in Gerrit")
Hi Mason. I've updated the test to alphabetical order and removed unnecessary components from the test HTML. Could you help start the test run?
Related details
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
1 comment
Assuming the tests pass, LGTM! Thanks for contributing to Chromium! 🎉
I can land this for you, just let me know.
Related details
- Haoran Tang
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haoran Tang (Gerrit)
Haoran Tang added 1 comment![]( "Open in Gerrit")
Assuming the tests pass, LGTM! Thanks for contributing to Chromium! 🎉
I can land this for you, just let me know.
Hi Mason. As I’m not a Chromium committer, it seems that two Code-Review +1s are needed to proceed. Would I need an additional reviewer for this CL to be landed.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 2 comments![]( "Open in Gerrit")
Haoran TangAssuming the tests pass, LGTM! Thanks for contributing to Chromium! 🎉
I can land this for you, just let me know.
Hi Mason. As I’m not a Chromium committer, it seems that two Code-Review +1s are needed to proceed. Would I need an additional reviewer for this CL to be landed.
Ahh right, I forgot about that. I'll add one.
+dizhangg for another review.
Related details
- Di Zhang
- Haoran Tang
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Di Zhang (Gerrit)
Di Zhang voted Code-Review+1![]( "Open in Gerrit")
- Haoran Tang
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haoran Tang (Gerrit)
Haoran Tang added 1 comment![]( "Open in Gerrit")
Haoran TangAssuming the tests pass, LGTM! Thanks for contributing to Chromium! 🎉
I can land this for you, just let me know.
Mason FreedHi Mason. As I’m not a Chromium committer, it seems that two Code-Review +1s are needed to proceed. Would I need an additional reviewer for this CL to be landed.
Ahh right, I forgot about that. I'll add one.
Hi, Mason. Since the tests have passed, could you help submit it for me? Thank you for your guidance and for reviewing my CL!
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haoran Tang (Gerrit)
Haoran Tang voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Review
| 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/49206.
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
- Haoran Tang
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Haoran Tang
Code-Coverage
Code-Review
| 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
Fix: Mis-nested form tags inside template parsed incorrectly
Ensures that form end tags inside templates forcibly close any inner
tags, aligning behavior with HTML5 standard:
https://html.spec.whatwg.org/multipage/parsing.html#parsing:~:text=An%20end%20tag%20whose%20tag%20name%20is%20%22form%22
Bug: 352896478
TEST=Added web test case to check parsing behavior with nested form
tags.
- M AUTHORS
- M third_party/blink/renderer/core/html/parser/html_tree_builder.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- A third_party/blink/web_tests/external/wpt/html/syntax/parsing/misnested-form-in-template.html
Code-Review: +1 by Mason Freed, +1 by Di Zhang
| 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/49206
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brayan Cruz (Gerrit)
Brayan Cruz added 1 comment![]( "Open in Gerrit")
- Di Zhang
- Haoran Tang
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ho Cheung (Gerrit)
Ho Cheung added 1 comment![]( "Open in Gerrit")
I observed the following results on Chrome Canary, which still don't seem to be expected.[1]
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ho Cheung (Gerrit)
Ho Cheung added 1 comment![]( "Open in Gerrit")
I observed the following results on Chrome Canary, which still don't seem to be expected.[1]
- Di Zhang
- Haoran Tang
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haoran Tang (Gerrit)
Haoran Tang added 1 comment![]( "Open in Gerrit")
Ho CheungI observed the following results on Chrome Canary, which still don't seem to be expected.[1]
Perhaps I'm missing something?
Thanks for the update. I’ll look into this in the coming days and keep you noticed.
Related details
- Di Zhang
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ho Cheung (Gerrit)
Ho Cheung added 1 comment![]( "Open in Gerrit")
Ho CheungI observed the following results on Chrome Canary, which still don't seem to be expected.[1]
Haoran TangPerhaps I'm missing something?
Thanks for the update. I’ll look into this in the coming days and keep you noticed.
Enabling the experimental flag `chrome://flags/#enable-experimental-web-platform-features` can solve the problem.
Related details
- Di Zhang
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |