Skip to content
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

Update content model for customizable select #10586

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented Aug 28, 2024

This PR updated the content model for the <select>, <option>, and <optgroup> elements in support of customizable <select>.

Fixes #10317

  • At least two implementers are interested (and none opposed):
    • Google
  • Tests are written and can be reviewed and commented upon at:
    • I'm not sure if this change can be tested?
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
    • Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): …
    • Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): …
  • MDN issue is filed: …
  • The top of this comment includes a clear commit message to use.

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


/dom.html ( diff )
/form-elements.html ( diff )
/grouping-content.html ( diff )
/index.html ( diff )
/scripting.html ( diff )
/text-level-semantics.html ( diff )

This PR updated the content model for the <select>, <option>, and
<optgroup> elements in support of customizable <select>.

Fixes whatwg#10317
@zcorpan zcorpan self-requested a review September 12, 2024 16:07
@zcorpan
Copy link
Member

zcorpan commented Sep 16, 2024

Is the idea to allow <select><div><h1><option> and such? Or only <select><div><div><option>?

If the former, you need to change the definition of phrasing content to include "option (if it is a descendant of a select element), optgroup (if it is a descendant of a select element)".

If the latter, you need to change the content model if div and span to switch on whether there's an ancestor select element, and if so allow "select element inner content elements". (But still disallow nesting of optgroup and option)

source Outdated
Comment on lines 53234 to 53235
<dd>Zero or more <span>select element inner content elements</span>.</dd>
<dd>Zero or one child <code>button</code>.</dd>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<dd>Zero or more <span>select element inner content elements</span>.</dd>
<dd>Zero or one child <code>button</code>.</dd>
<dd>Zero or more <span>select element inner content elements</span>, followed by zero or one
<code>button</code> element.</dd>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The button should probably go first, and I wanted to keep the "zero or one" concept with the button, so I wrote it like this:

<dd>Zero or one <code>button</code> element followed by zero or more <span>select element inner content elements</span>.</dd>

Look good?

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
<dd>If the element has no <code data-x="attr-option-label">label</code> attribute and is a child
of a <code>datalist</code> element: <span data-x="text content">Text</span>.</dd>
<dd>If the element has no <code data-x="attr-option-label">label</code> attribute: <span>Text content</span> and zero or more <code>div</code>, <code>span</code>, <code>noscript</code>, <code>img</code>, <span>SVG <code>svg</code></span>, and <span data-x="script-supporting elements">script-supporting</span> elements.</dd>
<dd><div class="note">If an <code>img</code> is used within an <code>option</code> and there is no other text within the <code>option</code>, then the <code>option</code> won't have an accessible name unless the <code>img</code> also has a non-empty <code data-x="attr-img-alt">alt</code> attribute.</div></dd>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have notes in the "element" box, put it further down along with the prose about the content model. The note is also not correct, so maybe it's better to remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was what @scottaohara suggested, but as per this comment, it sounds like we can add notes and examples as a follow up. I removed the note for now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

besides the placement could you elaborate on what was wrong with the note @zcorpan? i might have phrased it a bit differently, but it seemed to be saying <option><img src=...></option> would have no accessible name, and that is correct.

@zcorpan zcorpan mentioned this pull request Sep 16, 2024
5 tasks
source Outdated Show resolved Hide resolved
@josepharhar
Copy link
Contributor Author

Is the idea to allow <select><div><h1><option> and such? Or only <select><div><div><option>?

If the former, you need to change the definition of phrasing content to include "option (if it is a descendant of a select element), optgroup (if it is a descendant of a select element)".

If the latter, you need to change the content model if div and span to switch on whether there's an ancestor select element, and if so allow "select element inner content elements". (But still disallow nesting of optgroup and option)

