Convert <DETAILS> to manual assignent. [chromium/src : main]
Aleks Totic (Gerrit)
Attention is currently required from: Mason Freed.
Aleks Totic would like Mason Freed to review this change.
Convert <DETAILS> to manual assignent.
Some user agent custom elements used non-standard slot assignment.
Convert it to manual slot assignment.
Elements that want to do manual assignment:
- Implement Element::ManuallyAssignSlots method
- SetSlotAssignmentMode(SlotAssignmentMode::kManual);
First implementation: details element.
Bug: 1179356
Change-Id: Ida73ae8436f78da134b3b5414a1e4f972983dd63
---
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/core/dom/slot_assignment.cc
M third_party/blink/renderer/core/html/html_details_element.cc
M third_party/blink/renderer/core/html/html_details_element.h
M third_party/blink/renderer/core/html/html_slot_element.cc
M third_party/blink/renderer/core/html/html_slot_element.h
6 files changed, 86 insertions(+), 12 deletions(-)
To view, visit change 3163573. To unsubscribe, or for help writing mail filters, visit settings.
Aleks Totic (Gerrit)
Attention is currently required from: Mason Freed.
1 comment:
Patchset:
wdyt?
I did my original idea where manual reassignment happens in RecalcAssignments, and it seems to work.
What I like about it is that it reuses existing invalidation.
To view, visit change 3163573. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aleks Totic.
8 comments:
Patchset:
Ok, so I do like this approach after all. Looks pretty clean! I'm still a bit concerned that there will be some gotcha around the timing of slot recalculation. But so far not showing up in tests, so I'm happy!
File third_party/blink/renderer/core/dom/slot_assignment.cc:
if (owner_->IsUserAgent() && owner_->IsManualSlotting()) {
owner_->host().ManuallyAssignSlots();
}Should this happen above the AX block just above? I'm wondering what happens if the ManuallyAssignSlots() code happens to add/change slots within the shadow root. I don't feel like the AX tests fully cover this kind of thing. We could also try to outlaw slot changes within ManuallyAssignSlots() instead, if it's a problem? (Also, none of this could be a problem, just thinking about it.)
Patch Set #1, Line 390:
&& !owner_->IsManualSlotting()Seems like this should be a DCHECK inside the if() block. If !supportsNameBased, then it *must* be a UA shadow root, and if we enable manual slotting on a UA root, we shouldn't also enable name based slot assignment. Right?
File third_party/blink/renderer/core/html/html_details_element.cc:
Patch Set #1, Line 151:
fast/dom/shadow/offset-parent-does-not-leak-ua-shadow.htmThat's awful! Let's not add traversal code where we don't need it, just for a test. Nothing other than this class can *normally* manipulate the shadow content, so let's assume it's in the right place and DCHECK if it isn't. And then modify the test to do something more sane.
Even better - would it make sense to just store a reference to the summary and content slots on the object, and then you don't even need to "know" that they're first and second children?
Patch Set #1, Line 174:
if (IsFirstSummary(child)) {Since this can only happen once, perhaps summary_nodes can just be a single summary_node?
Patch Set #1, Line 180:
if (summary_slot) {Same comment on these two - both slots should *always* exist, right?
File third_party/blink/renderer/core/html/html_slot_element.h:
Patch Set #1, Line 119:
assign() c++ implementation.Thanks for adding comments like this - always helpful.
File third_party/blink/renderer/core/html/html_slot_element.cc:
void HTMLSlotElement::assign(HeapVector<Member<V8UnionElementOrText>> js_nodes,
ExceptionState&) {
UseCounter::Count(GetDocument(), WebFeature::kSlotAssignNode);
if (js_nodes.IsEmpty() && manually_assigned_nodes_.IsEmpty())
return;
HeapVector<Member<Node>> nodes;
for (V8UnionElementOrText* union_node : js_nodes) {I like this cleanup. Might be a bit slower, but this code path shouldn't be hot.
To view, visit change 3163573. To unsubscribe, or for help writing mail filters, visit settings.
Aleks Totic (Gerrit)
Attention is currently required from: Mason Freed.
6 comments:
Patchset:
Everything but the fix for the UserAgentDOM manipulation test
File third_party/blink/renderer/core/dom/slot_assignment.cc:
if (owner_->IsUserAgent() && owner_->IsManualSlotting()) {
owner_->host().ManuallyAssignSlots();
}
Should this happen above the AX block just above? I'm wondering what happens if the ManuallyAssignSl […]
I believe the existing code is in the correct place.
The AX code invalidates children of the existing slots.
I believe that manual reassignment should happen after invalidation.
Patch Set #1, Line 390:
&& !owner_->IsManualSlotting()
Seems like this should be a DCHECK inside the if() block. […]
I am confused here.
not name based implies ShadowRoot.
But, shadowRoot without manual slotting still uses old-style custom/default slots.Once we remove custom/default slots completely, these two lines go away completely.
File third_party/blink/renderer/core/html/html_details_element.cc:
Patch Set #1, Line 151:
fast/dom/shadow/offset-parent-does-not-leak-ua-shadow.htm
That's awful! Let's not add traversal code where we don't need it, just for a test. […]
Modifying that test is tricky. For test to work, user agent shadow dom must have a slot inside a div.
Details does not. Do we have any other user agent elements, where slot has a parent?I've thought about holding onto the references as WeakMembers. But, holding does have it costs in that you have to keep them in sync if anything changes.
I think: first node is is summary, 2nd node is details is clean enough. If ShadowDOM was more complex, holding onto nodes would have more appeal.
Patch Set #1, Line 174:
if (IsFirstSummary(child)) {Since this can only happen once, perhaps summary_nodes can just be a single summary_node?
But I still need an array to pass to Assign()
Patch Set #1, Line 180:
if (summary_slot) {Same comment on these two - both slots should *always* exist, right?
No problem. If I assume nothing other then this class manipulates shadow DOM, use case becomes easier.
To view, visit change 3163573. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aleks Totic.
5 comments:
File third_party/blink/renderer/core/dom/slot_assignment.cc:
if (owner_->IsUserAgent() && owner_->IsManualSlotting()) {
owner_->host().ManuallyAssignSlots();
}
I believe the existing code is in the correct place. […]
👍
Patch Set #1, Line 390:
&& !owner_->IsManualSlotting()
I am confused here. […]
See [1]. If !SupportsNameBasedSlotAssignment(), then it's a) definitely a *UA* Shadow root, and b) nobody set the supports_name_based flag, which means it's an "old style" UA slot assignment. My point would be that it would therefore be a bug to set the ManualSlotting flag in this case. So we should convert the check for !IsManualSlotting to a DCHECK.
File third_party/blink/renderer/core/html/html_details_element.cc:
Patch Set #1, Line 151:
fast/dom/shadow/offset-parent-does-not-leak-ua-shadow.htm
Modifying that test is tricky. […]
Hmm, on the test, what about changing from a <details> element to just a plain <div>, but use Internals::createUserAgentShadowRoot() to attach a UA shadow root, and then do the rest of the test? Side-note: the more we make UA shadow DOM just like regular user shadow dom, the less we need tests like this.
Re: holding references to the slots - they don't have to even be WeakMembers, they can be plain Members. The HTMLDetailsElement owns its shadow dom, so it can own any references to nodes within the shadow dom. Remember that no outside code should be able to see or modify the UA shadow dom contents. So the slots would have to be kept in sync, but that'd only need to happen if you change the slots, and at least in the current code, you create them once and never change them again: [2]. So no tracking is needed. You could just convert "HTMLSlotElement* summary_slot =" to "summary_slot_ =" at [2] and you'd be done.
Patch Set #1, Line 174:
if (IsFirstSummary(child)) {
But I still need an array to pass to Assign()
True. Maybe add an overload or Assign() that takes a single value? Or this is ok as-is.
Patch Set #1, Line 180:
if (summary_slot) {
No problem. […]
That's a perfectly safe assumption.
To view, visit change 3163573. To unsubscribe, or for help writing mail filters, visit settings.
Aleks Totic (Gerrit)
Attention is currently required from: Mason Freed.
5 comments:
Patchset:
One outstanding issue left:
File third_party/blink/renderer/core/dom/slot_assignment.cc:
Patch Set #1, Line 390:
&& !owner_->IsManualSlotting()See [1]. […]
Done
Patch Set #1, Line 390:
&& !owner_->IsManualSlotting()
See [1]. […]
I've just tried it, and DCHECK gets triggered immediately for a details element.
supports_name_based_slot_assignment_ is false
SupportsNameBasedSlotAssignment is false.
IsManualSlotting is trueThis DCHECK fires:
if (!owner_->SupportsNameBasedSlotAssignment()) {
DCHECK(!owner_->IsManualSlotting());
return FindSlotInUserAgentShadow(node);
What am I missing here?
File third_party/blink/renderer/core/html/html_details_element.cc:
Patch Set #1, Line 151:
fast/dom/shadow/offset-parent-does-not-leak-ua-shadow.htm
Hmm, on the test, what about changing from a <details> element to just a plain <div>, but use Intern […]
Ack
Patch Set #1, Line 174:
if (IsFirstSummary(child)) {
True. Maybe add an overload or Assign() that takes a single value? Or this is ok as-is.
Leave as is.
To view, visit change 3163573. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aleks Totic.
Patch set 4:Code-Review +1
2 comments:
Patchset:
Nice, this LGTM! Should make the next two a piece of cake.
File third_party/blink/renderer/core/dom/slot_assignment.cc:
Patch Set #1, Line 390:
&& !owner_->IsManualSlotting()
Done
Ahh. Somehow I was under the (incorrect) assumption that you had flipped the SupportsNameBasedSlotAssignment to true for the <details> in this CL. I've been using "SupportsNameBasedSlotAssignment" as synonymous with "not an old school UA slot".
I'm good with keeping this as-is, and the whole thing will go away once you're done with this effort. 😊
To view, visit change 3163573. To unsubscribe, or for help writing mail filters, visit settings.