Relax `<select>` parser #10557

josepharhar · 2024-08-13T22:37:01Z

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

At least two implementers are interested (and none opposed):
Tests are written and can be reviewed and commented upon at:
- Update tests for relaxed <select> parser html5lib/html5lib-tests#178
- https://wpt.fyi/results/html/semantics/forms/the-select-element/customizable-select/select-parsing.tentative.html
Implementation bugs are filed:
- Chromium: Already implemented
- Gecko: …
- WebKit: …
MDN issue is filed: …
The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

/form-elements.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/parsing.html ( diff )

annevk · 2024-08-14T11:39:54Z

Why does this also make a bunch of changes to the processing model of select elements? If that needs to be part of this change that should really be better motivated in the commit message.

josepharhar · 2024-08-16T00:08:20Z

Why does this also make a bunch of changes to the processing model of select elements? If that needs to be part of this change that should really be better motivated in the commit message.

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 <select> and <option> in their HTML.

For example, without these changes to the processing model, the following <select> would not register as having any options at all:

<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?

annevk · 2024-08-16T06:11:37Z

Thanks! Given that rationale I think it's good to couple the changes, but that should be in the commit message as well.

zcorpan · 2024-08-19T15:59:08Z

This doesn't define optional tags for </option> and </optgroup> correctly.

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 select).

In particular, allow these without a parse error:

<select>
<optgroup>
<option>
<optgroup>
</select>

The second <optgroup> should pop the option and optgroup

<select>
<option><p>
<option>
</select>

This should generate implied end tags and pop the option.

<select>
<optgroup>
<hr>
<option>
<hr>
</select>

The hrs should pop the optgroup and option in a select.

See how the parser deals with <ruby><rtc><rt>, I think that can be used as a model for select.

source

josepharhar · 2024-08-19T21:28:14Z

Thanks! Given that rationale I think it's good to couple the changes, but that should be in the commit message as well.

Done.

This doesn't define optional tags for </option> and </optgroup> correctly.

@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?

zcorpan · 2024-08-22T09:02:46Z

Need to check that a select element is in scope so that parsing of option/optgroup tags outside of select doesn't change. Example <option><p>1</option>2<option>3
Need to add select to the "in scope" list so that <div><select></div><option> doesn't close the select.
Should report parse errors when unexpected elements are popped. Compare with what the spec does for rt/rtc in ruby. Example <select><option><span>1</option>2<option>3

josepharhar · 2024-08-22T23:57:54Z

Need to check that a select element is in scope so that parsing of option/optgroup tags outside of select doesn't change. Example <option><p>1</option>2<option>3

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?

Need to add select to the "in scope" list so that <div><select></div><option> doesn't close the select.

Done

Should report parse errors when unexpected elements are popped. Compare with what the spec does for rt/rtc in ruby. Example <select><option><span>1</option>2<option>3

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"?

zcorpan · 2024-08-23T11:58:59Z

The "in select scope" I think should be removed altogether since it assumes the stack will not have other elements when in a select, which is no longer the case. Use the normal "in scope" instead.

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 <option>), check the current node or the stack of open elements again, report a parse error if appropriate, then pop elements off the stack until the option or optgroup has been popped, then insert the new element.

I can look into this more next week and suggest more specific changes.

josepharhar · 2024-08-28T00:26:31Z

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 <option>), check the current node or the stack of open elements again, report a parse error if appropriate, then pop elements off the stack until the option or optgroup has been popped, then insert the new element.

Thanks! I gave this a try

josepharhar · 2024-09-04T20:34:56Z

@zcorpan how does the latest text look?

zcorpan

I think the option/optgroup cases are right after these changes.

source

zcorpan · 2024-09-10T09:58:49Z

source

+    <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>


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.

done. how does it look?

source

josepharhar · 2024-09-10T22:34:51Z

This doesn't define optional tags for </option> and </optgroup> correctly.

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:

<hr> inside an <option>
nested <optgroup>s

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

zcorpan · 2024-09-11T08:53:59Z

It would be a breaking change from what is conforming HTML today, and break compat for sites that omit </option> and </optgroup> tags. It's the same as why we can't allow certain elements in p.

josepharhar · 2024-09-18T18:38:43Z

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.

zcorpan · 2024-09-18T19:33:56Z

Great.

hr no longer closing option and optgroup is a behavior change from what the parser currently does, which I'm somewhat uncomfortable with. It makes currently valid HTML invalid. @hsivonen @annevk WDYT?

annevk · 2024-09-19T10:36:22Z

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

hsivonen

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.

josepharhar · 2024-09-20T22:32:52Z

Ok, I pushed a change to disallow hr in option. How does it look now?

josepharhar · 2024-10-02T18:23:10Z

@zcorpan @hsivonen are there any other changes you think should be made?

josepharhar · 2024-10-02T21:07:31Z

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';

source

zcorpan · 2024-10-07T13:52:54Z

source

@@ -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>


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>

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

dbaron

Two small comments I just noticed:

source

zcorpan · 2024-10-07T19:49:01Z

source

+     <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>


"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 .

...to more closely match the current spec. However, if there's a desire to change this for #9247 then the current text (which is like "option") is better. cc @annevk

zcorpan · 2024-10-10T10:54:08Z

source

@@ -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>


Why was this removed?

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>

Doing the same thing in both cases for consistency sounds somewhat appealing to me

source

zcorpan · 2024-10-10T10:56:54Z

source


-     <li><p><span>Reconstruct the active formatting elements</span>, if any.</p></li>


Why was this removed?

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

annevk · 2024-10-10T13:59:14Z

source

