[HiC] Fix issues with shadow dom canvas children [chromium/src : main]
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) {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?
if (IsPseudoElement()) {Stephen ChenneyThis block is probably doing the same thing as Node::FlatTreeParentForChildDirty(). Might want to consolidate into some Node::FlatTreeParentWithoutRecalc() if they are actually the same.
Acknowledged
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. |
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. |