Skip to content

Commit

Permalink
Relax <select> parser rules
Browse files Browse the repository at this point in the history
This patch makes <select> allow tags besides <option>, <optgroup>, and
<hr>. Previously this was only allowed within a child <button> or
<datalist> tag inside <select>, but based on the feedback in whatwg we
should try to allow this content everywhere:
whatwg/html#10310

This behavior is guarded behind a flag. Since I am planning on shipping
parser changes for <select> before appearance:base-select, I am creating
a new flag for parser changes instead of reusing the existing
StylableSelect flag for appearance:base-select. The new flag is intended
to not only make the parser change, but also update the algorithms which
associate option/optgroup/hr elements with select elements to account
for the newly parsed elements.

If everything goes well, then we will need to change these WPTs which
this patch effectively marks as failing:
html/infrastructure/common-dom-interfaces/collections/htmloptionscollection.html
html/semantics/forms/the-select-element/select-value.html
html/syntax/parsing/

Bug: 1511354
Change-Id: I441f9645a592ac63764fef928e4e5acf3fdec5db
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5518837
Commit-Queue: Joey Arhar <[email protected]>
Reviewed-by: David Baron <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1329137}
  • Loading branch information
josepharhar authored and marcoscaceres committed Jul 22, 2024
1 parent 33aa217 commit 18b0909
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 48 deletions.
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
test(() => {
const select = document.createElement("select");
select.innerHTML = "<optgroup><option>1<optgroup><option>2";
select.innerHTML = "<optgroup><option>1</optgroup><optgroup><option>2";
assert_equals(select.value, "1");
select.querySelector("optgroup").remove();
assert_equals(select.value, "2");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,65 +5,80 @@
<script src="/resources/testharnessreport.js"></script>

<select>
<datalist>
<option class=one>one</option>
<div>
<option class=two>two</option>
<option class=one>one</option>
<datalist id=datalist1>
<option class=two>two</option>
<div id=div1>
<option class=three>three</option>
</div>
<option class=three>three</option>
<option class=four>four</option>
</datalist>
<datalist>
<option>ignored since not in first datalist</option>
<option class=five>five</option>
</datalist>
<div id=div2>
<option class=six>six</option>
</div>
</select>

<script>
const select = document.querySelector('select');

function runTest() {
const datalist = select.querySelector('datalist');
const datalist = document.getElementById('datalist1');
const firstOption = select.querySelector('option.one');
const secondOption = select.querySelector('option.two');
const thirdOption = select.querySelector('option.three');
const datalistChildDiv = datalist.querySelector('div');

let selectChildDiv = document.querySelector('select > div');
if (!selectChildDiv) {
selectChildDiv = document.createElement('div');
select.appendChild(selectChildDiv);
}
const fourthOption = select.querySelector('option.four');
const fifthOption = select.querySelector('option.five');
const sixthOption = select.querySelector('option.six');
const datalistChildDiv = document.getElementById('div1');
const selectChildDiv = document.getElementById('div2');

assert_equals(select.length, 3, 'select.length');
assert_equals(select.options.length, 3, 'select.options.length');
assert_equals(select.length, 6, 'select.length');
assert_equals(select.options.length, 6, 'select.options.length');
assert_equals(select.options[0], firstOption, 'select.options[0]');
assert_equals(select.options[1], secondOption, 'select.options[1]');
assert_equals(select.options[2], thirdOption, 'select.options[2]');
assert_equals(select.options[3], fourthOption, 'select.options[3]');
assert_equals(select.options[4], fifthOption, 'select.options[4]');
assert_equals(select.options[5], sixthOption, 'select.options[5]');

assert_equals(select.value, 'one', 'initial select.value');
select.value = 'two';
assert_equals(select.value, 'two', 'select.value after modifying');

secondOption.remove();
assert_equals(select.length, 2, 'select.length after removing an option');
assert_equals(select.options.length, 2, 'select.options.length after removing an option');
fourthOption.remove();
assert_equals(select.length, 5, 'select.length after removing an option');
assert_equals(select.options.length, 5, 'select.options.length after removing an option');
assert_equals(select.options[0], firstOption, 'select.options[0] after removing an option');
assert_equals(select.options[1], thirdOption, 'select.options[1] after removing an option');
assert_equals(select.options[1], secondOption, 'select.options[1] after removing an option');
assert_equals(select.options[2], thirdOption, 'select.options[0] after removing an option');
assert_equals(select.options[3], fifthOption, 'select.options[1] after removing an option');
assert_equals(select.options[4], sixthOption, 'select.options[1] after removing an option');

datalist.appendChild(secondOption);
assert_equals(select.length, 3, 'select.length after re-adding an option');
assert_equals(select.options.length, 3, 'select.options.length after re-adding an option');
datalist.appendChild(fourthOption);
assert_equals(select.length, 6, 'select.length after re-adding an option');
assert_equals(select.options.length, 6, 'select.options.length after re-adding an option');
assert_equals(select.options[0], firstOption, 'select.options[0] after re-adding an option');
assert_equals(select.options[1], thirdOption, 'select.options[1] after re-adding an option');
assert_equals(select.options[2], secondOption, 'select.options[2] after re-adding an option');
assert_equals(select.options[1], secondOption, 'select.options[1] after re-adding an option');
assert_equals(select.options[2], thirdOption, 'select.options[2] after re-adding an option');
assert_equals(select.options[3], fourthOption, 'select.options[2] after re-adding an option');
assert_equals(select.options[4], fifthOption, 'select.options[2] after re-adding an option');
assert_equals(select.options[5], sixthOption, 'select.options[2] after re-adding an option');

selectChildDiv.appendChild(secondOption);
assert_equals(select.length, 2, 'select.length after moving option to child div');
assert_equals(select.options.length, 2, 'select.options.length after moving option to child div');
selectChildDiv.appendChild(fourthOption);
assert_equals(select.length, 6, 'select.length after moving option to child div');
assert_equals(select.options.length, 6, 'select.options.length after moving option to child div');
assert_equals(select.options[0], firstOption, 'select.options[0] after moving option to child div');
assert_equals(select.options[1], thirdOption, 'select.options[1] after moving option to child div');
assert_equals(select.options[1], secondOption, 'select.options[1] after moving option to child div');
assert_equals(select.options[2], thirdOption, 'select.options[2] after moving option to child div');
assert_equals(select.options[3], fifthOption, 'select.options[3] after moving option to child div');
assert_equals(select.options[4], sixthOption, 'select.options[4] after moving option to child div');
assert_equals(select.options[5], fourthOption, 'select.options[5] after moving option to child div');

// reset back to normal for the next test
datalistChildDiv.appendChild(secondOption);
datalist.appendChild(fourthOption);
select.value = 'one';
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,9 @@
</select>

<select id=s6>
<button>
<button></button>
</button>
<datalist>
<datalist></datalist>
</datalist>
<div>
<option><img>option</option>
</div>
</select>

<div id=afterlast>
Expand All @@ -55,15 +52,15 @@
// previous test case didn't leave the HTML parser open on another element.
assert_equals(document.getElementById('s1').parentNode, document.body);
assert_equals(document.getElementById('s1').innerHTML, `
div 1
<div>div 1</div>
<button>button</button>
div 2
<div>div 2</div>
<datalist>
<option>option</option>
</datalist>
div 3
<div>div 3</div>
`);
}, '<button>s and <datalist>s should be allowed in <select>.');
}, '<div>s, <button>s, and <datalist>s should be allowed in <select>.');

test(() => {
assert_equals(document.getElementById('s2').parentNode, document.body);
Expand Down Expand Up @@ -97,14 +94,11 @@
test(() => {
assert_equals(document.getElementById('s6').parentNode, document.body);
assert_equals(document.getElementById('s6').innerHTML, `
<button>
</button>
<datalist>
</datalist>
<div>
<option><img>option</option>
</div>
`);
}, 'Nested <button>s or <datalist>s in <select> should be dropped.');
}, 'Divs and imgs should be allowed as direct children of select and within options without a datalist.');

test(() => {
assert_equals(document.getElementById('afterlast').parentNode, document.body);
Expand Down

0 comments on commit 18b0909

Please sign in to comment.