+    <p>For each <var>ancestor</var> of <var>insertedOption</var>'s <span
+    data-x="ancestor">ancestors</span>:</p>


I think you need to define ordering here and below.

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

josepharhar · 2024-10-11T23:06:23Z

I created an html5lib PR to update tests: html5lib/html5lib-tests#178

- whatwg/html#10557 - html5lib/html5lib-tests#178

josepharhar marked this pull request as ready for review August 13, 2024 22:39

josepharhar mentioned this pull request Aug 13, 2024

HTML parser changes for customizable <select> #10310

Open

annevk requested review from zcorpan and hsivonen August 14, 2024 11:36

annevk added normative change topic: parser labels Aug 14, 2024

dmsnell mentioned this pull request Aug 16, 2024

HTML API: Roadmap WordPress/gutenberg#60397

Open

10 tasks

zcorpan requested changes Aug 19, 2024

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

josepharhar force-pushed the selectparser branch from cf37251 to afb732d Compare August 19, 2024 21:28

josepharhar force-pushed the selectparser branch from afb732d to e08df7e Compare August 21, 2024 20:21

josepharhar force-pushed the selectparser branch from ea6d608 to 21bc5e6 Compare August 23, 2024 00:01

josepharhar force-pushed the selectparser branch from 21bc5e6 to a57c84e Compare August 28, 2024 00:26

zcorpan requested changes Sep 5, 2024

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

josepharhar force-pushed the selectparser branch from a57c84e to a1edb00 Compare September 5, 2024 20:52

zcorpan reviewed Sep 10, 2024

View reviewed changes

josepharhar force-pushed the selectparser branch from a1edb00 to 9507110 Compare September 10, 2024 20:20

josepharhar force-pushed the selectparser branch from 9507110 to 1330ad5 Compare September 10, 2024 22:35

josepharhar mentioned this pull request Sep 18, 2024

Customizable <select> element #9799

Open

hsivonen suggested changes Sep 20, 2024

View reviewed changes

dont allow hr in option

9c72f6c

dmsnell mentioned this pull request Sep 27, 2024

HTML API: Plans for 6.8 WordPress/gutenberg#63037

Open

11 tasks

re-add out of select cases

e07cb01

zcorpan requested changes Oct 3, 2024

View reviewed changes

source Outdated Show resolved Hide resolved

source Outdated Show resolved Hide resolved

re-add reconstruct formatting elements

5117d13

zcorpan reviewed Oct 7, 2024

View reviewed changes

dbaron reviewed Oct 7, 2024

View reviewed changes

source Show resolved Hide resolved

source Show resolved Hide resolved

zcorpan reviewed Oct 7, 2024

View reviewed changes

This was referenced Oct 9, 2024

Relax <select> parser mozilla/standards-positions#1086

Open

Relax <select> parser WebKit/standards-positions#414

Open

josepharhar added 2 commits October 9, 2024 11:54

fix non-select case

33370b7

fix variable names

76e0d7f

zcorpan reviewed Oct 10, 2024

View reviewed changes

annevk reviewed Oct 10, 2024

View reviewed changes

josepharhar added 2 commits October 10, 2024 15:24

re-add reconstruct active formatting elements

58b9ca9

remove pop optgroup

b8ba8e6

josepharhar added a commit to josepharhar/html5lib-tests that referenced this pull request Oct 11, 2024

Update tests for relaxed <select> parser

87db59f

This PR updates the tree-construction dat files for the HTML change which will allow additional tags within <select>: whatwg/html#10557

josepharhar mentioned this pull request Oct 11, 2024

Update tests for relaxed <select> parser html5lib/html5lib-tests#178

Open

flavorjones added a commit to sparklemotion/nokogiri that referenced this pull request Oct 16, 2024

Implement the latest select parsing changes

3cefda2

- whatwg/html#10557 - html5lib/html5lib-tests#178

flavorjones mentioned this pull request Oct 16, 2024

Implement the latest select parsing changes sparklemotion/nokogiri#3317

Open

flavorjones added a commit to sparklemotion/nokogiri that referenced this pull request Oct 16, 2024

Implement the latest select parsing changes

eb62613

- whatwg/html#10557 - html5lib/html5lib-tests#178

		@@ -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>


		<li><p><span>Reconstruct the active formatting elements</span>, if any.</p></li>

		<p>For each <var>ancestor</var> of <var>insertedOption</var>'s <span
		data-x="ancestor">ancestors</span>:</p>

Relax <select> parser #10557

Are you sure you want to change the base?

Relax <select> parser #10557

Conversation

josepharhar commented Aug 13, 2024 • edited by pr-preview bot Loading

annevk commented Aug 14, 2024

josepharhar commented Aug 16, 2024

annevk commented Aug 16, 2024

zcorpan commented Aug 19, 2024

josepharhar commented Aug 19, 2024

zcorpan commented Aug 22, 2024

josepharhar commented Aug 22, 2024

zcorpan commented Aug 23, 2024

josepharhar commented Aug 28, 2024

josepharhar commented Sep 4, 2024

zcorpan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josepharhar commented Sep 10, 2024

zcorpan commented Sep 11, 2024

josepharhar commented Sep 18, 2024

zcorpan commented Sep 18, 2024

annevk commented Sep 19, 2024

hsivonen left a comment

Choose a reason for hiding this comment

josepharhar commented Sep 20, 2024

josepharhar commented Oct 2, 2024

josepharhar commented Oct 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josepharhar commented Oct 11, 2024

Relax `<select>` parser #10557

Relax `<select>` parser #10557

josepharhar commented Aug 13, 2024 •

edited by pr-preview bot

Loading