-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Relax <select>
parser
#10557
base: main
Are you sure you want to change the base?
Relax <select>
parser
#10557
Conversation
Why does this also make a bunch of changes to the processing model of |
The changes in the processing model do support the new proposed content model for select, but they also mitigate compat issues for websites which are already putting tags in between For example, without these changes to the processing model, the following <select>
<div>
<option>...</option>
...
</div>
</select> In my compat analysis, I found a lot of websites which are doing this, so in order to ship the parser changes separately from customizable select in chrome, I plan to ship the parser changes and these processing model changes together, because otherwise there would be too much breakage. I'm happy to put them in a separate PR if you want, or keep them here and update the commit message (sorry for not putting it in there). Which would you prefer? |
Thanks! Given that rationale I think it's good to couple the changes, but that should be in the commit message as well. |
This doesn't define optional tags for The definition for "have a particular element in select scope" may be needed for that, but should be changed to be similar to "have a particular element in button scope" (but for In particular, allow these without a parse error: <select>
<optgroup>
<option>
<optgroup>
</select> The second <select>
<option><p>
<option>
</select> This should generate implied end tags and pop the <select>
<optgroup>
<hr>
<option>
<hr>
</select> The See how the parser deals with |
cf37251
to
afb732d
Compare
Done.
@zcorpan thanks for the feedback! I did some experimenting and added some logic to the start tags for option, optgroup, and hr. What do you think? |
afb732d
to
e08df7e
Compare
|
I thought that this is covered by "While the stack of open elements has an option element in select scope". What exactly should I change?
Done
I'm guessing this is from "If the current node is not now a rtc element or a ruby element, this is a parse error," right? Should I add "If the current node is not now an option element, this is a parse error" after "While the stack of open elements has an option element in select scope, pop an element from the stack of open elements"? |
ea6d608
to
21bc5e6
Compare
The "in select scope" I think should be removed altogether since it assumes the stack will not have other elements when in a High-level of what I think should happen: when parsing option or optgroup start tag: check that select is in scope, check that option or optgroup is in scope, generate implied end tags (except for optgroup when handling I can look into this more next week and suggest more specific changes. |
21bc5e6
to
a57c84e
Compare
Thanks! I gave this a try |
@zcorpan how does the latest text look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the option/optgroup cases are right after these changes.
a57c84e
to
a1edb00
Compare
source
Outdated
<ol> | ||
<li><p><span>Generate implied end tags</span> except for <code>option</code> and | ||
<code>optgroup</code> elements.</p></li> | ||
|
||
<li><p>If the <span>current node</span> is not now an <code>option</code> or | ||
<code>optgroup</code> element, this is a <span>parse error</span>.</p></li> | ||
|
||
<li><p>While the <span>stack of open elements</span> <span data-x="has an element in scope">has | ||
an <code>option</code> element in scope</span> or <span data-x="has an element in scope">has an | ||
<code>optgroup</code> element in scope</span>, pop an element from the <span>stack of open | ||
elements</span>.</p></li> | ||
</ol> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should report a parse error if there's an element between the optgroup and option in the stack:
<select>
<optgroup>
<div>
<option>
<optgroup>
This can be done by popping the option
element if that is the current node (after generating implied end tags except for option
and optgroup
), then generate implied end tags except for optgroup
elements, parse error if the current node is not an optgroup
element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. how does it look?
a1edb00
to
9507110
Compare
I was talking to @mfreed7 about the changes we've made in the PR so far, and I feel like I couldn't provide a good explanation for why we are making the parser not support cases like these:
Is it just compat reasons? Is there a good justification? |
This patch makes the parser allow additional tags in <select> besides <option>, <optgroup>, and <hr>, mostly by removing the "in select" and "in select in table" parser modes. In order to replicate the behavior where opening a <select> tag within another open <select> tag inserts a </select> close tag, a traversal through the stack of open elements was added which I borrowed from the <button> part of the parser. This will need test changes to be implemented in html5lib. Fixes whatwg#10310
9507110
to
1330ad5
Compare
It would be a breaking change from what is conforming HTML today, and break compat for sites that omit |
Generating all implied end tags with no exceptions in the optgroup case disallows nested optgroups, but I guess I'm ok with that since we can make additional changes to allow it later. I'll apply the changes in here so far to the chromium implementation and start working on html5lib tests so we can get this merged. |
I'm not a big fan of breaking conforming HTML. Especially as there doesn't appear to be a compelling reason from reading the thread. I recommend not using "grandfathering" by the way, it has a rather bad history: https://en.wikipedia.org/wiki/Grandfather_clause |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should retain existing behavior for hr
both for imaginable Web compat and for the principle of avoiding breaking existing conforming HTML.
I'm marking this as Request changes for that reason, but I admit that I'm not confident from looking at the spec diff alone that I can understand all other implications of this spec change, so there may well be other issues.
Ok, I pushed a change to disallow hr in option. How does it look now? |
I just re-added some cases to option and optgroup parsing to continue supporting auto-closing tags outside of a select tag to support cases like this, which I found in a test case here: const select = document.createElement('select');
select.innerHTML = '<option>one<option>two'; |
@@ -129438,6 +129444,8 @@ document.body.appendChild(text); | |||
|
|||
<li><p>If the <span>current node</span> is an <code>optgroup</code> element, then pop that node | |||
from the <span>stack of open elements</span>.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These "Otherwise" steps should only pop an option
element if it's the current node to match the current spec.
<li><p>If the <span>current node</span> is an <code>option</code> element, then pop the
<span>current node</span> off the <span>stack of open elements</span>.</p></li>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I just realized that I messed up this part and copied it from the end tag steps instead of the start tag steps, as well as the formatting step for the option start tag. I just rewrote this part here, how does it look? 33370b7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small comments I just noticed:
<li><p><span>Generate implied end tags</span> except for <code>optgroup</code> | ||
elements.</p></li> | ||
|
||
<li><p>If the <span>stack of open elements</span> <span data-x="has an element in scope">has an | ||
<code>option</code> element in scope</span>, then this is a <span>parse error</span>.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"hr" should be changed to match what "optgroup" does (when select is in scope)
If the stack of open elements has a select element in scope , then:
Generate implied end tags .
If the stack of open elements has an option element in scope or has an optgroup element in scope , then this is a parse error .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -129413,8 +129413,6 @@ document.body.appendChild(text); | |||
<ol> | |||
<li><p>If the <span>current node</span> is an <code>option</code> element, then pop the | |||
<span>current node</span> off the <span>stack of open elements</span>.</p></li> | |||
|
|||
<li><p><span>Reconstruct the active formatting elements</span>, if any.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I re-added it to the "otherwise" case.
Do you think we should also run this in the not-"otherwise" case when we are in a select tag?
If we parse this html:
<select><div><i></div><option>option
We will get this resulting innerHTML if we reconstruct in both cases:
<select><div><i></i></div><i><option>option</option></i></select><i>
Otherwise if we only reconstruct in the "otherwise" case, we get this:
<select><div><i></i></div><option><i>option</i></option></select><i>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing the same thing in both cases for consistency sounds somewhat appealing to me
source
Outdated
|
||
<li><p><span>Reconstruct the active formatting elements</span>, if any.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-added, but leaving this open to make sure we match the same behavior for option and optgroup based on the outcome of the other thread
<p>For each <var>ancestor</var> of <var>insertedOption</var>'s <span | ||
data-x="ancestor">ancestors</span>:</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to define ordering here and below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that would be good to clarify. I've seen "in tree order" appended to loops like this before, but I'm not sure that's appropriate here - I want to start at the parent of insertedOption, and then keep going up from there. I'm worried that saying "in tree order" could imply the opposite.
Should I say "starting from the parent of insertedOption"?
This PR updates the tree-construction dat files for the HTML change which will allow additional tags within <select>: whatwg/html#10557
I created an html5lib PR to update tests: html5lib/html5lib-tests#178 |
This patch makes the parser allow additional tags in
<select>
besides<option>
,<optgroup>
, and<hr>
, mostly by removing the "in select" and "in select in table" parser modes.In order to replicate the behavior where opening a
<select>
tag within another open<select>
tag inserts a</select>
close tag, a traversal through the stack of open elements was added which I borrowed from the<button>
part of the parser.This patch also changes the processing model to make
<select>
look through all its descendants in the DOM tree for<option>
elements, rather than just children and optgroup children which conform to the content model. This is needed for compat reasons because there are websites which put other tags in between their<select>
and<option>
s which would break with this parser change unless we also update this processing model. More context here and here.Fixes #10310
<select>
parser mozilla/standards-positions#1086<select>
parser WebKit/standards-positions#414(See WHATWG Working Mode: Changes for more details.)
/form-elements.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/parsing.html ( diff )