Update shadow root for nesting <input> in <select> [chromium/src : main]
Joey Arhar (Gerrit)
Joey Arhar voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 15 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
15 comments
LGTM! Lots of comments, but most are small
return content_model_violations_count_ > 0U;nit: just `return content_model_violations_count_;`
UserAgentShadowRoot()->SetNeedsAssignmentRecalc();Why is this needed? I'd have thought that the node removal at line 1567 would be enough. Is it because something is missing on ShadowRoot for this, perhaps? E.g. we have [Element::ChildrenChanged()](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/element.cc;l=7824;drc=3da6fd070ba13b34eba5bc5bb3fd0bb6c2d1797f) which takes care of this for normal removals.
if (num_descendant_inputs_ == 1) {== 1 ? I'm guessing this is because we don't need to change the shadow structure after 1. Perhaps add a comment?
if (num_descendant_inputs_ == 0) {nit: `!num_descendant_inputs_`
UserAgentShadowRoot()->SetNeedsAssignmentRecalc();Again, here, it feels weird to have to manually call this. Do we know why it's needed?
UserAgentShadowRoot()->SetNeedsAssignmentRecalc();ditto
EXPECT_TRUE(!!input_slot());It feels a little weird to validate that the UpdateStyleAndLayout creates it. Maybe ok to leave, but it'll break if, e.g. we happen to update style and layout from the callbacks that happen above this point.
// <div role=listbox>uber-nit: close this `</div>`
popover_input_slot_ = nullptr;Maybe `CHECK(!select_->NumDescendantInputs())` ?
auto it = select_->ChildrenDescendantCounts().find(&child);Perhaps `DCHECK(!!popover_input_slot_)` ?
if (it->value.num_inputs && !it->value.num_options) {Maybe add a comment that the other situation (we have both) is handled elsewhere?
popover_input_slot_->Assign(children_with_descendant_input);Perhaps `DCHECK(RuntimeEnabledFeatures::FilterableSelectEnabled())`
children_with_descendant_input.push_back(child);Ditto my comments from above here and below.
if (slot.IsInUserAgentShadowRoot() &&
IsA<HTMLSelectElement>(slot.OwnerShadowHost()) &&
To<HTMLSelectElement>(slot.OwnerShadowHost())->UsesMenuList() &&
slot.GetIdAttribute() == shadow_element_names::kSelectOptions) {
return true;
}nit:
```
if (auto select = DynamicTo<HTMLSelectElement>(slot.OwnerShadowHost());
select && select->UsesMenuList() &&
slot.IsInUserAgentShadowRoot() &&
slot.GetIdAttribute() == shadow_element_names::kSelectOptions) {
return true;
}
```
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 14 comments![]( "Open in Gerrit")
return content_model_violations_count_ > 0U;Joey Arharnit: just `return content_model_violations_count_;`
Done
Why is this needed? I'd have thought that the node removal at line 1567 would be enough. Is it because something is missing on ShadowRoot for this, perhaps? E.g. we have [Element::ChildrenChanged()](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/element.cc;l=7824;drc=3da6fd070ba13b34eba5bc5bb3fd0bb6c2d1797f) which takes care of this for normal removals.
You're right, this isn't needed and I removed it.
I originally added it while debugging something and there was another change which actually ended up fixing the underlying issue but this was leftover.
== 1 ? I'm guessing this is because we don't need to change the shadow structure after 1. Perhaps add a comment?
Done
if (num_descendant_inputs_ == 0) {Joey Arharnit: `!num_descendant_inputs_`
Done
Again, here, it feels weird to have to manually call this. Do we know why it's needed?
i added a comment and added test coverage which will fail if this is removed
UserAgentShadowRoot()->SetNeedsAssignmentRecalc();Joey Arharditto
Done
EXPECT_TRUE(!!input_slot());It feels a little weird to validate that the UpdateStyleAndLayout creates it. Maybe ok to leave, but it'll break if, e.g. we happen to update style and layout from the callbacks that happen above this point.
UpdateStyleAndLayout does not create this slot element, all the checks for its existence are before a call to UpdateStyleAndLayout. The Calls to UpdateStyleAndLayout are just to run slot assignment.
// <div role=listbox>Joey Arharuber-nit: close this `</div>`
Done
popover_input_slot_ = nullptr;Joey ArharMaybe `CHECK(!select_->NumDescendantInputs())` ?
Done
auto it = select_->ChildrenDescendantCounts().find(&child);Joey ArharPerhaps `DCHECK(!!popover_input_slot_)` ?
Done
Maybe add a comment that the other situation (we have both) is handled elsewhere?
Done
popover_input_slot_->Assign(children_with_descendant_input);Joey ArharPerhaps `DCHECK(RuntimeEnabledFeatures::FilterableSelectEnabled())`
Done
Ditto my comments from above here and below.
Done
if (slot.IsInUserAgentShadowRoot() &&
IsA<HTMLSelectElement>(slot.OwnerShadowHost()) &&
To<HTMLSelectElement>(slot.OwnerShadowHost())->UsesMenuList() &&
slot.GetIdAttribute() == shadow_element_names::kSelectOptions) {
return true;
}nit:
```
if (auto select = DynamicTo<HTMLSelectElement>(slot.OwnerShadowHost());
select && select->UsesMenuList() &&
slot.IsInUserAgentShadowRoot() &&
slot.GetIdAttribute() == shadow_element_names::kSelectOptions) {
return true;
}
```
- 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. |