[SelectMenu] Extend HTMLOptionsCollection to support <selectmenu>. [chromium/src : main]
Ionel Popescu (Gerrit)
Attention is currently required from: Dan Clark, Mason Freed.
Ionel Popescu would like Dan Clark and Mason Freed to review this change.
[SelectMenu] Extend HTMLOptionsCollection to support <selectmenu>.
Add support for a readonly 'options' attribute on <selectmenu>.
As we plan to remove support for part="option" and allow only <option>
elements to be used as options, the HTMLOptionsCollection is extended to
support <selectmenu>.
Since shadow tree traversal is needed to build selectmenu.options,
FlatTreeTraversal is extended to receive a matcher that is used to
filter the returned elements. Same approach is used for DOM tree
traversal[1].
[1]: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/element_traversal.h
Bug: 1191131
Change-Id: If2730faaf8ea117fa01fcc148fdd56b2e8146d85
---
M third_party/blink/renderer/core/dom/flat_tree_traversal.h
M third_party/blink/renderer/core/html/collection_type.h
M third_party/blink/renderer/core/html/forms/html_options_collection.cc
M third_party/blink/renderer/core/html/forms/html_options_collection.h
M third_party/blink/renderer/core/html/forms/html_select_menu_element.cc
M third_party/blink/renderer/core/html/forms/html_select_menu_element.h
M third_party/blink/renderer/core/html/forms/html_select_menu_element.idl
M third_party/blink/renderer/core/html/html_collection.cc
M third_party/blink/web_tests/external/wpt/html/semantics/forms/the-selectmenu-element/selectmenu-parts-structure.tentative.html
9 files changed, 178 insertions(+), 9 deletions(-)
To view, visit change 3055671. To unsubscribe, or for help writing mail filters, visit settings.
Ionel Popescu (Gerrit)
Attention is currently required from: Dan Clark, Mason Freed.
Patch set 2:Commit-Queue +1
1 comment:
Patchset:
PTAL, thanks!
To view, visit change 3055671. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Attention is currently required from: Dan Clark, Mason Freed.
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/29797.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Dan Clark (Gerrit)
Attention is currently required from: Ionel Popescu, Mason Freed.
1 comment:
File third_party/blink/renderer/core/dom/flat_tree_traversal.h:
while (node && !is_match(*node))
node = FlatTreeTraversal::Next(*node, stay_within);I'm not sure that this approach is going to be sufficient for all cases where we need to skip over nested <selectmenu>/<select>s in the parts search.
Consider cases like this:
<selectmenu>
<selectmenu>
<option>one</option>
</selectmenu>
<option>two</option>
</selectmenu>The outer <selectmenu> should only see <option> "two" as a valid option part, because "one" is in a nested <selectmenu>. But this function won't do so; it will continue into the subtree of elements that don't pass the is_match() check.
This is in contrast to SelectMenuPartTraversal in https://chromium-review.googlesource.com/c/chromium/src/+/3053078, which skips over subtrees of nested <selectmenu>/<select>.
I don't see a good way to handle this with the MatchFunc pattern, so we might need something like SelectMenuPartTraversal after all. We make FlatTreeTraversal use MatchFunc differently from ElementTraversal, but that seems potentially confusing.
To view, visit change 3055671. To unsubscribe, or for help writing mail filters, visit settings.
Ionel Popescu (Gerrit)
Attention is currently required from: Dan Clark, Mason Freed.
1 comment:
File third_party/blink/renderer/core/dom/flat_tree_traversal.h:
while (node && !is_match(*node))
node = FlatTreeTraversal::Next(*node, stay_within);
I'm not sure that this approach is going to be sufficient for all cases where we need to skip over n […]
Indeed, there a different behavior where the FlatTreeTraversal / ElementTraversal traverse the entire tree and uses the MatchFunc to filter the elements, as opposed to SelectMenuPartTraversal that skips over subtrees.
I think there are 2 solutions here:
1. adjust the MatchFunc to ensure that the parts are not part of a nested <selectmenu/select>.
2. keep SelectMenuPartTraversal and add a comment on why it is needed.I think we may find that (1) has a slightly higher complexity due to many corner cases so we should probably pursue (2).
To view, visit change 3055671. To unsubscribe, or for help writing mail filters, visit settings.
Dan Clark (Gerrit)
Attention is currently required from: Ionel Popescu, Mason Freed.
1 comment:
File third_party/blink/renderer/core/html/forms/html_select_menu_element.cc:
Patch Set #4, Line 470:
EnsureCachedCollection<HTMLOptionsCollection>(kSelectMenuOptions);I've been thinking about this change some more, and it's a little suspicious that HTMLSelectMenuElement is going to have two separate collections of its options: the HTMLOptionsCollection created here, and HTMLSelectMenuElement::option_parts_.
It would be great if we could just get rid of HTMLSelectMenuElement::option_parts_ and use this HTMLOptionsCollection instead. However we've been using in OptionPartInserted/OptionPartRemoved in order to keep track of whether we've already registered the OptionPartEventListener option_part_listener_ once on each option part. We might also need to keep track of similar things for options in the future, that need to be set when an option is registered as a part and unset when it stops being a part.
I don't know how to make that work with HTMLCollection collections, since they are Ensured on-demand. I don't see a good way to know whether a given element in the HTMLOptionsCollection has already been processed in OptionPartInserted or not.
But, HTMLOptionElement already has is_descendant_of_select_menu_, so could we check that, instead of checking whether an option is present in option_parts_? (Although we might need to upgrade from a bool to a HTMLSelectMenuElement pointer, since there might be bugs when an <option> is moved directly from one <selectmenu> to another).
Or alternatively, could we instead invent a new class that exposes HTMLSelectMenuElement::option_parts_ with the HTMLOptionsCollection IDL interface, instead of using the EnsureCachedCollection stuff?
To view, visit change 3055671. To unsubscribe, or for help writing mail filters, visit settings.
Domenic Denicola (Gerrit)
Patchset:
Hey, just popping in after seeing this exported to WPT: Can I suggest not doing this, or perhaps having a standards discussion first? HTMLOptionsCollection is a legacy "fake array" and I don't think we would accept any HTML Standard PRs that continue spreading it throughout the platform. New features should use ObservableArray, if anything. (Although IMO I don't think an options property is needed at all.)
To view, visit change 3055671. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Ionel Popescu.
1 comment:
Patchset:
Hey, just popping in after seeing this exported to WPT: Can I suggest not doing this, or perhaps hav […]
I'll wait to review this CL until this comment gets discussed, and also some of the related comments in https://chromium-review.googlesource.com/c/chromium/src/+/3053078. Sound ok?
To view, visit change 3055671. To unsubscribe, or for help writing mail filters, visit settings.
Ionel Popescu (Gerrit)
1 comment:
Patchset:
I'll wait to review this CL until this comment gets discussed, and also some of the related comments […]
I am going to put this CL on hold until there is a decision for: https://github.com/openui/open-ui/issues/380. Thanks!
To view, visit change 3055671. To unsubscribe, or for help writing mail filters, visit settings.
Ionel Popescu (Gerrit)
Ionel Popescu abandoned this change.
To view, visit change 3055671. To unsubscribe, or for help writing mail filters, visit settings.