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

Define the <selectedcontent> element #10633

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

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented Sep 18, 2024

The <selectedcontent> element is part of the customizable <select> proposal: #9799

It allows authors to declaratively clone the contents of the currently selected <option> of a <select> and style it independently for use in a base appearance <select>'s button.

The timing of cloning has been discussed here:
#10520

The selectedcontent element has been discussed generally here: w3c/csswg-drafts#10242

html-aria PR: w3c/html-aria#528
html-aam PR: w3c/html-aam#566

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


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

The `<selectedoption>` element is part of the customizable `<select>`
proposal: whatwg#9799

It allows authors to declaratively clone the contents of the currently
selected `<option>` of a `<select>` and style it independently for use
in a base appearance `<select>`'s button.

The timing of cloning has been discussed here:
whatwg#10520

The selectedoption element has been discussed generally here:
w3c/csswg-drafts#10242
@annevk
Copy link
Member

annevk commented Sep 19, 2024

It seems this is missing a lot of the boilerplate that new elements normally have as well as changes to content models, indexes, etc. See also this checklist at the top of source:

 !   Adding a new element involves editing the following sections:
 !    - section for the element itself
 !    - descriptions of the element's categories
 !    - images/content-venn.svg
 !    - syntax, if it's void or otherwise special
 !    - parser, if it's not phrasing-level
 !    - rendering
 !    - obsolete section
 !    - element, attribute, content model, and interface indices

scottaohara added a commit to w3c/aria that referenced this pull request Sep 27, 2024
I recreated the [original PR](w3c/html-aam#566) by @josepharhar

The `<selectedoption>` element is part of the [customizable select feature](whatwg/html#9799) and is being added to HTML [here](whatwg/html#10633).

## Implementation

* WPT tests: web-platform-tests/wpt#45096
* Implementations (link to issue or when done, link to commit):
   * WebKit: TODO
   * Gecko: TODO
   * Blink: https://chromium.googlesource.com/chromium/src/+/18b5eac27b14b409503aa8047cf9358082a0e0df

Co-authored-by: Joey Arhar @josepharhar
@josepharhar
Copy link
Contributor Author

Thanks!

  • I added a dl with a bunch of metadata for the element itself.
  • I didn't include this element in any categories
  • I didn't update the content-venn image because I didn't add it to any categories.
  • I didn't update syntax or parsing because it doesn't have any special rules and I didn't implement anything in the HTML parser for this.
  • I didn't update rendering because I noticed that elements like <div> and <span> don't have sections in there, and selectedoption is supposed to render like those elements.
  • I didn't update the obsolete section because nothing is becoming obsolete...?
  • I updated the element and interface indexes

@annevk
Copy link
Member

annevk commented Oct 4, 2024

This still seems incomplete. Where does the button element indicate this element can be a descendant, for instance? If you don't adjust the categories, you still need to account for how the categories are used.

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

This still seems incomplete. Where does the button element indicate this element can be a descendant, for instance? If you don't adjust the categories, you still need to account for how the categories are used.

Thanks, I updated the content model of the button element

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
@josepharhar
Copy link
Contributor Author

I just added a "disabled" concept to help prevent infinite loops during cloning due to putting a selectedcontent element inside multiple select elements, another selectedcontent element, or an option element.

@josepharhar
Copy link
Contributor Author

I made the select.value and select.selectedIndex setters trigger cloning to follow this resolution: openui/open-ui#1119 (comment)

Copy link
Member

@dbaron dbaron left a comment

Choose a reason for hiding this comment

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

A few comments from looking at parts of this:

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

In response to feedback from @dbaron, I made the one selectedcontent element which receives updates be the first one in tree order instead of the first one to be inserted. I also removed the select descendant selectedoption elements list.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 20, 2024
Since we are only planning to support one <selectedcontent> element in
<select>, this patch only performs a clone into the first
<selectedcontent> in tree order.

This is in response to feedback here:
whatwg/html#10633 (comment)

This patch also removes the logic which clears <selectedcontent>
elements when they are removed/disconnected.

Change-Id: I1580ec9f12df463d1f5134905e3e527cfefa694d
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 20, 2024
Since we are only planning to support one <selectedcontent> element in
<select>, this patch only performs a clone into the first
<selectedcontent> in tree order.

This is in response to feedback here:
whatwg/html#10633 (comment)

This patch also removes the logic which clears <selectedcontent>
elements when they are removed/disconnected.

Change-Id: I1580ec9f12df463d1f5134905e3e527cfefa694d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6043186
Reviewed-by: David Baron <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1399301}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 20, 2024
Since we are only planning to support one <selectedcontent> element in
<select>, this patch only performs a clone into the first
<selectedcontent> in tree order.

This is in response to feedback here:
whatwg/html#10633 (comment)

This patch also removes the logic which clears <selectedcontent>
elements when they are removed/disconnected.

Change-Id: I1580ec9f12df463d1f5134905e3e527cfefa694d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6043186
Reviewed-by: David Baron <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1399301}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 8, 2025
…ent> element, a=testonly

