1586916 - Non-anonymous deep-tree-walker doesn't throw when set to anonymous node

Status: NEW

(Core :: DOM: Core & HTML, defect, P3)

Description

[Brendan Dahl [:bdahl]]

While porting test_bug522601.xhtml to use the shadow DOM, I found the tree walker doesn't throw like the old XBL implementation. For now, I'll mark the test todo in the new file test_bug522601-shadow.xhtml.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Probably because nodes in the shadow tree aren't anonymous from their parent's point of view, which is what the checks in the two-arg version of inDeepTreeWalker::SetCurrentNode seem to be checking. And even those checks were broken if the parent had only anonymous kids, since they are only done when the sibling list is nonempty!

Comment 2

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)

And even those checks were broken if the parent had only anonymous kids, since they are only done when the sibling list is nonempty!

Beside above bug, right now inIDeepTreeWalker.showAnonymousContent=false also hides the shadow dom,

• If this is the behavior we expect, then https://searchfox.org/mozilla-central/rev/97976753a21c1731e18177de9e5ce78ea3b3da2d/layout/inspector/tests/test_bug522601-shadow.xhtml#171 should throw an exception, otherwise, the tree traverse will be in a weird state. And we should also rename the showAnonymousContent to showAnonymousContentAndShadowDOM or something.
• If we don't expect inIDeepTreeWalker.showAnonymousContent=false also hides the shadow dom, but only XBL instead. Then the traverse result of showAnonymousContent=false is wrong and https://searchfox.org/mozilla-central/rev/97976753a21c1731e18177de9e5ce78ea3b3da2d/layout/inspector/tests/test_bug522601-shadow.xhtml#171 should not throw.

The only api consumer is Devtool/inspector, I think we might need DevTool people gives some input regarding what is the behavior they expect.
Hsinyi, do you know who from Devtool could provide some input? Thanks.

Comment 3

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

(In reply to Edgar Chen [:edgar] from comment #2)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)

And even those checks were broken if the parent had only anonymous kids, since they are only done when the sibling list is nonempty!

Beside above bug, right now inIDeepTreeWalker.showAnonymousContent=false also hides the shadow dom,

• If this is the behavior we expect, then https://searchfox.org/mozilla-central/rev/97976753a21c1731e18177de9e5ce78ea3b3da2d/layout/inspector/tests/test_bug522601-shadow.xhtml#171 should throw an exception, otherwise, the tree traverse will be in a weird state. And we should also rename the showAnonymousContent to showAnonymousContentAndShadowDOM or something.
• If we don't expect inIDeepTreeWalker.showAnonymousContent=false also hides the shadow dom, but only XBL instead. Then the traverse result of showAnonymousContent=false is wrong and https://searchfox.org/mozilla-central/rev/97976753a21c1731e18177de9e5ce78ea3b3da2d/layout/inspector/tests/test_bug522601-shadow.xhtml#171 should not throw.

The only api consumer is Devtool/inspector, I think we might need DevTool people gives some input regarding what is the behavior they expect.
Hsinyi, do you know who from Devtool could provide some input? Thanks.

Hi Gabriel, are you the right person for the question above?

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Julian and Brian might know more about this than I do.

Comment 5

[Brian Grinstead [:bgrins]]

I think Julian is the right person to answer. I'll just add the context that we don't need to worry about handling XBL in the inspector anymore.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Right, the only question here is how Shadow DOM should be treated in this stuff.

Comment 7

[Julian Descottes [:jdescottes]]

Right now the inspector is combining a walker with showAnonymousContent=false and one with showAnonymousContent=true in order to display webcomponents in the markup view.

Using the example at https://simple-webcomponent.glitch.me/
markup:

  <test-component >
    <div slot="slot1">slot1-1</div>
    <div slot="unslotted">unslotted</div>
  </test-component>

component

  defineComponent('test-component', `
    <div id="slot1-container">
      <slot name="slot1"></slot>
    </div>`);

In the markup view, this gets rendered as:

  <test-component>
    #shadow-root
      <div id="slot1-container">
        <slot name="slot1">
           <div> ->
        </slot>
      </div>
    <div slot="slot1">slot1-1</div>
    <div slot="unslotted">unslotted</div>
  </test-component>

In terms of walkers, to build this display, when we are on the <test-component> element, we create a walker with showAnonymousContent=false because we know it will see the direct children of this element, ie

    <div slot="slot1">slot1-1</div>
    <div slot="unslotted">unslotted</div>

If this non anonymous walker starts to return shadowDOM as well, it would also see <div id="slot1-container">?
How would the walker jump from the shadow dom children to the direct children? Via nextSibling()?
Overall I feel that such a change would just make it harder for us to differentiate the direct children from the shadow dom children, but maybe I don't understand exactly how you plan to implement this.

Another thing, today <div slot="unslotted">unslotted</div> is only accessible with a showAnonymousContent=false anonymous walker (because the element is nowhere in the actual DOM tree). With the proposed change, would this element still be available via a walker?

I am open to change the APIs we use on DevTools side, but we will need to make sure we can still implement the features above.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I don't think there is a proposed change here so far; just an attempt to figure out what devtools needs. Sounds like we do want to keep the concept of walkers that do or don't show anonymous content and include shadow DOM in that content. And that we should treat the shadow root as not being part of that shadow DOM, effectively, and fail attempts to set a non-anonymous-returning walker to point to a direct child of a shadow root, right?