[HiC] Fix issues with shadow dom canvas children [chromium/src : main]
Stephen Chenney (Gerrit)
Stephen Chenney added 1 comment![]( "Open in Gerrit")
There is a huge amount of potential testing to be done here. I tried to cover the basics to be sure that paint flags are set inside the shadow tree and the style system also correctly identified canvas children inside a shadow dom. Note it fixes the more complex example with forms and the autofill example.
I still need to add unit tests for the autofill problem, but please provide any early review if you have the chance.
Related details
- Philip Rogers
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
- Philip Rogers
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Chenney (Gerrit)
Stephen Chenney voted and added 2 comments![]( "Open in Gerrit")
Votes added by Stephen Chenney
| Commit-Queue | +1 |
I'll wait until the tests pass
Yep. Thanks.
I'm wondering how important it is to avoid resetting the style on elements outside the flat tree during a moveBefore operation. I understand that the point of moveBefore is to avoid recomputing style if possible, but maybe we can tolerate it for elements inside a shadow-dom. Anyways, this versions passes tests locally but I have not thoroughly tested the change I made to fix the moveBefore test.
Related details
- Philip Rogers
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 5 comments![]( "Open in Gerrit")
Slightly worried about performance, but some DOM owner should have a look as well.
Recursively mark shadow dom contetn as a canvas childcontent
when it it's slotting status changes (flat tree parentits
void Element::SetIsCanvasOrInCanvasSubtreeRecursively(bool value) {Remind me again why we cannot rely on an inherited ComputedStyle flag. Do we need to know this information for non-rendered elements, or before we do a style recalc?
if (IsPseudoElement()) {This block is probably doing the same thing as Node::FlatTreeParentForChildDirty(). Might want to consolidate into some Node::FlatTreeParentWithoutRecalc() if they are actually the same.
Related details
- Philip Rogers
- Stephen Chenney
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. |
Stephen Chenney (Gerrit)
Stephen Chenney added 5 comments![]( "Open in Gerrit")
There are a couple of cases where we removed code that changes things when the tree status changes (video state and autofill suggestions). I'll need to add tests for those cases or otherwise make sure it's right. But I have a question about how to detect the situation. See other comments.
Recursively mark shadow dom contetn as a canvas childStephen Chenneycontent
Done
when it it's slotting status changes (flat tree parentStephen Chenneyits
Done
void Element::SetIsCanvasOrInCanvasSubtreeRecursively(bool value) {Remind me again why we cannot rely on an inherited ComputedStyle flag. Do we need to know this information for non-rendered elements, or before we do a style recalc?
We can and the latest CL does that. Sorry for wasting your time on this version, though the tests remain. One question arises from the need to respond to changes in canvas subtree status: Does StyleDidChange get called if a flag gets set on the style even though no property changes?
This block is probably doing the same thing as Node::FlatTreeParentForChildDirty(). Might want to consolidate into some Node::FlatTreeParentWithoutRecalc() if they are actually the same.
- Philip Rogers
- Rune Lillesveen
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. |
Stephen Chenney (Gerrit)
Stephen Chenney added 4 comments![]( "Open in Gerrit")
void HTMLInputElement::DidChangeIsCanvasOrInCanvasSubtree() {I'm worried about still needing this code path, though the test still passes. I'll verify more carefully.
void HTMLSelectElement::DidChangeIsCanvasOrInCanvasSubtree() {Ditto.
void HTMLTextAreaElement::DidChangeIsCanvasOrInCanvasSubtree() {ditto
void HTMLVideoElement::DidChangeIsCanvasOrInCanvasSubtree() {I need to verify whether we have testing that covers this code path because it has disappeared.
Rune Lillesveen (Gerrit)
Rune Lillesveen added 3 comments![]( "Open in Gerrit")
builder.SetIsInCanvasSubtree(true);This does a FlatTreeTraversal::ParentElement() for all elements resolving style.
Wondering if it would make sense to instead have two bits, one non-inherited for canvas elements which are true for layoutSubtree(), and one inherited which the element is in a canvas+layoutSubtree() subtree. Then, setting the bits only happen for the StyleAdjuster for the canvas+layoutSubtree() elements. Checking for stacking context could check the parent_style's flag for layoutSubtree() if its own flag for in-canvas-subtree is true.
Not sure if layoutSubtree() inside layoutSubtree() is possible, but something like this:
```
[subtree-root] [canvas-subtree]
div 0 0
canvas[layoutSubtree] 1 1
div 0 1
canvas 0 1
div 0 1
canvas[layoutSubtree] 1? 1
```
void Element::SetIsCanvasOrInCanvasSubtreeRecursively(bool value) {Stephen ChenneyRemind me again why we cannot rely on an inherited ComputedStyle flag. Do we need to know this information for non-rendered elements, or before we do a style recalc?
We can and the latest CL does that. Sorry for wasting your time on this version, though the tests remain. One question arises from the need to respond to changes in canvas subtree status: Does StyleDidChange get called if a flag gets set on the style even though no property changes?
Should be, as long as custom_compare is not set to true.
void HTMLInputElement::DidChangeIsCanvasOrInCanvasSubtree() {I'm worried about still needing this code path, though the test still passes. I'll verify more carefully.
I would think so. Slotting an autofilled input into a canvas subtree needs to change the rendering, I'd assume. I'm unsure that works well with this code as-is. Doing DOM changes from style recalc can typically be problematic. It could be that the rendering should behave as if the suggested value was the empty string, rather than actually setting it. But I'd need to look deeper to understand what's necessary.
Related details
- Philip Rogers
- Stephen Chenney
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. |
Stephen Chenney (Gerrit)
Stephen Chenney added 4 comments![]( "Open in Gerrit")
Still working to make sure the autofill suggestions get cleared. And I'll get rid of the flat tree traversal for all elements.
builder.SetIsInCanvasSubtree(true);This does a FlatTreeTraversal::ParentElement() for all elements resolving style.
Wondering if it would make sense to instead have two bits, one non-inherited for canvas elements which are true for layoutSubtree(), and one inherited which the element is in a canvas+layoutSubtree() subtree. Then, setting the bits only happen for the StyleAdjuster for the canvas+layoutSubtree() elements. Checking for stacking context could check the parent_style's flag for layoutSubtree() if its own flag for in-canvas-subtree is true.
Not sure if layoutSubtree() inside layoutSubtree() is possible, but something like this:
```
[subtree-root] [canvas-subtree]
div 0 0
canvas[layoutSubtree] 1 1
div 0 1
canvas 0 1
div 0 1
canvas[layoutSubtree] 1? 1
```
I think we can just check here for `IsA<HTMLCanvasElement>(element)` and `SetIsInCanvasSubtree(true)` and rely on inheritance to mark the subtree. And then in Element::IsInCanvasSubtree we explicitly check for the element being a canvas and return false. It sort of inverts the current IsInCanvasOrCanvasSubtree/IsInCanvasSubtree code. I'll verify that works.
void Element::SetIsCanvasOrInCanvasSubtreeRecursively(bool value) {Stephen ChenneyRemind me again why we cannot rely on an inherited ComputedStyle flag. Do we need to know this information for non-rendered elements, or before we do a style recalc?
Rune LillesveenWe can and the latest CL does that. Sorry for wasting your time on this version, though the tests remain. One question arises from the need to respond to changes in canvas subtree status: Does StyleDidChange get called if a flag gets set on the style even though no property changes?
Should be, as long as custom_compare is not set to true.
Thanks. It turns out I can use the InsertedInto logic to fix the issue with video.
void HTMLVideoElement::DidChangeIsCanvasOrInCanvasSubtree() {I need to verify whether we have testing that covers this code path because it has disappeared.
This was a bug. I have managed to create a test and fixed the issue.
Related details
- Philip Rogers
- Rune Lillesveen
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. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
builder.SetIsInCanvasSubtree(true);Stephen ChenneyThis does a FlatTreeTraversal::ParentElement() for all elements resolving style.
Wondering if it would make sense to instead have two bits, one non-inherited for canvas elements which are true for layoutSubtree(), and one inherited which the element is in a canvas+layoutSubtree() subtree. Then, setting the bits only happen for the StyleAdjuster for the canvas+layoutSubtree() elements. Checking for stacking context could check the parent_style's flag for layoutSubtree() if its own flag for in-canvas-subtree is true.
Not sure if layoutSubtree() inside layoutSubtree() is possible, but something like this:
```
[subtree-root] [canvas-subtree]
div 0 0
canvas[layoutSubtree] 1 1
div 0 1
canvas 0 1
div 0 1
canvas[layoutSubtree] 1? 1
```
I think we can just check here for `IsA<HTMLCanvasElement>(element)` and `SetIsInCanvasSubtree(true)` and rely on inheritance to mark the subtree. And then in Element::IsInCanvasSubtree we explicitly check for the element being a canvas and return false. It sort of inverts the current IsInCanvasOrCanvasSubtree/IsInCanvasSubtree code. I'll verify that works.
Don't you need to treat a <canvas> element in the <canvas> subtree differently from the root <canvas>?
Related details
- Philip Rogers
- Stephen Chenney
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. |
Stephen Chenney (Gerrit)
Stephen Chenney added 5 comments![]( "Open in Gerrit")
I meant to send this last night, but didn't. Anyways ...
Trying to use the style system has been an exercise in frustration. It seems we still need the equivalent of "DidChangeCanvasSubtree" for autofill suppression (currently recreating it in InsertedInto) and it looks like the iframe examples still need the recursive marking thing of some kind. But even then due to the timing of calls seems to cause problems particularly for moveBefore. At this point I think it's better to revert back to using the flags on element, particularly since this is an element concept rather than CSS. Maybe file a bug to revisit it later if we really want to use style.
builder.SetIsInCanvasSubtree(true);Stephen ChenneyThis does a FlatTreeTraversal::ParentElement() for all elements resolving style.
Wondering if it would make sense to instead have two bits, one non-inherited for canvas elements which are true for layoutSubtree(), and one inherited which the element is in a canvas+layoutSubtree() subtree. Then, setting the bits only happen for the StyleAdjuster for the canvas+layoutSubtree() elements. Checking for stacking context could check the parent_style's flag for layoutSubtree() if its own flag for in-canvas-subtree is true.
Not sure if layoutSubtree() inside layoutSubtree() is possible, but something like this:
```
[subtree-root] [canvas-subtree]
div 0 0
canvas[layoutSubtree] 1 1
div 0 1
canvas 0 1
div 0 1
canvas[layoutSubtree] 1? 1
```
Rune LillesveenI think we can just check here for `IsA<HTMLCanvasElement>(element)` and `SetIsInCanvasSubtree(true)` and rely on inheritance to mark the subtree. And then in Element::IsInCanvasSubtree we explicitly check for the element being a canvas and return false. It sort of inverts the current IsInCanvasOrCanvasSubtree/IsInCanvasSubtree code. I'll verify that works.
Don't you need to treat a <canvas> element in the <canvas> subtree differently from the root <canvas>?
We do not support nested canvas at this time, so no need for distinct treatment, and we have tests.
void HTMLInputElement::DidChangeIsCanvasOrInCanvasSubtree() {I'm worried about still needing this code path, though the test still passes. I'll verify more carefully.
I would think so. Slotting an autofilled input into a canvas subtree needs to change the rendering, I'd assume. I'm unsure that works well with this code as-is. Doing DOM changes from style recalc can typically be problematic. It could be that the rendering should behave as if the suggested value was the empty string, rather than actually setting it. But I'd need to look deeper to understand what's necessary.
I looked at this and I am very uncomfortable about returning an empty value when queried rather than setting the suggested value to null/empty. The problem is that not all code uses the getter method to access the suggested value. It's certainly harder to do it when set, however, so we may have no choice.
I also discovered the problem with using style changes to trigger setting the value. Lesson learned.
void HTMLSelectElement::DidChangeIsCanvasOrInCanvasSubtree() {Stephen ChenneyDitto.
Done
void HTMLTextAreaElement::DidChangeIsCanvasOrInCanvasSubtree() {Stephen Chenneyditto
Done
Related details
- Philip Rogers
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 2 comments![]( "Open in Gerrit")
The style flag approach lgtm from my side, but win-rel reports about a flaky test, and pdr@ should review.
if (IsA<HTMLCanvasElement>(FlatTreeTraversal::ParentElement(*element))) {state.ParentElement() should be cheaper
Related details
- Philip Rogers
- Stephen Chenney
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. |
Stephen Chenney (Gerrit)
Stephen Chenney added 2 comments![]( "Open in Gerrit")
Finally have something that works consistently.
if (IsA<HTMLCanvasElement>(FlatTreeTraversal::ParentElement(*element))) {Stephen Chenneystate.ParentElement() should be cheaper
Acknowledged
Related details
- Philip Rogers
- Rune Lillesveen
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 7 comments![]( "Open in Gerrit")
void Element::SetIsCanvasOrInCanvasSubtreeRecursively(bool value) {Merge this into `Element::SetIsCanvasOrInCanvasSubtree`? It's super confusing that we have both `Element::SetIsCanvasOrInCanvasSubtreeRecursively` and `Element ::SetIsCanvasOrInCanvasSubtree`.
Can you add the following test? I think it will fail.
```
TEST_F(WebFormControlElementTest,
TextControlPreviewDisabledWhenUnslottedParentSlottedIntoCanvas) {
ScopedCanvasDrawElementForTest forced_canvas_draw_element_feature(true);
GetDocument().body()->SetHTMLUnsafeWithoutTrustedTypes(R"(
<canvas layoutsubtree>
<div id="host">
<template shadowrootmode="open">
<div id="slotwrapper">
<slot name="s1"></slot>
</div>
</template>
<div id="unslotted_parent">
<input id="input_id">
</div>
</div>
</canvas>
)");
Element* input_elmt = GetElementById("input_id");
ASSERT_TRUE(input_elmt);WebFormControlElement input(DynamicTo<HTMLFormControlElement>(input_elmt));
// The input is currently unslotted, so it is not in the flat tree or canvas.
EXPECT_FALSE(input_elmt->IsInCanvasSubtree());
// Suggestions should work while unslotted outside flat tree canvas.
input.SetSuggestedValue("suggestion");
EXPECT_EQ(input.SuggestedValue().Ascii(), "suggestion");
// Now dynamically slot unslotted_parent into the canvas slot.
GetElementById("unslotted_parent")
->setAttribute(html_names::kSlotAttr, AtomicString("s1"));
// Force slot assignment recalc and style update.
GetDocument().UpdateStyleAndLayoutTree();
EXPECT_TRUE(input_elmt->IsInCanvasSubtree());
// The input element's suggested value should be cleared when it enters
// the canvas flat tree.
EXPECT_TRUE(input.SuggestedValue().IsEmpty());
}
```
EXPECT_CALL((*MockMediaPlayer()), RequestVideoFrameCallback()).Times(1);Can you also test that removing the video does not request video frame callbacks?
```
// Appending to canvas should request video frames.
EXPECT_CALL(*MockMediaPlayer(), RequestVideoFrameCallback()).Times(1);
GetElementById("canvas")->appendChild(video);
UpdateAllLifecyclePhasesForTest();
testing::Mock::VerifyAndClearExpectations(MockMediaPlayer());
// Removing from canvas should not trigger another frame callback request.
EXPECT_CALL(*MockMediaPlayer(), RequestVideoFrameCallback()).Times(0);
video->remove();
UpdateAllLifecyclePhasesForTest();
testing::Mock::VerifyAndClearExpectations(MockMediaPlayer());
```
<link rel="author" href="mailto:vmp...@chromium.org">remove or update to schenney@
<html class="reftest-wait">remove reftest-wait
document.addEventListener('paint-received', (event) => {This test can run before this event is received. Can you wrap this in `promise_test(async () => {...}`?
<body>remove body and un-indent the <canvas>...</canvas>
Related details
- Rune Lillesveen
- Stefan Zager
- Stephen Chenney
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. |
Stephen Chenney (Gerrit)
Stephen Chenney added 8 comments![]( "Open in Gerrit")
Note the slotting test you suggested adding fails because things slot anywhere if they do not include a specific "slot" attribute. So the test was not verifying a change in slotting status.
void Element::SetIsCanvasOrInCanvasSubtreeRecursively(bool value) {Merge this into `Element::SetIsCanvasOrInCanvasSubtree`? It's super confusing that we have both `Element::SetIsCanvasOrInCanvasSubtreeRecursively` and `Element ::SetIsCanvasOrInCanvasSubtree`.
Done
This is covered by TEST_F(WebFormControlElementTest,
TextControlPreviewDisabledInCanvasWhenNestedAndSlotted)
EXPECT_CALL((*MockMediaPlayer()), RequestVideoFrameCallback()).Times(1);Can you also test that removing the video does not request video frame callbacks?
```
// Appending to canvas should request video frames.
EXPECT_CALL(*MockMediaPlayer(), RequestVideoFrameCallback()).Times(1);
GetElementById("canvas")->appendChild(video);
UpdateAllLifecyclePhasesForTest();
testing::Mock::VerifyAndClearExpectations(MockMediaPlayer());// Removing from canvas should not trigger another frame callback request.
EXPECT_CALL(*MockMediaPlayer(), RequestVideoFrameCallback()).Times(0);
video->remove();
UpdateAllLifecyclePhasesForTest();
testing::Mock::VerifyAndClearExpectations(MockMediaPlayer());
```
Done
remove or update to schenney@
Done
<html class="reftest-wait">Stephen Chenneyremove reftest-wait
Done
document.addEventListener('paint-received', (event) => {This test can run before this event is received. Can you wrap this in `promise_test(async () => {...}`?
This is listening for the custom event from the custom element. It actually needs to be in this location so that the event listener is set up before the custom element is created because otherwise the custom element can send the event before the listener is there to get it, causing timeouts.
So the sequence looks like this:
- Set up the event listener for "paint-received" on the main document
- Creating the canvas-widget causes it to request "paint" events
- When the custom widget gets the "paint" event it paints, and then sends "paint-received"
- The main document event handler runs and executes the test on the data from the custom widget
I verified that the test does run:
canvas-in-custom-element-images-ignored.tentative.html: Test OK. Subtests passed 1/1. Unexpected 0
Were you concerned about something else?
remove body and un-indent the <canvas>...</canvas>
- Philip Rogers
- Rune Lillesveen
- Stefan Zager
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. |
Stephen Chenney (Gerrit)
Stephen Chenney added 1 comment![]( "Open in Gerrit")
Please review Patchset 16 because the changes in Patchset 17 are only there to see if we need to check the subtree when the root node status changed. For some reason they are failing and I'm working out why. Meanwhile. Patchset 16 works.
Stephen Chenney (Gerrit)
Stephen Chenney added 1 comment![]( "Open in Gerrit")
The latest version is the one for now.
In a follow up I plan to switch the element flag back to IsInCanvasSubtree because making that cheap and then checking IsA<HTMLCanvasElement> in addition for IsCanvasOrInCanvasSubtree is cheaper than the current version that does a lot of work to find a parent in IsInCanvasSubtree.
Related details
- Philip Rogers
- Rune Lillesveen
- Stefan Zager
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. |
Philip Rogers (Gerrit)
Philip Rogers added 4 comments![]( "Open in Gerrit")
* The style based relied on suppressing autofill data at the getter stage, rather than on setting. I think a setter approach is more robust to future changes.nit: wrap
// Only set canvas subtree state for elements that are participating inRemove the comment and IsNodeInFlatTree check and just call `element->SetIsCanvasOrInCanvasSubtree(element->IsInCanvasSubtree());`?
This test fails, and passes with this suggestion.
```
TEST_F(
WebFormControlElementTest,
TextControlPreviewDisabledWhenSlottedIntoCanvasAfterStyleEnsuredOutsideFlatTree) {
ScopedCanvasDrawElementForTest forced_canvas_draw_element_feature(true);
ScopedGetComputedStyleOutsideFlatTreeForTest scoped_feature(true);
GetDocument().body()->SetHTMLUnsafeWithoutTrustedTypes(R"(
<div id="host">
<template shadowrootmode="open">
<canvas id="canvas" layoutsubtree>
<slot name="canvas_slot"></slot>
</canvas>
</template>
<input id="input">
</div>
)");
HTMLInputElement* input =
DynamicTo<HTMLInputElement>(GetElementById("input"));
ASSERT_TRUE(input);
GetDocument().UpdateStyleAndLayoutTree();
// Ensure computed style outside the flat tree before assigning to a slot.
input->EnsureComputedStyle();
WebFormControlElement form_control(input);
form_control.SetSuggestedValue("suggestion");
// Now slot 'input' into the canvas slot inside the shadow root.
input->setAttribute(html_names::kSlotAttr, AtomicString("canvas_slot"));
GetDocument().UpdateStyleAndLayoutTree();
EXPECT_TRUE(input->IsInCanvasSubtree());
EXPECT_TRUE(input->IsCanvasOrInCanvasSubtree());
EXPECT_TRUE(form_control.SuggestedValue().IsEmpty());
}
```
element->DetachOverscroll();Call `element->SetIsCanvasOrInCanvasSubtree(element->IsInCanvasSubtree());` here?
Here's a test that fails without this:
```
TEST_F(WebFormControlElementTest,
TextControlCanvasSubtreeStaleWhenUnslottedFromCanvas) {
ScopedCanvasDrawElementForTest forced_canvas_draw_element_feature(true);
GetDocument().body()->SetHTMLUnsafeWithoutTrustedTypes(R"(
<div id="host">
<template shadowrootmode="open">
<canvas id="canvas" layoutsubtree>
<slot name="s1"></slot>
</canvas>
</template>
<input id="input" slot="s1">
</div>
)");
HTMLInputElement* input =
DynamicTo<HTMLInputElement>(GetElementById("input"));
GetDocument().UpdateStyleAndLayoutTree();
EXPECT_TRUE(input->IsInCanvasSubtree());
EXPECT_TRUE(input->IsCanvasOrInCanvasSubtree());
// Unslot the input element so it is removed from the flat tree.
input->setAttribute(html_names::kSlotAttr, AtomicString("nonexistent"));
GetDocument().UpdateStyleAndLayoutTree();
EXPECT_FALSE(input->IsInCanvasSubtree());
// When removed from the flat tree (RemovedFromFlatTree), its canvas subtree
// state should be updated so IsCanvasOrInCanvasSubtree() is false.
EXPECT_FALSE(input->IsCanvasOrInCanvasSubtree());
}
```
document.addEventListener('paint-received', (event) => {Stephen ChenneyThis test can run before this event is received. Can you wrap this in `promise_test(async () => {...}`?
This is listening for the custom event from the custom element. It actually needs to be in this location so that the event listener is set up before the custom element is created because otherwise the custom element can send the event before the listener is there to get it, causing timeouts.
So the sequence looks like this:
- Set up the event listener for "paint-received" on the main document
- Creating the canvas-widget causes it to request "paint" events
- When the custom widget gets the "paint" event it paints, and then sends "paint-received"
- The main document event handler runs and executes the test on the data from the custom widget
I verified that the test does run:
canvas-in-custom-element-images-ignored.tentative.html: Test OK. Subtests passed 1/1. Unexpected 0Were you concerned about something else?
- Rune Lillesveen
- Stefan Zager
- Stephen Chenney
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. |
Stephen Chenney (Gerrit)
Stephen Chenney added 3 comments![]( "Open in Gerrit")
* The style based relied on suppressing autofill data at the getter stage, rather than on setting. I think a setter approach is more robust to future changes.Stephen Chenneynit: wrap
Done
// Only set canvas subtree state for elements that are participating inThis test passes without the change, after fixing errors with the test case.
The test case has errors, but after fixing the test it works. A canvas cannot be the parent of a slot.
Related details
- Philip Rogers
- Rune Lillesveen
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
1 comment
Related details
- Rune Lillesveen
- Stefan Zager
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +0 |
1 comment
Hold on, a few comments
Related details
- Rune Lillesveen
- Stefan Zager
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 5 comments![]( "Open in Gerrit")
if (IsA<HTMLCanvasElement>(*this)) {Did we lose an early-out here?
```
void Element::SetIsCanvasOrInCanvasSubtree(bool value) {
value = value || IsA<HTMLCanvasElement>(*this);
if (value == IsCanvasOrInCanvasSubtree()) {
return;
}
```
Without `value == IsCanvasOrInCanvasSubtree()`, we'll have a performance regression.
element->DetachOverscroll();A canvas cannot be the parent of a slot.
Is this true? For example, these seem the same to me:
case a, div parent of slot:
```
<my-widget-a>
<template shadowrootmode="open">
<!-- The div directly hosts any light DOM content slotted into it -->
<div>
this is slotted:[
<slot name="content-a"></slot>
]
</div>
</template>
<div slot="content-a" id="slotted_child_a">Slotted Content A</div>
</my-widget-a>
```
case b, canvas parent of slot:
```
<my-widget-b>
<template shadowrootmode="open">
<!-- The canvas directly hosts any light DOM content slotted into it -->
<canvas layoutsubtree>
<slot name="content-b"></slot>
</canvas>
</template>
<div slot="content-b" id="slotted_child_b">Slotted Content B</div>
</my-widget-b>
```
If these are indeed the same and if direct children of canvas should be slotted, we need to add:
```
// Returns the flat tree parent element, skipping over any HTMLSlotElements.
static Element* ParentElementSkippingSlots(const Node&);
Element* FlatTreeTraversal::ParentElementSkippingSlots(const Node& node) {
Element* parent = ParentElement(node);
while (parent && IsA<HTMLSlotElement>(parent)) {
parent = ParentElement(*parent);
}
return parent;
}
```And update:
1. VerifyDrawElementImageEligibility in third_party/blink/renderer/core/html/canvas/html_canvas_element.cc
```
- if (element->parentElement() != this) {
+ const Element* parent =
+ FlatTreeTraversal::ParentElementSkippingSlots(*element);
+ if (parent != this) {
```
2. DirectReasonsForPaintProperties in third_party/blink/renderer/core/paint/compositing/compositing_reason_finder.cc
```
- auto* canvas_parent =
- DynamicTo<HTMLCanvasElement>(element->parentElement());
+ const Element* parent =
+ FlatTreeTraversal::ParentElementSkippingSlots(*element);
+ auto* canvas_parent = DynamicTo<HTMLCanvasElement>(parent);
```
3. ForceStackingAndContainingBlockForCanvasLayoutSubtree in third_party/blink/renderer/core/css/resolver/style_adjuster.cc
```
- if (const auto* canvas =
- DynamicTo<HTMLCanvasElement>(element->parentElement())) {
+ const Element* parent =
+ FlatTreeTraversal::ParentElementSkippingSlots(*element);
+ if (const auto* canvas = DynamicTo<HTMLCanvasElement>(parent)) {
```
4. PopulateCanvasChildState in third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
```
+static HTMLCanvasElement* FindCanvasParent(const LayoutObject& object) {
+ const Element* element = DynamicTo<Element>(object.GetNode());
+ if (!element) {
+ return nullptr;
+ }
+ const Element* parent =
+ FlatTreeTraversal::ParentElementSkippingSlots(*element);
+ return DynamicTo<HTMLCanvasElement>(const_cast<Element*>(parent));
+}
+
static void PopulateCanvasChildState(const LayoutObject& object,
EffectPaintPropertyNode::State& state) {
CHECK(IsA<LayoutBox>(object));
- auto& canvas_fragment = object.Parent()->FirstFragment();
+ HTMLCanvasElement* canvas = FindCanvasParent(object);
+ CHECK(canvas && canvas->GetLayoutObject());
+ auto& canvas_fragment = canvas->GetLayoutObject()->FirstFragment();
```
Here's the test for the compositing reason finder case which will pass with this change:
```
TEST_P(CompositingReasonFinderTest, CanvasChildSlotted) {
ScopedCanvasDrawElementForTest forced_canvas_draw_element_feature(true);
GetDocument().GetSettings()->SetScriptEnabled(true);
SetBodyInnerHTML(R"HTML(
<div id="host">
<div id="slotted" slot="s1" style="width: 10px; height: 10px;"></div>
</div>
)HTML");
auto* host = GetDocument().getElementById(AtomicString("host"));
ASSERT_TRUE(host);
ShadowRoot& shadow_root =
host->AttachShadowRootForTesting(ShadowRootMode::kOpen);
shadow_root.SetInnerHTMLWithoutTrustedTypes(R"HTML(
<canvas id="canvas" layoutsubtree>
<slot name="s1"></slot>
</canvas>
)HTML");
UpdateAllLifecyclePhasesForTest();
Element* slotted = GetElementById("slotted");
ASSERT_TRUE(slotted);
EXPECT_TRUE(slotted->IsInCanvasSubtree());
EXPECT_TRUE(slotted->IsCanvasOrInCanvasSubtree());
LayoutObject* layout_object = slotted->GetLayoutObject();
ASSERT_TRUE(layout_object);
EXPECT_REASONS(CompositingReason::kCanvasChild,
CompositingReasonFinder::DirectReasonsForPaintProperties(
*layout_object));
}
```I think case b, above, is the canonical way that content will be slotted into canvas. If that is true, can you also add a WPT test of that scenario?
EXPECT_TRUE(input.Unwrap<HTMLInputElement>()->IsInCanvasSubtree());Remove this line (this is tested on line 238)
// Because of bug 1, the inner element flags are not set to true, remainingnit: remove "Because of bug 1, "
const HeapVector<Member<Node>>& AssignedNodesNoRecalc() const {WDYT of this comment?
```
// Use AssignedNodes() in almost all cases. Only use AssignedNodesNoRecalc()
// when calling RecalcAssignment() is forbidden or dangerous (e.g., during tree
// modifications, moveBefore semantics, or state propagation like
// Element::SetIsCanvasOrInCanvasSubtree where eventual consistency is
// guaranteed by subsequent FlatTreeParentChanged/RemovedFromFlatTree hooks).
```
Related details
- Rune Lillesveen
- Stefan Zager
- Stephen Chenney
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. |
Stephen Chenney (Gerrit)
Stephen Chenney added 5 comments![]( "Open in Gerrit")
if (IsA<HTMLCanvasElement>(*this)) {Did we lose an early-out here?
```
void Element::SetIsCanvasOrInCanvasSubtree(bool value) {
value = value || IsA<HTMLCanvasElement>(*this);
if (value == IsCanvasOrInCanvasSubtree()) {
return;
}
```Without `value == IsCanvasOrInCanvasSubtree()`, we'll have a performance regression.
I tried to keep the early out. I wrote a DCHECK-enabled verifier to make sure that if we early outed the subtree was correct. That revealed the need to still process the children, so we can't early out.
Now maybe we can still early out if we call this method on a different element at the various call sites, like on the slotted thing and not the slot or something similar. If we try that we definitely need the verifier in place. I would leave it for a follow up where I also try to reduce the cost of Element::IsInCanvasSubtree().
Mmm, you're right that by spec a `<canvas>` can be the parent of a `<slot>`. I was mistakenly thinking of a `<template>` element not allowed as a canvas direct child. But `HTMLSlotElement::Assign(const HeapVector<Member<Node>>& nodes)` is never called when the slot is a child of the canvas, so in practice we might not support it. I'll poke some more to figure out what's happening.
EXPECT_TRUE(input.Unwrap<HTMLInputElement>()->IsInCanvasSubtree());Remove this line (this is tested on line 238)
Done
// Because of bug 1, the inner element flags are not set to true, remainingnit: remove "Because of bug 1, "
Done
const HeapVector<Member<Node>>& AssignedNodesNoRecalc() const {WDYT of this comment?
```
// Use AssignedNodes() in almost all cases. Only use AssignedNodesNoRecalc()
// when calling RecalcAssignment() is forbidden or dangerous (e.g., during tree
// modifications, moveBefore semantics, or state propagation like
// Element::SetIsCanvasOrInCanvasSubtree where eventual consistency is
// guaranteed by subsequent FlatTreeParentChanged/RemovedFromFlatTree hooks).
```
- Philip Rogers
- Rune Lillesveen
- Stefan Zager
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. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
if (IsA<HTMLCanvasElement>(*this)) {Stephen ChenneyDid we lose an early-out here?
```
void Element::SetIsCanvasOrInCanvasSubtree(bool value) {
value = value || IsA<HTMLCanvasElement>(*this);
if (value == IsCanvasOrInCanvasSubtree()) {
return;
}
```Without `value == IsCanvasOrInCanvasSubtree()`, we'll have a performance regression.
I tried to keep the early out. I wrote a DCHECK-enabled verifier to make sure that if we early outed the subtree was correct. That revealed the need to still process the children, so we can't early out.
Now maybe we can still early out if we call this method on a different element at the various call sites, like on the slotted thing and not the slot or something similar. If we try that we definitely need the verifier in place. I would leave it for a follow up where I also try to reduce the cost of Element::IsInCanvasSubtree().
The calls to `SetIsCanvasOrInCanvasSubtree` are not guarded by the feature flag, so this change will result in a significant perf regression.
Can you see if a testcase can be made from the case where the DCHECK verifier was firing which demonstrates a bug? Is it possible that the DCHECK verifier incorrectly fired due the calls to SetIsCanvasOrInCanvasSubtree during slotting? Or, is it possible that the DCHECK verifier was firing due to one of the bugs we've subsequently fixed? Maybe best to investigate this after the canvas-slot fix.
Related details
- Rune Lillesveen
- Stefan Zager
- Stephen Chenney
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. |
Stephen Chenney (Gerrit)
Stephen Chenney voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Rune Lillesveen
- Stefan Zager
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. |
Stephen Chenney (Gerrit)
Stephen Chenney added 1 comment![]( "Open in Gerrit")
if (IsA<HTMLCanvasElement>(*this)) {Stephen ChenneyDid we lose an early-out here?
```
void Element::SetIsCanvasOrInCanvasSubtree(bool value) {
value = value || IsA<HTMLCanvasElement>(*this);
if (value == IsCanvasOrInCanvasSubtree()) {
return;
}
```Without `value == IsCanvasOrInCanvasSubtree()`, we'll have a performance regression.
Philip RogersI tried to keep the early out. I wrote a DCHECK-enabled verifier to make sure that if we early outed the subtree was correct. That revealed the need to still process the children, so we can't early out.
Now maybe we can still early out if we call this method on a different element at the various call sites, like on the slotted thing and not the slot or something similar. If we try that we definitely need the verifier in place. I would leave it for a follow up where I also try to reduce the cost of Element::IsInCanvasSubtree().
The calls to `SetIsCanvasOrInCanvasSubtree` are not guarded by the feature flag, so this change will result in a significant perf regression.
Can you see if a testcase can be made from the case where the DCHECK verifier was firing which demonstrates a bug? Is it possible that the DCHECK verifier incorrectly fired due the calls to SetIsCanvasOrInCanvasSubtree during slotting? Or, is it possible that the DCHECK verifier was firing due to one of the bugs we've subsequently fixed? Maybe best to investigate this after the canvas-slot fix.
The verifier had some issues, now fixed. So it's now enabled for all DCHECK builds and doesn't cause and unit tests to fail.
Related details
- Philip Rogers
- Rune Lillesveen
- Stefan Zager