Automatic update from web-platform-tests
Only update first appended <selectedcontent> element

Since we are only planning to support one <selectedcontent> element in
<select>, this patch only performs a clone into the first
<selectedcontent> in tree order.

This is in response to feedback here:
whatwg/html#10633 (comment)

This patch also removes the logic which clears <selectedcontent>
elements when they are removed/disconnected.

Change-Id: I1580ec9f12df463d1f5134905e3e527cfefa694d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6043186
Reviewed-by: David Baron <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1399301}

--

wpt-commits: 2331838b392c5802a97e5623769e0ad0b7b7102f
wpt-pr: 49804
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jan 10, 2025
…ent> element, a=testonly

Automatic update from web-platform-tests
Only update first appended <selectedcontent> element

Since we are only planning to support one <selectedcontent> element in
<select>, this patch only performs a clone into the first
<selectedcontent> in tree order.

This is in response to feedback here:
whatwg/html#10633 (comment)

This patch also removes the logic which clears <selectedcontent>
elements when they are removed/disconnected.

Change-Id: I1580ec9f12df463d1f5134905e3e527cfefa694d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6043186
Reviewed-by: David Baron <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1399301}

--

wpt-commits: 2331838b392c5802a97e5623769e0ad0b7b7102f
wpt-pr: 49804
sadym-chromium pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 14, 2025
Since we are only planning to support one <selectedcontent> element in
<select>, this patch only performs a clone into the first
<selectedcontent> in tree order.

This is in response to feedback here:
whatwg/html#10633 (comment)

This patch also removes the logic which clears <selectedcontent>
elements when they are removed/disconnected.

Change-Id: I1580ec9f12df463d1f5134905e3e527cfefa694d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6043186
Reviewed-by: David Baron <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1399301}
@hsivonen
Copy link
Member

Adding a proper backlink to: mozilla/standards-positions#1142