h1 elements are not supposed to be part of the content model, so I implemented the div and span switching by pulling out the content models for option and optgroup and using them in div and span.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
@@ -53203,7 +53280,7 @@ interface <dfn interface>HTMLButtonElement</dfn> : <span>HTMLElement</span> {
<dt><span data-x="concept-element-contexts">Contexts in which this element can be used</span>:</dt>
<dd>Where <span>phrasing content</span> is expected.</dd>
<dt><span data-x="concept-element-content-model">Content model</span>:</dt>
<dd>Zero or more <code>option</code>, <code>optgroup</code>, <code>hr</code>, and <span data-x="script-supporting elements">script-supporting</span> elements.</dd>
<dd>Zero or one <code>button</code> element followed by zero or more <span>select element inner content elements</span>.</dd>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Description of "As first child of select element" should be added to "Contexts in which this element can be used:" of section of "4.10.6 The button element".

In this case, this button element is neither of "Submit Button" state, "Reset Button" state or "Button" state.
Therefore, a fourth state of button element sholud be defined.

In addition, Shouldn't The following sentent be added to section of "4.10.6 The button element"?

If the button element is a fourth state, "popovertarget", "popovertargetaction", "type", "name" and "value" attribute are must not be spedified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Description of "As first child of select element" should be added to "Contexts in which this element can be used:" of section of "4.10.6 The button element".

Done

In this case, this button element is neither of "Submit Button" state, "Reset Button" state or "Button" state. Therefore, a fourth state of button element sholud be defined.

In addition, Shouldn't The following sentent be added to section of "4.10.6 The button element"?

If the button element is a fourth state, "popovertarget", "popovertargetaction", "type", "name" and "value" attribute are must not be spedified.

Those states are based on the type attribute. Since we didn't add any states when adding popovertarget, I don't think we should add a state here.

I suppose that we could say that the type attribute should not be set when the button is the first child of a select or when it has the popovertarget attribute though? Perhaps as a follow-up?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In "type" attribute of button element, the attribute's missing value default and invalid value default are both the Submit Button state.
Therefore, if "type" attribute would be simply prohibited, button element which is child of select element would be Submit Button state.
Because this is not appropriate, shouldn't the fourth state of button element be defined?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defining a fourth button type sounds like it could be potential scope creep, but @mtrootyy does make a good point that it is probably worth noting that this button is not a submit button, and that it also probably shouldn't even allow many of the attributes that would be valid for a button in other contexts.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
<span>inter-element whitespace</span>.</dd>
<dd>If the element has no <code data-x="attr-option-label">label</code> attribute and is a child
of a <code>datalist</code> element: <span data-x="text content">Text</span>.</dd>
<dd>If the element has no <code data-x="attr-option-label">label</code> attribute: Zero or more <span>option element inner content elements</span>.</dd>
<dt><span data-x="concept-element-attributes">Content attributes</span>:</dt>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Content model:" should become the following.

    If the element has a label attribute and a value attribute: Nothing.
    If the element has a label: Text.
    If the element has no value attribute: Text.
    If the element has no label attribute and has a value attribute: Zero or more option element inner content elements.

Because the submitted value must be simply text, and "option element inner content elements" is only used to decorate label text.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I want to force authors to set a value attribute for simple cases like this:

<select>
  <option><svg>...</svg><span>Purple</span></option>
  <option><svg>...</svg><span>Green</span></option>
</select>

In this case, we can just use the text contents of the option, which would be "purple" and "green", just like we already do without the value attribute.

I suppose it would make sense to enforce that there is either a value attribute or some text node descendant of the option. Do you agree?

Copy link

@mtrootyy mtrootyy Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<select name="a">
<option value="2^2"><span>2<sup>2</sup></span></option>
<option value="square root of 3"><math><msqrt><mn>3</mn></msqrt></math></option>
</select>

In the above case, if there is no value attribute, the submitted value will be "22","3".

<select name="b">
<option value="female"><del>male</del>,<span>female</span></option>
<option value="male"><span>male</span>,<del>female</del></option>
</select>

In the above case, if there is no value attribute, the submitted value will be "male,female" regardless of which option is selected.

Because there are many cases like this, shouldn't it be enforce to specify the value attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the author is actually using the value property of the select for anything, then I think they will notice odd cases like this and use the value attribute, just like they are already doing today since it is currently optional.

I could imagine cases today where the author puts non-ascii unicode characters into the option's text, and then has difficulties parsing it later when its submitted to their server in a form. I don't want to start requiring an explicit value attribute in cases like this either.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If value attribute shouldn't be required in this case, shouldn't some kind of note regarding above points be described?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use some examples. Or are we adding those as part of another PR?

source Outdated
<h6>Select element inner content elements</h6>

<p><dfn>Select element inner content elements</dfn> are <code>option</code> elements and other
elements which are used to group and decorate them with a <code>select</code> element.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-wrote it like this: ... are the elements which are allowed as descendants of select elements.

source Outdated Show resolved Hide resolved
source Outdated
@@ -12785,6 +12785,68 @@ https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C%21DOCTYPE%20HTML%3E%
</ul>


<h6>Select element inner content elements</h6>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's rather confusing to have both select element and "Select element".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I should reference <code>select</code> in the <dfn> for this concept, and I didn't make all of the references to it do that either.

Should I do that instead? Or are there other places where I'm using <code>select</code> that should just be "select"?

scottaohara added a commit to w3c/aria that referenced this pull request Oct 22, 2024
In regards to the updated content model for the select element and its allowed children, an `optgroup` can have a `legend` element as its first child, and this `legend` needs to be able to name the `optgroup` similarly to how a `legend` names a `fieldset`.

see:
whatwg/html#10586
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@scottaohara
Copy link
Collaborator

scottaohara commented Oct 30, 2024

This PR removes some important(?) context about option use within / outside of a datalist.
for instance, without mention of datalist, the second condition in the option content model doesn't make sense / doesn't appear to do anything with the text inside of the option:

If the element has a label attribute but no value attribute: Text.

per the above, if i have <option label=foo>test</option> with the current select, all browsers use "foo" as the visible text. But, per the current spec, this makes more sense because other conditions are for use of option as not a child of a datalist. If the option is a child of a datalist, that example renders "test" as the primary label, and "foo" as a sort of sub-text (if rendered at all)... and at least chromium browsers expose that subtext as part of the option...

but again, that only works within a datalist, which would be good to point out (unless we want to do similar for options within a select - which could be useful.....)

Additionally, testing with the current chromium canary build, following the first content model condition results in no visible label for an option

If the element has a label attribute and a value attribute: Nothing.

so <option value=foo label=test> renders as an empty option with the customizable select, but renders "foo" as a visible label with the current select.

if the label attribute is not being used in the customizable select, i think this needs to be clearly stated. (checking/rechecking the definition for option and the label attribute, i don't see this called out?)

but just as importantly, it should also be clear whether new content model allowances are only allowed within options of a styleable select, or if they are allowed for options in a datalist. the current PR reads as if this is a change for everywhere, and i'm not sure if that's really the intent at this point?

Edit: we spoke about some of this in OpenUI yesterday, so just wanted to acknowledge what I wrote here is at least going to be partially addressed.

@annevk annevk added the topic: select The <select> element label Nov 8, 2024
@annevk annevk requested a review from sideshowbarker November 8, 2024 13:14
@josepharhar
Copy link
Contributor Author

This PR removes some important(?) context about option use within / outside of a datalist.

I re-added the datalist cases and added the new content model under the case where there is no label attribute and there is no parent datalist.

Additionally, testing with the current chromium canary build, following the first content model condition results in no visible label for an option

This has been fixed

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
<dt><span data-x="concept-element-contexts">Contexts in which this element can be used</span>:</dt>
<dd>Where <span>phrasing content</span> is expected.</dd>
<dt><span data-x="concept-element-content-model">Content model</span>:</dt>
<dd><span>Phrasing content</span>.</dd>
<dd>If the element is a descendant of an <code>option</code> element: Zero or more <span>option element inner content elements</span>.</dd>
Copy link

@mtrootyy mtrootyy Nov 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this sentence necessary?
If it is necessary here, then it seems necessary for "Content model:" of all of the other element of "Phrasing content" as well as .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to believe that we don't have to patch the content model in so many ways in so many places in order to define what can go in an option element, so yeah I'm in favor of reverting this change. I likely added it in response to other feedback in this PR though so I'd like to find that first.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OP says this resolves #10317 (comment) (among other things) but I don't see that aspect defined here. Where is that defined?

source Outdated Show resolved Hide resolved
@josepharhar
Copy link
Contributor Author

work be done to not render both the text from the label attribute AND the legend - which is what's happening now and seems rather rubbish - especially if someone has to write duplicate / close to duplicate content just so they can put an image into their optgroup label

The text in the legend element replaces the text in the label attribute for both rendering and what's exposed to the accessibility tree. The fact that both the label attribute gets rendered when you have a legend is a bug, I'll fix it here: https://issues.chromium.org/issues/378601807

@scottaohara
Copy link
Collaborator

work be done to not render both the text from the label attribute AND the legend - which is what's happening now and seems rather rubbish - especially if someone has to write duplicate / close to duplicate content just so they can put an image into their optgroup label

The text in the legend element replaces the text in the label attribute for both rendering and what's exposed to the accessibility tree. The fact that both the label attribute gets rendered when you have a legend is a bug, I'll fix it here: https://issues.chromium.org/issues/378601807

good to know. i need to revise the HTML AAM PR for this now, since i was attempting to account for both being rendered until your comment.

@josepharhar
Copy link
Contributor Author

OP says this resolves #10317 (comment) (among other things) but I don't see that aspect defined here. Where is that defined?

I replied on that comment in the issue thread

source Outdated Show resolved Hide resolved
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 25, 2024
This CL adds checks for attributes tabindex and contenteditable of
<option> descendants. Tests are introduced to verify behavior.

Checks based on whatwg/html#10586
Part 3 based on https://chromium-review.googlesource.com/c/chromium/src/+/5906182

Bug: 347890366
Change-Id: Ie4e4e7b655aeacc9d7e95067d17172c7869aa003
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6044294
Commit-Queue: Ana Sollano Kim <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1387905}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: select The <select> element
Development

Successfully merging this pull request may close these issues.

Content model and 'what' to render for stylable <select> elements
5 participants