Attention is currently required from: Rune Lillesveen.
Mason Freed would like Rune Lillesveen to review this change.
Remove UpdateDistributionForUnknownReasons()
This is a leftover from the Shadow DOM v0 codebase, and was
only called in a small handful of places. Most of those were
moved into StyleEngine::UpdateActiveStyle(), and all were
converted to a call to RecalcSlotAssignments(). This CL
also contains other related cleanup, removing "distribution"
as a concept.
This should have no effect on behavior.
Bug: 937746
Change-Id: Iaa6ae72e383444d839ecf1c346e87a83504ffa7a
---
M third_party/blink/renderer/core/css/style_engine.cc
M third_party/blink/renderer/core/dom/README.md
M third_party/blink/renderer/core/dom/document.cc
M third_party/blink/renderer/core/dom/node.cc
M third_party/blink/renderer/core/dom/node.h
M third_party/blink/renderer/core/html/html_element.cc
M third_party/blink/renderer/core/inspector/inspector_css_agent.cc
M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
M third_party/blink/web_tests/SlowTests
R third_party/blink/web_tests/http/tests/devtools/elements/shadow/shadow-slot-assignment-expected.txt
R third_party/blink/web_tests/http/tests/devtools/elements/shadow/shadow-slot-assignment.js
11 files changed, 22 insertions(+), 53 deletions(-)
To view, visit change 3160780. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Rune Lillesveen.
1 comment:
Patchset:
futhark@, PTAL, thanks!
To view, visit change 3160780. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Rune Lillesveen, Alice Boxhall.
Mason Freed would like Alice Boxhall to review this change.
Remove UpdateDistributionForUnknownReasons()
This is a leftover from the Shadow DOM v0 codebase, and was
only called in a small handful of places. Most of those were
moved into StyleEngine::UpdateActiveStyle(), and all were
converted to a call to RecalcSlotAssignments(). This CL
also contains other related cleanup, removing "distribution"
as a concept.
This should have no effect on behavior.
Bug: 937746
Change-Id: Iaa6ae72e383444d839ecf1c346e87a83504ffa7a
---
M third_party/blink/renderer/core/css/style_engine.cc
M third_party/blink/renderer/core/dom/README.md
M third_party/blink/renderer/core/dom/document.cc
M third_party/blink/renderer/core/dom/node.cc
M third_party/blink/renderer/core/dom/node.h
M third_party/blink/renderer/core/html/html_element.cc
M third_party/blink/renderer/core/inspector/inspector_css_agent.cc
M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
M third_party/blink/web_tests/SlowTests
R third_party/blink/web_tests/http/tests/devtools/elements/shadow/shadow-slot-assignment-expected.txt
R third_party/blink/web_tests/http/tests/devtools/elements/shadow/shadow-slot-assignment.js
11 files changed, 22 insertions(+), 53 deletions(-)
To view, visit change 3160780. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Rune Lillesveen, Alice Boxhall.
1 comment:
Patchset:
+aboxhall@ for ax_object_cache_impl.cc. Just a comment there. Thanks!
To view, visit change 3160780. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Alice Boxhall, Mason Freed.
2 comments:
File third_party/blink/renderer/core/css/style_engine.cc:
Patch Set #2, Line 546: GetDocument().GetSlotAssignmentEngine().RecalcSlotAssignments();
There are a few places we call UpdateActiveStyle() and didn't update slot assignments without this CL. Updating slot assignments is not logically part of updating active stylesheets and rulesets, and for instance for accessing document.fonts, I think this will introduce a slot assignment update in a case where we didn't do that before:
<style>
@font-face { font-family: ... }
</style>
<div id="host"><span></span></div>
<script>
let root = host.attachShadow();
root.innerHTML = "<slot></slot>";
// This will now trigger slot assignments.
document.fonts.check(...);
</script>
File third_party/blink/renderer/core/dom/document.cc:
I wonder why we needed this here. Might be a common code path with element style resolution that triggers a DCHECK?
To view, visit change 3160780. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Alice Boxhall, Mason Freed.
Patch set 2:Code-Review +1
Attention is currently required from: Rune Lillesveen, Alice Boxhall.
2 comments:
File third_party/blink/renderer/core/css/style_engine.cc:
Patch Set #2, Line 546: GetDocument().GetSlotAssignmentEngine().RecalcSlotAssignments();
There are a few places we call UpdateActiveStyle() and didn't update slot assignments without this C […]
Hmm interesting. What are the rules for <style> tags inside shadow dom? I'm wondering, e.g., what happens if there's a style tag inside slot fallback content. So modifying your example:
<div id="host"><template shadowroot=open>
<slot>
Fallback content
<style>
* {font-family: ...;}
</style>
</slot>
</template>slotted content</div>
Should the <style> element get loaded there? If it depends on whether content is slotted, then perhaps we might need the slot assignment update? Just thinking out loud.
I'm generally happy putting back the explicit calls to RecalcSlotAssignments() before UpdateActiveStyle(), if you think that's better. I was mainly seeing if I could clean up some of the previous calls, since they sounded like it was unknown whether/why they were needed.
File third_party/blink/renderer/core/dom/document.cc:
I wonder why we needed this here. […]
This was one that I hoped to eliminate by pushing it into UpdateActiveStyle(). I can try just removing it entirely (after I remove RecalcSlotAssignments() from UpdateActiveStyle()) and see what happens...?
To view, visit change 3160780. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Rune Lillesveen, Alice Boxhall.
1 comment:
File third_party/blink/renderer/core/dom/document.cc:
// TODO(masonf): remove me
//GetSlotAssignmentEngine().RecalcSlotAssignments()
We'll see what happens on this patchset.
To view, visit change 3160780. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Alice Boxhall, Mason Freed.
4 comments:
File third_party/blink/renderer/core/css/style_engine.cc:
Patch Set #2, Line 546: GetDocument().GetSlotAssignmentEngine().RecalcSlotAssignments();
Hmm interesting. What are the rules for <style> tags inside shadow dom? I'm wondering, e.g., what happens if there's a style tag inside slot fallback content. So modifying your example:
<div id="host"><template shadowroot=open>
<slot>
Fallback content
<style>
* {font-family: ...;}
</style>
</slot>
</template>slotted content</div>Should the <style> element get loaded there? If it depends on whether content is slotted, then perhaps we might need the slot assignment update? Just thinking out loud.
The html spec only says it needs to be connected, so I believe it should be parsed/applied.
I'm generally happy putting back the explicit calls to RecalcSlotAssignments() before UpdateActiveStyle(), if you think that's better. I was mainly seeing if I could clean up some of the previous calls, since they sounded like it was unknown whether/why they were needed.
Yes, I think we should move it out of UpdateActiveStyle().
File third_party/blink/renderer/core/dom/document.cc:
This was one that I hoped to eliminate by pushing it into UpdateActiveStyle(). […]
Yep
File third_party/blink/renderer/core/dom/document.cc:
// TODO(masonf): remove me
//GetSlotAssignmentEngine().RecalcSlotAssignments()
We'll see what happens on this patchset.
Looks green to me.
File third_party/blink/renderer/core/inspector/inspector_css_agent.cc:
Patch Set #4, Line 2273: element->GetDocument().GetSlotAssignmentEngine().RecalcSlotAssignments();
I assume this is necessary for PseudoCSSRulesForElement() below. For style recalc, we typically call RecalcSlotAssignments for the document before traversing the DOM for style recalc, not every time we call StyleResolver::StyleForElement(), but this is a one-off for a single element. Might be better to do RecalcSlotAssignments() in PseudoCSSRulesForElement() unless there are cases where we call it for many elements traversing over the tree.
Browsing the source a bit, we UpdateStyleAndLayoutTreeForNode(), which will update the slot assignments in this case:
So, the APIs for inspector probably needs a closer look.
You may leave RecalcSlotAssignments() here for now with a comment saying it's needed for matching rules on an up-to-date flat tree in PseudoCSSRulesForElement(). Or, move it into PseudoCSSRulesForElement(). Your choice.
To view, visit change 3160780. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Alice Boxhall, Mason Freed.
1 comment:
Patchset:
Note that @aboxhall is on leave.
To view, visit change 3160780. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Rune Lillesveen, Alice Boxhall, Aaron Leventhal.
5 comments:
Patchset:
Note that @aboxhall is on leave.
Yep, thanks for the heads-up. nektar@ +1'd this CL for me.
We really should add an OOO notification to aboxhall@'s Gerrit account, to avoid this problem going forward. Though that'll be hard without her help.
File third_party/blink/renderer/core/css/style_engine.cc:
Patch Set #2, Line 546: GetDocument().GetSlotAssignmentEngine().RecalcSlotAssignments();
> Hmm interesting. What are the rules for <style> tags inside shadow dom? I'm wondering, e.g. […]
Done
File third_party/blink/renderer/core/dom/document.cc:
Yep
Done
File third_party/blink/renderer/core/dom/document.cc:
// TODO(masonf): remove me
//GetSlotAssignmentEngine().RecalcSlotAssignments()
Looks green to me.
Great! I removed the comment and the call.
File third_party/blink/renderer/core/inspector/inspector_css_agent.cc:
Patch Set #4, Line 2273: element->GetDocument().GetSlotAssignmentEngine().RecalcSlotAssignments();
I assume this is necessary for PseudoCSSRulesForElement() below. […]
Thanks for pointing that out. I went to move it into PseudoCSSRulesForElement, but looking a bit more around the code, there definitely are several loops I found that traverse the tree and call that once per element. E.g. [1] (through CssRulesForElement()).
While extra calls to RecalcSlotAssignments should be very fast (they early out if nothing needs recalc), I'm still inclined to just add the TODO comment here for now, if that works for you.
To view, visit change 3160780. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Rune Lillesveen, Alice Boxhall, Aaron Leventhal.
Mason Freed uploaded patch set #6 to this change.
Remove UpdateDistributionForUnknownReasons()
This is a leftover from the Shadow DOM v0 codebase, and was
only called in a small handful of places. Most of those were
replaced by calls to RecalcSlotAssignments(), but a few were
removed entirely. This CL also contains other related cleanup,
removing "distribution" as a concept.
This should have no effect on behavior.
Bug: 937746
Change-Id: Iaa6ae72e383444d839ecf1c346e87a83504ffa7a
---
M third_party/blink/renderer/core/dom/README.md
M third_party/blink/renderer/core/dom/document.cc
M third_party/blink/renderer/core/dom/node.cc
M third_party/blink/renderer/core/dom/node.h
M third_party/blink/renderer/core/html/html_element.cc
M third_party/blink/renderer/core/inspector/inspector_css_agent.cc
M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
M third_party/blink/web_tests/SlowTests
R third_party/blink/web_tests/http/tests/devtools/elements/shadow/shadow-slot-assignment-expected.txt
R third_party/blink/web_tests/http/tests/devtools/elements/shadow/shadow-slot-assignment.js
10 files changed, 29 insertions(+), 54 deletions(-)
To view, visit change 3160780. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Alice Boxhall, Mason Freed, Aaron Leventhal.
Patch set 6:Code-Review +1
2 comments:
Patchset:
lgtm
File third_party/blink/renderer/core/inspector/inspector_css_agent.cc:
Patch Set #4, Line 2273: element->GetDocument().GetSlotAssignmentEngine().RecalcSlotAssignments();
Thanks for pointing that out. […]
Yes, wfm.
To view, visit change 3160780. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Alice Boxhall, Aaron Leventhal.
Patch set 6:Commit-Queue +2
1 comment:
Patchset:
Thanks for the review!
To view, visit change 3160780. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ submitted this change.
Remove UpdateDistributionForUnknownReasons()
This is a leftover from the Shadow DOM v0 codebase, and was
only called in a small handful of places. Most of those were
replaced by calls to RecalcSlotAssignments(), but a few were
removed entirely. This CL also contains other related cleanup,
removing "distribution" as a concept.
This should have no effect on behavior.
Bug: 937746
Change-Id: Iaa6ae72e383444d839ecf1c346e87a83504ffa7a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3160780
Reviewed-by: Rune Lillesveen <fut...@chromium.org>
Reviewed-by: Nektarios Paisios <nek...@chromium.org>
Commit-Queue: Mason Freed <mas...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#922231}
---
M third_party/blink/renderer/core/dom/README.md
M third_party/blink/renderer/core/dom/document.cc
M third_party/blink/renderer/core/dom/node.cc
M third_party/blink/renderer/core/dom/node.h
M third_party/blink/renderer/core/html/html_element.cc
M third_party/blink/renderer/core/inspector/inspector_css_agent.cc
M third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
M third_party/blink/web_tests/SlowTests
R third_party/blink/web_tests/http/tests/devtools/elements/shadow/shadow-slot-assignment-expected.txt
R third_party/blink/web_tests/http/tests/devtools/elements/shadow/shadow-slot-assignment.js
10 files changed, 29 insertions(+), 54 deletions(-)