I'm worried about breaking the implied invariant that the parser creates exactly one element node per (possibly implied) non-erroneous start tag. I don't at this time have a concrete end-to-end scenario of what badness exactly may ensue, but it's a bad sign about breaking an invariant leading to issues that then need addressing that there's already a remark about preventing infinite loops upthread and a separate issue about the timing of cloning (#10520).

In the error handling case where one element per start tag does not hold, the spec says "Create an element for the token for which the element node was created, in the HTML namespace" instead of cloning a formatting element. This is in response to https://www.w3.org/Bugs/Public/show_bug.cgi?id=6743 , which was motivated by avoidance of data flows from the live DOM into the tree builder algorithm. However, it looks like the option cloning as proposed could happen on the main thread in a way that does not affect the subsequent decisions of the tree builder algorithm.

Do I understand correctly that cloning into the regular DOM instead of a UA shadow DOM came from w3c/csswg-drafts#10242 (comment) ?

@josepharhar
Copy link
Contributor Author

Do I understand correctly that cloning into the regular DOM instead of a UA shadow DOM came from w3c/csswg-drafts#10242 (comment) ?

Yes. We had many lengthy discussions about this, since cloning into shadow DOM avoids a few problems. But it also introduced a myriad of issues that were much more serious than the ones posed by cloning into regular DOM.

source Show resolved Hide resolved
source Show resolved Hide resolved

<li><p>The first <code>option</code> element in the <span
data-x="concept-select-option-list">list of options</span>, in <span>tree order</span>, whose
<span data-x="concept-option-value">value</span> is equal to the given new value, if any, must
Copy link
Member

Choose a reason for hiding this comment

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

Is it just a straight-up equal comparison we want? Chromium seems to do some stuff with whitespace, do we need to do the same here?

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 chromium code there which does stuff with whitespace is accounted for in the option's value here (option value uses option text in the spec): https://html.spec.whatwg.org/#dom-option-text

Assigning to select.value doesn't seem to do anything special with whitespace: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_select_element.cc;l=636-671;drc=fc2575ac635633b2972b49eec663250ac05a1a53

Copy link
Member

Choose a reason for hiding this comment

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

Ah OK got it. So I don't think anything is required here then. Can we resolve this?

source Show resolved Hide resolved
source Show resolved Hide resolved

<p><code>selectedcontent</code> elements become associated with a <code>select</code> element when
the <code>selectedcontent</code> is a <span>descendant</span> of the <code>select</code>'s first
child <code>button</code>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

But the "get a select's enabled selectedcontent" algorithm doesn't limit its search to the select element's first child button. Is this out of date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I pretty much just said this because it is meant to be used inside the button. I removed this paragraph.

Copy link
Member

Choose a reason for hiding this comment

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

Great, this can be resolved right? (Just want to double check with you instead of resolving it myself)

<li><p>Let <var>documentFragment</var> be the result of running <span>convert nodes into a
node</span> given <var>nodes</var> and <var>option</var>'s <span>node document</span>.</p></li>

<li><p><span>Ensure pre-insertion validity</span> of <var>documentFragment</var> into
Copy link
Member

Choose a reason for hiding this comment

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

Now that I think about it, do we actually have to call this? Are there any circumstances where this could actually throw an exception? The only one that stands out to me is if nodes contains a doctype, but I don't think that's even possible, right? How does the Chromium impl look here? Are there any known cases where we see an exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah because an author can call $0.replaceChildren($0.parentNode) which would trip the second pre-insertion validity condition and throw an exception. I don't think we can run into that in our case, since we definitely aren't ever trying to insert parents into their children, right?

So the real question is about whether we need this call for the other pre-insertion conditions. I don't think we can ever trip those either though.

We are also calling it within ReplaceChildren in chromium I think: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_selected_content_element.cc;l=35;drc=8c797f761029413678dc8d9c2308d78336046ab8

OK this is great to see. Note that we're asserting no exception in the impl, meaning we're guaranteeing that all pre-insertion checks will pass, and never rseult in an error. So I think we can remove this from the spec.

source Show resolved Hide resolved
source Show resolved Hide resolved

<ol>
<li><p>If <var>nearestSelectAncestor</var> is null, then set <var>nearestSelectAncestor</var>
to <var>select</var>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

So the Chromium implementation seems to unconditionally assign <var>nearestSelectAncestor</var> in this case, not limiting assignment to the non-null case. That seems to not matter in the implementation since the last two steps of this algorithm that actually use the nearest select ancestor don't appear the impl. What am I missing?

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 difference here is that chromium's select elements still keep a list of descendant selectedcontent elements as a performance optimization which I removed from this PR earlier, and all ancestor selects get this selectedcontent element added to their list of descendants, even when there are nested select elements.

The implementation doesn't seem to correctly remove them in the corresponding HTMLSelectedContentElement::RemovedFrom, so I'm going to change the implementation to match the spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Cool! I think this can be resolved now then, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: forms topic: select The <select> element
Development

Successfully merging this pull request may close these issues.

7 participants