-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 |
But that's true of the rest of this PR also, correct? E.g. making |
I expect we can't allow nesting of |
To clarify, I meant: It would be a breaking change for existing conforming HTML content. |
fwiw, i can't think of a good reason for an author to even need an if someone wants to create a visual separation between the text within an option - then that sounds like something that could/should be accomplished with CSS. The semantics/a11y mappings of a separator dont' really make sense in the context of an option |
Yeah that's fair, UIs won't support nested optgroups, at least not yet. There is a separate issue to track this here: #5789 I suppose that if we want to support nested optgroups then we could revisit this, and disallow nested optgroups in the parser for now.
We can forbid hr in select in the content model and still allow it to be parsed, just like we are doing with many other tags in this change. I suspect that hr in option would be a significantly less breaking change than other things. I see the conforming HTML argument though and I don't feel that strongly about this. Mason's point is that we should only create special cases like this if we have a really good reason. If not changing conforming HTML is that important, then I think its probably fine since hr in select is not an important use case. |
|
Since the content model in #10586 only allows If, for web compat, it's ok to only close |
The content model PR currently doesn't list hr elements in the content model for options
Eventually allowing other HTML elements is ideal as we add support for additional use cases and find ways to make them accessible.
As Mason said, ideally we try removing restrictions and see what real issues arise. Would I implement this by removing the "generate implied end tags" line? |
If we envision allowing more HTML elements later, then we should at least generate implied end tags, so that HTML authoring rules are somewhat consistent. For example, it should be possible to omit |
Ah I see. So if I leave that in and then remove the "While the stack of open elements has an option element in scope, pop an element" part and the equivalent one for optgroup then we should be good? |
No, still need to pop the Something like this:
This also removes "Reconstruct the active formatting elements , if any.", so that that happens inside the |
Thanks! I implemented your suggestion |
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.
Two small comments I just noticed:
|
||
<ol> | ||
<li> | ||
<p>For each <var>ancestor</var> of <var>option</var>'s <span |
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.
option
here should be insertedOption
|
||
<ol> | ||
<li><p>If <var>ancestor</var> is a <code>select</code> element, then run the <span>selectedness | ||
setting algorithm</span> given <var>select</var> and return.</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.
select
isn't a variable here; I think you want ancestor
<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.
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
(See WHATWG Working Mode: Changes for more details.)
/form-elements.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/parsing.html ( diff )