Track nested <input> in <select> for FilterableSelect [chromium/src : main]
Mason Freed (Gerrit)
Mason Freed added 16 comments![]( "Open in Gerrit")
Overall, I think I like the approach. It'll be interesting to see how this affects general perf, e.g. speedometer. But for now, behind the flag, I think it's ok.
Member<Node> nearest_ancestor_select_child_;`Element` perhaps? Or `ContainerNode` might be better?
Also, these two could use a descriptive comment.
HTMLSelectElement::AssociatedSelectAndOptgroupAndDatalist(*this);I think it'd be good to rename this to indicate that it's a (potentially slow) tree walk. Perhaps `WalkAncestorsForRelatedParts` or something?
(Because I'm actually a bit worried about performance - this will likely negatively impact speedometer, once the flag is enabled. Ok to land now, but heads up for later.)
if (result.select != nearest_ancestor_select_) {Since we're calling this after the element is removed (right?), won't this always be true? Or is this for the case where you remove the entire tree?
void HTMLOptionElement::UpdateAncestors() {Maybe the same comment? E.g. `WalkAncestorsAndUpdate()`?
// num_descendant_inputs_ is redundant with the sum of num_inputs in eachPerhaps it's worth a helper function that only DCHECKs that this var matches the sum of the map contents? (DCHECK because this will be slow.) Then sprinkle calls to this function in interesting places?
// child element. This is used in order to change the structure of the DOM in
// the shadow root and to determine which slot each child node is slotted
// into.You might also include why it's a count and not a bool. (I think the answer is that it helps in tracking removals?)
const HeapHashMap<Member<Node>, DescendantCounts> ChildrenDescendantCounts()This should probably return by reference, not value (copy).
void InputRemoved(HTMLInputElement*, Node* select_child);These could use descriptive comments. And I wonder if the arg should be `nearest_ancestor_select_child`? I'm on the fence about that, so if you prefer as-is, ok.
HTMLSelectElement* select;
HTMLOptGroupElement* optgroup;
HTMLDataListElement* datalist;
Node* select_child;It seems like all of these should be initialized to `nullptr`? Especially if you take my suggestion below to use designated initializers.
node.AddConsoleMessage(PErhaps add a TODO to make this a proper accessibility warning on the Issues pane, with the red squiggly?
const Element& element) {Is it possible to make this non-const, to avoid the const_cast below?
return {nullptr, ancestor_optgroup, nullptr, nullptr};For all of these, I think it'd be a lot more readable (and easier to expand, as in this patch) to use designated initializers:
```
return {.optgroup = ancestor_optgroup};
```
auto insert_result = children_descendant_counts_map_.insert(I don't understand why we're inserting here? There are two paths (input and option) and both of them CHECK that the retrieved value is not what we're inserting here. Seems much more straightforward to just use `children_descendant_counts_map_.at()` to retrieve the value (or CHECK if it's not there) and then just CHECK that one of the two values is `>0`. Either that, or I'm missing something about how this works.
// This has to be run after `insert_result` is destroyed or else a DCHECKThe above comment *might* also fix this situation.
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 15 comments![]( "Open in Gerrit")
Member<Node> nearest_ancestor_select_child_;`Element` perhaps? Or `ContainerNode` might be better?
Also, these two could use a descriptive comment.
i added a comment and changed it to ContainerNode
HTMLSelectElement::AssociatedSelectAndOptgroupAndDatalist(*this);I think it'd be good to rename this to indicate that it's a (potentially slow) tree walk. Perhaps `WalkAncestorsForRelatedParts` or something?
(Because I'm actually a bit worried about performance - this will likely negatively impact speedometer, once the flag is enabled. Ok to land now, but heads up for later.)
Done
if (result.select != nearest_ancestor_select_) {Since we're calling this after the element is removed (right?), won't this always be true? Or is this for the case where you remove the entire tree?
In the case where the "entire tree" above the select element is removed, then yeah result.select will still be the same as nearest_ancestor_select_.
Member<Node> nearest_ancestor_select_child_;Joey Arharditto
Done
void HTMLOptionElement::UpdateAncestors() {Maybe the same comment? E.g. `WalkAncestorsAndUpdate()`?
Done
// num_descendant_inputs_ is redundant with the sum of num_inputs in eachPerhaps it's worth a helper function that only DCHECKs that this var matches the sum of the map contents? (DCHECK because this will be slow.) Then sprinkle calls to this function in interesting places?
Done
// child element. This is used in order to change the structure of the DOM in
// the shadow root and to determine which slot each child node is slotted
// into.You might also include why it's a count and not a bool. (I think the answer is that it helps in tracking removals?)
Done
const HeapHashMap<Member<Node>, DescendantCounts> ChildrenDescendantCounts()This should probably return by reference, not value (copy).
Done
void InputRemoved(HTMLInputElement*, Node* select_child);These could use descriptive comments. And I wonder if the arg should be `nearest_ancestor_select_child`? I'm on the fence about that, so if you prefer as-is, ok.
Done
HTMLSelectElement* select;
HTMLOptGroupElement* optgroup;
HTMLDataListElement* datalist;
Node* select_child;It seems like all of these should be initialized to `nullptr`? Especially if you take my suggestion below to use designated initializers.
Done
node.AddConsoleMessage(PErhaps add a TODO to make this a proper accessibility warning on the Issues pane, with the red squiggly?
Done
const Element& element) {Is it possible to make this non-const, to avoid the const_cast below?
Removing const from this would require also removing const from OwnerSelectElement on a bunch of classes, which cascades into removing const from a lot of places, including HTMLOptGroupElement::SupportsFocus for example.
Adding const to the returned struct also causes issues when the returned node is used to call other things such as HTMLSelectElement::OptionInserted.
I think we should keep this const_cast, do you agree?
return {nullptr, ancestor_optgroup, nullptr, nullptr};For all of these, I think it'd be a lot more readable (and easier to expand, as in this patch) to use designated initializers:
```
return {.optgroup = ancestor_optgroup};
```
Done
auto insert_result = children_descendant_counts_map_.insert(I don't understand why we're inserting here? There are two paths (input and option) and both of them CHECK that the retrieved value is not what we're inserting here. Seems much more straightforward to just use `children_descendant_counts_map_.at()` to retrieve the value (or CHECK if it's not there) and then just CHECK that one of the two values is `>0`. Either that, or I'm missing something about how this works.
makes sense, i replaced the insert with a find
// This has to be run after `insert_result` is destroyed or else a DCHECKThe above comment *might* also fix this situation.
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
// num_descendant_inputs_ is redundant with the sum of num_inputs in eachJoey ArharPerhaps it's worth a helper function that only DCHECKs that this var matches the sum of the map contents? (DCHECK because this will be slow.) Then sprinkle calls to this function in interesting places?
Done
i actually did this earlier, and now that ive re-added it, i remembered why i removed it - its so slow that it makes one of the select tests time out (the test already has <meta name="timeout" content="long">).
I moved it to the getter for the number of descendant inputs, hopefully that performs better and provides the same benefit.
Related details
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 5 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
5 comments
LGTM, but do fix up the `.find` - replace with the `.at`.
// num_descendant_inputs_ is redundant with the sum of num_inputs in eachJoey ArharPerhaps it's worth a helper function that only DCHECKs that this var matches the sum of the map contents? (DCHECK because this will be slow.) Then sprinkle calls to this function in interesting places?
Joey ArharDone
i actually did this earlier, and now that ive re-added it, i remembered why i removed it - its so slow that it makes one of the select tests time out (the test already has <meta name="timeout" content="long">).
I moved it to the getter for the number of descendant inputs, hopefully that performs better and provides the same benefit.
Wow! Ok, good reason not to do it I guess. Thanks for trying.
const Element& element) {Joey ArharIs it possible to make this non-const, to avoid the const_cast below?
Removing const from this would require also removing const from OwnerSelectElement on a bunch of classes, which cascades into removing const from a lot of places, including HTMLOptGroupElement::SupportsFocus for example.
Adding const to the returned struct also causes issues when the returned node is used to call other things such as HTMLSelectElement::OptionInserted.
I think we should keep this const_cast, do you agree?
I wish const weren't so viral. Yeah, given what you said, this is ok.
children_descendant_counts_map_.at(nearest_ancestor_select_child);This is extra, and I think it can be used instead of the find, right?
unsigned num_inputs_in_map = 0;Maybe just for future-safety, wrap this block in `{}` so we don't accidentally use any of it later.
BTW, this is good. This is where I was thinking about DCHECKing anyway.
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 2 comments![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
| Commit-Queue | +2 |
2 comments
children_descendant_counts_map_.at(nearest_ancestor_select_child);This is extra, and I think it can be used instead of the find, right?
Whoops, don't know how that got there and I removed it. I guess it would be functionally the same since at() will crash if the value isn't in the map and we are explicitly CHECKing the same thing...
I like find() more because it allows for erasing the value at the end of the method without the need to give it a key type and do another lookup.
Maybe just for future-safety, wrap this block in `{}` so we don't accidentally use any of it later.
BTW, this is good. This is where I was thinking about DCHECKing anyway.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Track nested <input> in <select> for FilterableSelect
Currently, the only way to use FilterableSelect was to create a
sibling <input> and listbox <select> element and connect them with
idrefs, like this:
<input filter=select>
<select id=select size=4>
This patch starts the work to make it possible to nest an <input> inside
a <select> to make filtering work like this:
<select>
<input>
In order to dynamically switch the DOM of the shadowroot between the
existing structure and the new structure outlined above, and to slot
each child of the select element into the correct slot, this patch adds
logic to the InsertedInto and RemovedFrom methods on HTMLOptionElement
and HTMLInputElement to tell their ancestor select element which one of
the select's children had a descendant option or input added to it.
HTMLSelectElement now stores a map of each of its child nodes to the
number of options and input element descendants (inclusive) of that
child.
- M third_party/blink/renderer/core/html/forms/html_input_element.cc
- M third_party/blink/renderer/core/html/forms/html_input_element.h
- M third_party/blink/renderer/core/html/forms/html_opt_group_element.cc
- M third_party/blink/renderer/core/html/forms/html_option_element.cc
- M third_party/blink/renderer/core/html/forms/html_option_element.h
- M third_party/blink/renderer/core/html/forms/html_select_element.cc
- M third_party/blink/renderer/core/html/forms/html_select_element.h
- M third_party/blink/renderer/core/html/forms/html_select_element_test.cc
- M third_party/blink/renderer/core/html/forms/option_list.cc
- M third_party/blink/renderer/core/html/html_hr_element.cc
Code-Review: +1 by Mason Freed, +1 by Joey Arhar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |