Closed Bug 1053898 Opened 9 years ago Closed 5 years ago

Show shadow roots in markup view

Categories

(DevTools :: Inspector, enhancement, P3)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: bgrins, Assigned: jdescottes)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(21 files, 8 obsolete files)

Bug 1053898 - Update browser_markup_anonymous_03 test for new shadowdom display;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1053898 - Update DocumentWalker constructor to use optional object argument;
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1053898 - Update DocumentWalker constructor to support showAnonymousContent parameter;
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1053898 - Remove unused WalkerActor methods parents() and siblings;
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1053898 - Update framework/selection to support slotted selections;
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1053898 - Update framework/selection setNodeFront signature to use object argument;
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1053898 - Add integration tests for DevTools inspector shadow dom support;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1053898 - Update WalkerActor to return shadowRoot as child of host element;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1053898 - Update markup view to display shadow roots as #shadow-root;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1053898 - Update Walker and Node actors to listen to slotchange events on shadow roots;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1053898 - Update Walker and Node actors to observe mutations in shadow roots;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1053898 - Update WalkerActor to return light DOM nodes as children of host element;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1053898 - Update NodeActor with new properties to detect slotted nodes in markup-view;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1053898 - Update MarkupView to track the hovered container rather than the hovered nodeFront;
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1053898 - Add _markContainerAsSelected() API in MarkupView;
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1053898 - Add setContainer and hasContainer APIs in MarkupView;
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1053898 - Display slotted nodes in markup view;
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1053898 - Update inspector to prevent context-menu on slotted elements;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1053898 - Replace ElementContainer _buildEventTooltipContent() with generic click handler;
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1053898 - Update SlottedNodeEditor to show a reveal link on hover;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1053898 - Update WalkerActor to return ::after,::before in children() of host element;
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
Continuing the discussion about displaying shadow roots in the markup view with the deepTreeWalker (from https://bugzilla.mozilla.org/show_bug.cgi?id=777674#c51 to https://bugzilla.mozilla.org/show_bug.cgi?id=777674#c62).  Once we have anonymous content inspection we will want to show shadow roots in the inspector.
It would be good to narrow down what to show and how it should displayed in the markup view (especially with regards to older shadow roots and insertion points, I think).

Angelina, with the work and research you've done with using web components, did you find that the implementation in Chrome's inspector covered everything you needed, or was it lacking anything?  Any other thoughts or ideas about what we should be showing to developers in the inspector as they are working with shadow DOM?
Flags: needinfo?(afabbro)
In the screenshot I've attached You'll see that there are three separate shadow roots on a single (custom) element, airship-omnibar.

In Chrome, every time you add another shadow root, it is appended onto the markup in the inspector. There is no visual indication of which shadow root is the youngest shadow root, or the older shadow root. We should have a visual indication of which tree is the youngest shadow and which is the older shadow. I suppose it's fairly intuitive that the 'last one added is at the bottom, and it's the youngest' but we can be more clear than that about the state of the trees and their relationship.

In the case where one shadow root is embedded in another, it might be nice to click on a <shadow> tag, and then have the corresponding shadow that is at that insertion point highlight in the inspector. Similarly, when clicking on <content select="someSelector"> tags, highlight the corresponding children of the parent node that are distributed at that insertion point.

The 'line down the side' to indicate which elements are a part of the shadow tree (kind of like comment threads found around the 'net) I'm not a fan of, I think when you have a few trees nested (and IMHO I don't know why you'd have more than two, but still, imagine a complicated parent tree structure to the shadow trees) that interface idea tends to break down and look visually cluttered - instead perhaps we should tint the background a light grey or light blue - some visual contrast that isn't vertical lines, basically.
Flags: needinfo?(afabbro)
This is great stuff Angelina, thanks, and what do you think about the problem we discussed in the comments mentioned above? I think https://bugzilla.mozilla.org/show_bug.cgi?id=777674#c58 is the best summary. Both William and I are against the approach chrome took in general, and would prefer to focus on representing the composed tree by default.
Flags: needinfo?(afabbro)
See Also: → 1066672
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> This is great stuff Angelina, thanks, and what do you think about the
> problem we discussed in the comments mentioned above? I think
> https://bugzilla.mozilla.org/show_bug.cgi?id=777674#c58 is the best summary.
> Both William and I are against the approach chrome took in general, and
> would prefer to focus on representing the composed tree by default.

I'm not sure I fully understand the suggested approach in https://bugzilla.mozilla.org/show_bug.cgi?id=777674#c58.  Regarding 'Children of a host node are very different from the shadow root', where else would you put them in the markup view?

Maybe it would help me understand if you could give an example of what you think the markup tree would ideally look like in attachment 8477120 [details].  Here is the tree from the screenshot as I see it in Chrome:

<div id="userInterface">
  ▶ <airship-tabbar>…</airship-tabbar>
  ▼ <airship-omnibar>
      ▼ #shadow-root
        ▶ <style>…</style>
        ▶ <input type="text">
        ▶ <input type="submit">
      ▼ #shadow-root
        ▶ <h1>HI THERE FRIEND</h1>
      ▼ #shadow-root
        <span>There should be a greeting after this:</span>
        <shadow></shadow>
    <airship-omnibar>
</div>
Flags: needinfo?(gkrizsanits)
(In reply to Brian Grinstead [:bgrins] from comment #4)
> I'm not sure I fully understand the suggested approach in
> https://bugzilla.mozilla.org/show_bug.cgi?id=777674#c58.  Regarding
> 'Children of a host node are very different from the shadow root', where
> else would you put them in the markup view?
> 
> Maybe it would help me understand if you could give an example of what you
> think the markup tree would ideally look like in attachment 8477120 [details]
> [details].  Here is the tree from the screenshot as I see it in Chrome:

Since I don't have the code for this tree and the picture shows only a subsection of it,
that does not show the most important <conent> instantiation points, I cannot do that.

But if you open up http://w3c.github.io/webcomponents/spec/shadow/ and go to section 3.4,
there is a very useful image there. On the left hand side there are the light DOM tree and
on the right hand side the composed tree. What I think Chen was pointing out that as you
can see in the the composed tree (what is actually rendered to the screen and hence the most
important thing to inspect imo) the layout is very different than what you see in the light DOM
tree. If anyone is curious ever about the layout on the left hand side, one can just look at the
code and it will tell you exactly that structure, regards of parents/children.

But sometimes it can be very difficult to imagine the actual layout that will be rendered, which
is the composed tree, just by reading the code, even this tiny example is hard to follow at first.

This is the reason why I would like to let users inspect this composed tree instead.

What Angelina suggested, is basically showing the light DOM tree + coloring the nodes that will be selected when a content/shadow insertion point is selected. It is one big step forward from what chrome has. It might be a viable alternative. But for example it does not show the order of the colored nodes. Also, if one of those nodes is another shadow insertion point, then what should happen exactly? It feels to me that for real life complex examples will be still a big challenge to see the actual composed tree even with the help of the colored highlighter helper...

So let me try. I will denote the children with c[n] and shadow hosts with sh[n] also the children without number on the last image will be c4 and c5.

The composed tree would look like this: (right)

▼ Document
   ▼ sh 1
      ▼ sh2
         ▼ c4
            ▼ c1
            ▼ c3
            ▼ c5
      ▼ c2

While the light DOM version would look like:

▼ Document
  ▼ c1
  ▼ # shadow-root
    ▼ sh1
      ▼ <content select=...></content>
      ▼ c3
      ▼ #shadow-root
        ▼ c4
          ▼ <content ...></content>
          ▼ c5
    ▼ <content select=...></content>  <-- selecting this will color c4
  ▼ c2


Let me know if it makes sense now or not... (And sorry if I made any mistake in these examples, but I hope they good enough to illustrate the two different concepts)
Flags: needinfo?(gkrizsanits)
Depends on: 920141
See Also: → 1079185
There is a report in Bug 1079185 that landing the current level of inspection for shadow DOM is less useful than it was before we landed anonymous content inspection (for a page like: http://gaia-components.github.io/gaia-components).  It's worth checking out, it seems pretty related to this discussion.
I'm pretty sure that the end of this, is that we need to support inspection for both the light DOM and the for the composed tree. I'm not sure how should we show both word, or maybe we should just add a switch somewhere...
Brian, while I'm fixing bug 1079185, do you think you have some time to go forth with the other inspection mode? Since I think we will need both and then just add a flag to switch between the two, or do some more fancy UI for delivering it nicely. It should be simple, you should just use the deeptreewalker, without showing anon content, and then check the nodes if they have shadowRoot (it's a prop on the nodes). It returns the youngest shadow root, and then you can get olderShadowRoot recursively until there is any more to get all the shadow roots attached to that node. Finally you should be able to create another deeptreewalker on these document fragments (show anon content still off) and this way you will be able to see the light DOM version of the shadow trees. Does this make any sense? Let me know if something is not clear, or if something is broken with the approach.
Flags: needinfo?(bgrinstead)
We had a discussion about a better way to visualize things.  I'm thinking that this new way will require quite some work on the frontend, so I'm now feeling like we should proceed with the idea in Comment 8 to show a view by default similar to what Chrome does.  And then iterate on that to improve the visualization to help people better understand how things are being displayed.
Flags: needinfo?(bgrinstead)
Flags: needinfo?(afabbro)
We from gaia QA automation would very much like this to have this for webIDE. It would make the search for elements much easier (with shadow DOM we currently use a workaround).
Should I file a new about this for webIDE or would this one also cover webIDE?
(In reply to Martijn Wargers [:mwargers] (QA) from comment #10)
> Should I file a new about this for webIDE or would this one also cover
> webIDE?

No, this bug is enough. There is nothing WebIDE specific.
You should convince this bug needs more attention...
Was reminded about this bug again today by Sara Soueidan.
See https://twitter.com/SaraSoueidan/status/598311324104396800 and https://twitter.com/patrickbrosset/status/598413884974825472
This bug isn't on my TODO list right now but if no one works on it in the meantime, I might take a look in a about month.
Assignee: nobody → gabriel.luong
Attached patch 1053898.patch (obsolete) — — Splinter Review 

    
    

    
  
ping, what's the status here? Still getting pinged by people about it:

https://twitter.com/anatudor/status/628684488470605825
Flags: needinfo?(pbrosset)
Flags: needinfo?(gabriel.luong)

    
    

    
  
(In reply to Jeff Griffiths (:canuckistani) from comment #14)
> ping, what's the status here? Still getting pinged by people about it:
> 
> https://twitter.com/anatudor/status/628684488470605825

(In reply to Jeff Griffiths (:canuckistani) from comment #14)
> ping, what's the status here? Still getting pinged by people about it:
> 
> https://twitter.com/anatudor/status/628684488470605825

Backlogged after promise debugger work on my queue.
Flags: needinfo?(gabriel.luong)

    
    

    
  
(In reply to Jeff Griffiths (:canuckistani) from comment #14)
> ping, what's the status here? Still getting pinged by people about it:
> 
> https://twitter.com/anatudor/status/628684488470605825

It's also worth noting that the feature itself is still off by default for web content: https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#1101

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #16)
...
> It's also worth noting that the feature itself is still off by default for
> web content:
> https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.
> js#1101

Yeah, that is helpful. If there is a bug for enabling shadow roots or whatever, that bug should block this bug.

    
    

    
  
From the looks of that pref, bug 811542 should block this I guess.
Depends on: webcomponents
Flags: needinfo?(pbrosset)

    
    

    
  
I really would like to have something like this for WebIDE, fwiw.

    
  
See Also:  → 1078374

    
    

    
  
Inspector Bugs Triage - Filter on CLIMBING SHOES
Priority: -- → P3
Assignee: gl → nobody

    
    

    
  
Hsin-Yi and I just chatted about this on Slack.

In order for devtools to display shadow DOM, it would make it a lot easier if the inDeepTreeWalker API we use would allow us to walk it too.
This is the main platform API that the inspector uses currently. It uses it to walk nodes of all types, including frames and there content documents, pseudo-elements and anonymous content.

Using content APIs like el.assignedNodes would work but would make it a lot harder to implement and maintain the code. So I'm really hoping that we could have everything the inspector can possibly show be managed behind this platform API.

    
    

    
  
(In reply to Patrick Brosset <:pbro> from comment #21)
> In order for devtools to display shadow DOM, it would make it a lot easier
> if the inDeepTreeWalker API we use would allow us to walk it too.
> This is the main platform API that the inspector uses currently. It uses it
> to walk nodes of all types, including frames and there content documents,
> pseudo-elements and anonymous content.

inDeepTreeWalker uses AllChildrenIterator, so it should "just work", then.

    
    

    
  
(In reply to Emilio Cobos Álvarez [:emilio] from comment #22)
> (In reply to Patrick Brosset <:pbro> from comment #21)
> > In order for devtools to display shadow DOM, it would make it a lot easier
> > if the inDeepTreeWalker API we use would allow us to walk it too.
> > This is the main platform API that the inspector uses currently. It uses it
> > to walk nodes of all types, including frames and there content documents,
> > pseudo-elements and anonymous content.
> 
> inDeepTreeWalker uses AllChildrenIterator, so it should "just work", then.

AllChildrenIterator should work for flattened tree, but if we'd like to show something like what Chrome has (showing the light DOM tree + highlighting the nodes that are assigned when a slot/shadow insertion point is selected), then we need special handling in inDeepTreeWalker, right?

    
    

    
  
But that doesn't sound like something inDeepTreeWalker does. More like .assignedSlots.
Or is there some reason to basically expose assignedSlots in inDeepTreeWalker API? That can be done of course, should be trivial.

    
    

    
  
Hmm, so, special handling should be in devtools (WalkerActor maybe?), it should display the shadow tree when a shadow host is found (like in comment 8, but simpler, since v1 remove the support for multiple shadow roots). And for each slot in the shadow tree, display the assigned nodes or the fallback content as the slot's children (this can be done using inDeepTreeWalker API or the existing .assignedNodes API). "highlighting the nodes that are assigned when a slot/shadow insertion point is selected" is a plus.

    
    

    
  
We discussed this during our regular meeting today. We think DevTools can use the existing web APIs to implement this. 
- To know whether an element is a shadow host, check for element.shadowRoot (if non-null, it's a shadow host)
- If element is a slot, slot.assignedNodes [2] returns the nodes the in the light tree that are assigned to this slot, so these are the children of the slot
- If slot.assignedNodes is empty, use the slot's children instead (fallback content)
- TextNode/Element.assignedSlot [3] returns the slot that the element is assigned to

With the above APIs, you'll be able to construct something like what Chrome DevTools has. With the exception of shadow roots in "closed" mode, which returns null for these APIs. We can grant extra permission for DevTools accessing these APIs in closed mode though.

Do you think this is feasible? Feel free to add your comments on this, since we are not that familiar with DevTools neither.


[1] https://developer.mozilla.org/en-US/docs/Web/API/Element/shadowRoot
[2] https://developer.mozilla.org/en-US/docs/Web/API/HTMLSlotElement/assignedNodes
[3] https://developer.mozilla.org/en-US/docs/Web/API/Element/assignedSlot

    
    

    
  
Yeah, I think we need to do that, since inDeepTreeWalker isn't really designed for cases like Shadow DOM v1. Flatten tree doesn't have ShadowRoots, and we definitely do want ShadowRoots in devtools.

And if needed, one could create a inIDeepTreeWalker-looking JS wrapper on top of normal DOM APIs.
Would need to just decide where ShadowRoots are put in that view vs. unassigned child nodes.

    
    

    
  
(In reply to Jessica Jong [:jessica] from comment #26)
> We discussed this during our regular meeting today. We think DevTools can
> use the existing web APIs to implement this. 
> - To know whether an element is a shadow host, check for element.shadowRoot
> (if non-null, it's a shadow host)
Would this have any performance implications for DevTools? We would need to check this on each and every node we walk.
> - If element is a slot, slot.assignedNodes [2] returns the nodes the in the
> light tree that are assigned to this slot, so these are the children of the
> slot
Sounds good. Chrome does something a bit special here. It displays a sort of "link" to the node assigned to this slot. The node itself still appears in the light tree as normal, but under the <slot> element is a link you can click to jump to this element.
> - If slot.assignedNodes is empty, use the slot's children instead (fallback
> content)
Ok
> - TextNode/Element.assignedSlot [3] returns the slot that the element is
> assigned to
Not sure if we need this?

I'm not sure how our highlighter would work in those cases, but I guess we'd have to try and fix things along the way.

    
    

    
  
(In reply to Patrick Brosset <:pbro> from comment #28)
> (In reply to Jessica Jong [:jessica] from comment #26)
> > We discussed this during our regular meeting today. We think DevTools can
> > use the existing web APIs to implement this. 
> > - To know whether an element is a shadow host, check for element.shadowRoot
> > (if non-null, it's a shadow host)
> Would this have any performance implications for DevTools? We would need to
> check this on each and every node we walk.

Well, it involves some pointer checks, I think it's ok, we can compare the performance afterwards though.

> > - If element is a slot, slot.assignedNodes [2] returns the nodes the in the
> > light tree that are assigned to this slot, so these are the children of the
> > slot
> Sounds good. Chrome does something a bit special here. It displays a sort of
> "link" to the node assigned to this slot. The node itself still appears in
> the light tree as normal, but under the <slot> element is a link you can
> click to jump to this element.

Yes, it'd great if we had something similar too. Actually, this is the part where we said "highlighting the nodes that are assigned when a slot/shadow insertion point is selected", the last part should be "when the assigned node under slot/shadow insertion point is selected".

> > - If slot.assignedNodes is empty, use the slot's children instead (fallback
> > content)
> Ok
> > - TextNode/Element.assignedSlot [3] returns the slot that the element is
> > assigned to
> Not sure if we need this?

Maybe not, let's see.

> 
> I'm not sure how our highlighter would work in those cases, but I guess we'd
> have to try and fix things along the way.

    
    

    
  
Going to spend some time reviving this patch with the current spec
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED

    
  
Depends on: 1269527

    
  

        Attachment #8621914 -
        Attachment is obsolete: true

    
  

        Attachment #8477120 -
        Attachment is obsolete: true

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
  
Early patches, still needs polish & tests. Shows shadow roots as separate element, next to the light tree. Slotted elements are displayed as "links" when viewed inside the shadow tree. 

Most of the complexity from this patch is in the walker-actor which has to special case shadow hosts, shadow roots and slotted elements when fetching children of those elements. Probably need to apply custom logic in siblings() and parents() methods, but since those are not used by devtools (other than tests) I didn't update them for now. 

We also introduce a new shadow node actor to represent slotted elements that are displayed as links, with dedicated views (container and editor).

    
  

        Attachment #8944936 -
        Attachment is obsolete: true

    
    

    
  
A few random things I found while testing:
- Right-click/inspect-element doesn't work with shadow dom content (this only select the custom element). It does with slotted light dom however. Not sure how big a deal this is though.
- The rule-view's highlighter icons (next to selectors) get a bit confused for things inside shadow roots. The one next to the "element" rule seem to highlight all elements under the shadow root. And the ones next to actual CSS rules don't seem to do anything.

    
    

    
  
Thanks for testing! 

> - Right-click/inspect-element doesn't work with shadow dom content (this only select the custom element).
> It does with slotted light dom however. Not sure how big a deal this is though.

For now the event we get in ContextMenu.jsm seems to completely ignore the shadow dom nodes.The event.target is just the custom element. Even if we would get the correct node, right now we only have logic to create an array of selectors able to traverse frames. We would need to modify this a bit to handle traversing shadow roots. 

> The rule-view's highlighter icons (next to selectors) get a bit confused for things inside shadow roots. 
> The one next to the "element" rule seem to highlight all elements under the shadow root. And the ones 
> next to actual CSS rules don't seem to do anything.

A lot of the inspector features work by passing selectors around in order to find matching nodes. After that we match elements by doing node.ownerDocument.querySelector(selector). Some spots where we do that:
- https://searchfox.org/mozilla-central/rev/84cea84b12145d752e50ddca6be5462c38510e35/toolkit/modules/css-selector.js#52
- https://searchfox.org/mozilla-central/rev/84cea84b12145d752e50ddca6be5462c38510e35/devtools/server/actors/highlighters/selector.js#45

This is not really compatible with nodes found in the shadow DOM. E.g. if we have (as represented with this patch in the markup-view):

<html>
<body>
<test-component>
  #shadow-root
    <div class="shadow">
      <slot name="slot1">fallback</slot>
    </div>
  <div slot="slot1">some content</div>
</test-component>

If we select the node with class "shadow", node.ownerDocument will point to the root document, but node.ownerDocument.querySelectorAll(".shadow") will return an empty NodeList. 

As far as I can tell, it looks like we have to detect if the node is under a shadow root, and in this case use the shadow root instead of node.ownerDocument in such methods. 

I don't know if there is an API to get the "owner" shadow root from a given node? Right now I climb up the tree until I find a shadow root to workaround the issue, but there is too much performance impact. Alternatively we could probably persist the information that "this node is under a shadow root" and pass it around everywhere we need, with a selector to retrieve the shadow root host.

smaug: do you know if there is any API similar to ownerDocument, that would return the shadow root to which a node belongs?
Flags: needinfo?(bugs)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8948536 [details]
Bug 1053898 - WIP: support and display shadow nodes

https://reviewboard.mozilla.org/r/217956/#review224870


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/modules/css-selector.js:130
(Diff revision 2)
>    // continue recursing up until it is unique enough.
>    if (ele.parentNode !== document) {
>      index = positionInNodeList(ele, ele.parentNode.children) + 1;
>      selector = findCssSelector(ele.parentNode) + " > " +
>        cssEscape(tagName) + ":nth-child(" + index + ")";
> +  } else if (!!shadowRoot) {

Error: Redundant double negation. [eslint: no-extra-boolean-cast]

    
    

    
  
(In reply to Julian Descottes [:jdescottes][:julian] from comment #44)
> smaug: do you know if there is any API similar to ownerDocument, that would
> return the shadow root to which a node belongs?

getRootNode?
https://dom.spec.whatwg.org/#dom-node-getrootnode
Flags: needinfo?(bugs)

    
  
Assignee: bgrinstead → jdescottes

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8948532 -
        Attachment is obsolete: true

    
  

        Attachment #8948533 -
        Attachment is obsolete: true

    
  

        Attachment #8948535 -
        Attachment is obsolete: true

    
  

        Attachment #8948536 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8948531 [details]
Bug 1053898 - Update framework/selection to support slotted selections;

Brian: let me know what you think about the approach. There are still APIs in the walker-actor.js that need to be updated to support the shadowDOM case, but as they have no consumer today I haven't touched them yet (parents() and siblings())

I think we could simplify a bit the whole thing and get rid of the slotted-node-actor and instead have everything in the node actor.

        Attachment #8948531 -
        Flags: feedback?(bgrinstead)

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #56)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #44)
> > smaug: do you know if there is any API similar to ownerDocument, that would
> > return the shadow root to which a node belongs?
> 
> getRootNode?
> https://dom.spec.whatwg.org/#dom-node-getrootnode

Perfect, that's what I was looking for.
Severity: normal → enhancement

    
    

    
  
Let's say I have a page with 

  <test-component>
    <div slot="slot1">content</div>
  </test-component>

and without any matching custom element in the beginning. 

Then dynamically the page calls customElements.define('test-component', ...). If the inspector is already viewing <test-component> we need to update its content. Is there any event or mutation we are supposed to receive in this situation?
Flags: needinfo?(bugs)

    
    

    
  
You mean some notification to tell if test-component constructor ends up calling attachShadow and that creates some slots which get assigned?
I guess slotchange should fire. Normally that stays within the shadow dom, but it should also propagate to
chrome event handler, so in practice to frame script.
Flags: needinfo?(bugs)

    
    

    
  
(In reply to Julian Descottes [:jdescottes][:julian] from comment #63)
> Comment on attachment 8948531 [details]
> Bug 1053898 - WIP support and display shadow roots
> 
> Brian: let me know what you think about the approach. There are still APIs
> in the walker-actor.js that need to be updated to support the shadowDOM
> case, but as they have no consumer today I haven't touched them yet
> (parents() and siblings())

Since they are unused, let's delete those functions in a commit at the beginning of the queue just to make things easier.

    
    

    
  
Comment on attachment 8948531 [details]
Bug 1053898 - Update framework/selection to support slotted selections;

https://reviewboard.mozilla.org/r/217946/#review226122

::: devtools/server/actors/inspector/walker-actor.js:703
(Diff revision 3)
>  
>      // If there's any room left, it means we've run all the way to the end.
>      // If we're centering, check if there are more items to read at the front.
>      let remaining = maxNodes - nodes.length;
>      if (options.center && remaining > 0 && nodes[0].rawNode != firstChild) {
> -      let firstNodes = this._readBackward(backwardWalker, remaining);
> +      walker.setStartingNode = lastBackwardNode;

This should be `walker.setStartingNode(lastBackwardNode)`

    
    

    
  
Comment on attachment 8948531 [details]
Bug 1053898 - Update framework/selection to support slotted selections;

This looks good, I like the approach. As we discussed, if there's a way to drop the slotted-node-actor and instead have early returns in relevant the node-actor functions that'll be a bit easier to deal with.

        Attachment #8948531 -
        Flags: feedback?(bgrinstead) → feedback+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  
Depends on: 1414286

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  
See Also:  → 1443923

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8948534 -
        Attachment is obsolete: true

    
    

    
  
Brian, Gabe, Patrick: I tried to split the review requests so that Brian (who followed the implementation) could review all implementation changes. Reviews assigned to Gabriel and Patrick are refactors needed for the feature but that should not impact the behavior of the inspector. Of course, feel free to review other changesets if you see something relevant to add.

    
    

    
  
One thing to note, integration tests are failing if Stylo is disabled, so I probably need to skip them on such platforms?

    
    

    
  
(In reply to Julian Descottes [:jdescottes][:julian] from comment #139)
> One thing to note, integration tests are failing if Stylo is disabled, so I
> probably need to skip them on such platforms?

Yeah, shadow DOM is only enabled with stylo.

skip-if = !stylo

should do.

    
    

    
  
Comment on attachment 8948528 [details]
Bug 1053898 - Update DocumentWalker constructor to use optional object argument;

https://reviewboard.mozilla.org/r/217940/#review231942

        Attachment #8948528 -
        Flags: review?(pbrosset) → review+

    
    

    
  
Comment on attachment 8948529 [details]
Bug 1053898 - Update DocumentWalker constructor to support showAnonymousContent parameter;

https://reviewboard.mozilla.org/r/217942/#review231946

        Attachment #8948529 -
        Flags: review?(pbrosset) → review+

    
    

    
  
Comment on attachment 8948530 [details]
Bug 1053898 - Remove unused WalkerActor methods parents() and siblings;

https://reviewboard.mozilla.org/r/217944/#review231950

::: devtools/server/actors/inspector/walker.js:200
(Diff revision 8)
> -  getDocumentWalker: function (node, whatToShow, skipTo) {
> +  getDocumentWalker: function (node, {whatToShow, skipTo, showAnonymousContent} = {}) {
>      // Allow native anon content (like <video> controls) if preffed on
> -    let nodeFilter = this.showAllAnonymousContent
> +    let filter = this.showAllAnonymousContent ? allAnonymousContentTreeWalkerFilter
> -                    ? allAnonymousContentTreeWalkerFilter
> -                    : standardTreeWalkerFilter;
> +                                              : standardTreeWalkerFilter;
> -    return new DocumentWalker(node, this.rootWin, {whatToShow, nodeFilter, skipTo});
> +
> +    return new DocumentWalker(node, this.rootWin,
> +      {whatToShow, filter, skipTo, showAnonymousContent});

I'm a bit confused here. We have 2 different mechanisms in place here that both deal with anonymous content:
- the filter, which only depends on a pref
- the showAnonymousContent property, which depends on a function argument
Can you help clarify please? Some jsdoc needed perhaps?

        Attachment #8948530 -
        Flags: review?(pbrosset)

    
    

    
  
Comment on attachment 8948530 [details]
Bug 1053898 - Remove unused WalkerActor methods parents() and siblings;

https://reviewboard.mozilla.org/r/217944/#review231950

> I'm a bit confused here. We have 2 different mechanisms in place here that both deal with anonymous content:
> - the filter, which only depends on a pref
> - the showAnonymousContent property, which depends on a function argument
> Can you help clarify please? Some jsdoc needed perhaps?

Good catch. It is confusing because, before, anonymous walkers used to return everything we needed. We could simply filter the result. Now we need to use non-anonymous walkers to get access to information that is hidden from the anonymous walkers (or rather that would be quite complicated to retrieve).

So we definitely should add some jsdoc here, and we might want to use the allAnonymous...Filter only if both the preference and the argument are true.

Alternatively, maybe we could have a dedicated method called getLightDomWalker, that would always create a Walker with standardFilter and showAnonymousContent to false. In most spots where I call getDocumentWalker with showAnonymousContent to false, it is done behind a check for "is this node in light DOM" or similar, so that should be easy to translate.

There's only one case where getLightDomWalker would not be a good fit. When retrieving children for a shadow root, we use such a walker for a reason unrelated to light dom. But we could use a regular (ie anonymous) walker with another filter. This would probably make the intent clearer as well.

    
    

    
  
Gabriel, Patrick: I have mostly discussed the overall approach for this feature with Brian before, it would be great to also get your opinion on the strategy here. Most importantly if you agree about the split of responsibilities between client and server.

To summarize the strategy used here:
- 1/ server now uses non-anonymous walkers in order to retrieve the light DOM of a host element
- 2/ server watches slotchange and mutations on shadowroots
- 3/ server returns both the light DOM and a shadow root as children of a host element
- 4/ server returns light DOM nodes inserted in <slot> elements as children of the slot as well, (returns the same nodeActor as the one returned as child of the host element)
- 5/ client now supports having two containers potentially representing a single nodeFront (which also impacts our selection)
- 6/ client uses a dedicated container and editor to represent the slotted nodes

I initially had another approach for step 4 and 5. Instead of returning the same NodeActor for slotted elements, I used to create new NodeActors to represent the slotted version. Changes to the markup-view and selection were much smaller, but the walker-actor was more complex. I was also concerned with the memory management on the actor side. This last point is the main reason why I favored the current solution.
Flags: needinfo?(pbrosset)
Flags: needinfo?(gl)

    
    

    
  
(In reply to Julian Descottes [:jdescottes][:julian] from comment #145)
> - 1/ server now uses non-anonymous walkers in order to retrieve the light
> DOM of a host element
Can you explain what happens when devtools.inspector.showAllAnonymousContent is true then?

> - 4/ server returns light DOM nodes inserted in <slot> elements as children
> of the slot as well, (returns the same nodeActor as the one returned as
> child of the host element)
Sending the same nodeActor makes sense to me. They are the same node. And using the same nodeActor probably makes the highlighter just work, without any other code needed.

> - 5/ client now supports having two containers potentially representing a
> single nodeFront (which also impacts our selection)
I see how this might have been a lot of code changes and added complexity. But I also see this as a potential for future features. Just being able to do this removes an arbitrary constraint.

> I initially had another approach for step 4 and 5. Instead of returning the
> same NodeActor for slotted elements, I used to create new NodeActors to
> represent the slotted version. Changes to the markup-view and selection were
> much smaller, but the walker-actor was more complex. I was also concerned
> with the memory management on the actor side. This last point is the main
> reason why I favored the current solution.
Yeah, I like your current approach. One nodeActor == one DOM node. It's just it so happens that we're deciding to show some nodes multiple times, because of how we walk to DOM. Maybe there's a future where we only show slotted things and no light DOM, in which case it'll be supported easily with your change (though your initial approach would have worked too).
Flags: needinfo?(pbrosset)

    
    

    
  
Thanks for the feedback Patrick! 

> Can you explain what happens when devtools.inspector.showAllAnonymousContent is true then?

Non-anonymous walkers are used to fetch the direct children of shadow hosts and shadow roots (the latter because of a technical issue, which we could handle differently). In this case it means that nodes normally retrieved for showAllAnonymousContent would not show up. This is only about direct children. If a <div> is a light DOM child, and has <scrollbar>s, they will show up, because the children of the <div> are retrieved via an anonymous walker.

In the current implementation, when fetching children for the host element, we complete the information from the lightdom walker by using an anonymous walker, in order to get before/after elements. We should probably extend this to also retrieve other anonymous elements in case showAllAnonymousContent is set to true.

For shadow root children, right now we use a lightdom walker because otherwise we might jump to after/before elements that are not really children of the shadowroot. We could use a new filter here, that would always avoid before/after, but that would still respect showAllAnonymousContent for other anonymous nodes.

Finally the last case is about unslotted light DOM nodes. In this case we can only ever use a lightdom walker to traverse the nodes, but those nodes are not really in the tree. It's not even clear if we should display them at all (for instance, Chrome doesn't). 

Are you fine with moving this into a follow up bug?

    
    

    
  
(In reply to Julian Descottes [:jdescottes][:julian] from comment #147)
> Are you fine with moving this into a follow up bug?
Yes. And thanks for the detailed explanations.

    
    

    
  
Comment on attachment 8948531 [details]
Bug 1053898 - Update framework/selection to support slotted selections;

https://reviewboard.mozilla.org/r/217946/#review233986

::: devtools/client/framework/selection.js:129
(Diff revision 9)
> -  setNodeFront: function (value, reason = "unknown") {
> +  /**
> +   * Update the currently selected node-front.
> +   *
> +   * @param {NodeFront} nodeFront
> +   *        The NodeFront being selected.
> +   * @param {String} reason: Reason that triggered the selection, will be fired with

Provide the @param description on the next line like you have done with nodeFront.

::: devtools/client/framework/selection.js:131
(Diff revision 9)
> +   *
> +   * @param {NodeFront} nodeFront
> +   *        The NodeFront being selected.
> +   * @param {String} reason: Reason that triggered the selection, will be fired with
> +   *        the "new-node-front" event.
> +   * @param {Boolean} isSlotted: Is the selection representing the slotted version of

Same as above.

        Attachment #8948531 -
        Flags: review?(gl) → review+
Flags: needinfo?(gl)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8948530 [details]
Bug 1053898 - Remove unused WalkerActor methods parents() and siblings;

https://reviewboard.mozilla.org/r/217944/#review234588


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/actors/inspector/walker.js:412
(Diff revision 9)
>                ? this.rootDoc.documentElement
>                : nodeDocument(node.rawNode).documentElement;
>      return this._ref(elt);
>    },
>  
> -  /**
> +  parentNode: function (node) {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]

    
    

    
  
Comment on attachment 8948528 [details]
Bug 1053898 - Update DocumentWalker constructor to use optional object argument;

https://reviewboard.mozilla.org/r/217940/#review234590


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/actors/inspector/walker.js:412
(Diff revision 8)
>                ? this.rootDoc.documentElement
>                : nodeDocument(node.rawNode).documentElement;
>      return this._ref(elt);
>    },
>  
>    parentNode: function (node) {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
- rebased against recent central
- addressed the first batch of review comments
- added a changeset to remove unused functions from the walker actor
- try (still need to skip-if !stylo) https://treeherder.mozilla.org/#/jobs?repo=try&revision=281169af07965be28ab92730ac98b710a3a7c842

    
    

    
  
Comment on attachment 8948530 [details]
Bug 1053898 - Remove unused WalkerActor methods parents() and siblings;

https://reviewboard.mozilla.org/r/217944/#review235458

As discussed on slack, this looks good to me, but you also need to remove the methods specs in devtools\shared\specs\inspector.js

        Attachment #8948530 -
        Flags: review?(pbrosset) → review+

    
    

    
  
Comment on attachment 8956780 [details]
Bug 1053898 - Update WalkerActor to return shadowRoot as child of host element;

https://reviewboard.mozilla.org/r/225746/#review236378

        Attachment #8956780 -
        Flags: review?(bgrinstead) → review+

    
    

    
  
Comment on attachment 8956781 [details]
Bug 1053898 - Update markup view to display shadow roots as #shadow-root;

https://reviewboard.mozilla.org/r/225748/#review236380

        Attachment #8956781 -
        Flags: review?(bgrinstead) → review+

    
    

    
  
Comment on attachment 8956782 [details]
Bug 1053898 - Update Walker and Node actors to listen to slotchange events on shadow roots;

https://reviewboard.mozilla.org/r/225750/#review236382

::: devtools/server/actors/inspector/node.js:82
(Diff revision 4)
>        }
>        this.mutationObserver = null;
>      }
> +
> +    if (this.slotchangeListener) {
> +      if (!Cu.isDeadWrapper(this.rawNode)) {

Should we use the `isNodeDead(this)` from inspector/utils instead? IOW: can rawNode ever be null/undefined?

::: devtools/server/actors/inspector/walker.js:313
(Diff revision 4)
>  
>      if (node.nodeType === Ci.nsIDOMNode.DOCUMENT_NODE) {
>        actor.watchDocument(this.onMutations);
>      }
> +
> +    if (node.nodeType === Ci.nsIDOMNode.DOCUMENT_FRAGMENT_NODE && node.host) {

Just making a note to confirm that this will get replaced with a helper function for detecting shadow roots later on

        Attachment #8956782 -
        Flags: review?(bgrinstead) → review+

    
    

    
  
Comment on attachment 8956783 [details]
Bug 1053898 - Update Walker and Node actors to observe mutations in shadow roots;

https://reviewboard.mozilla.org/r/225752/#review236384

::: devtools/server/actors/inspector/walker.js:314
(Diff revision 4)
>      if (node.nodeType === Ci.nsIDOMNode.DOCUMENT_NODE) {
> -      actor.watchDocument(this.onMutations);
> +      actor.watchDocument(node, this.onMutations);
>      }
>  
>      if (node.nodeType === Ci.nsIDOMNode.DOCUMENT_FRAGMENT_NODE && node.host) {
> +      actor.watchDocument(node.host.ownerDocument, this.onMutations);

Is ownerDocument not always right on the shadow root? Seems like we shouldn't need to check for it on the host.

        Attachment #8956783 -
        Flags: review?(bgrinstead) → review+

    
    

    
  
Comment on attachment 8956784 [details]
Bug 1053898 - Update WalkerActor to return light DOM nodes as children of host element;

https://reviewboard.mozilla.org/r/225754/#review236386

::: devtools/server/actors/inspector/walker.js:433
(Diff revision 4)
>    parentNode: function(node) {
> -    let walker = this.getDocumentWalker(node.rawNode);
> -    let parent = walker.parentNode();
> +    let parent;
> +    try {
> +      // If the node is a slotted light DOM child, we can not use an anonymous walker to
> +      // get the shadow host parent.
> +      let walker = node.isLightDOMChild ? this.getLightDOMWalker(node.rawNode)

Just making a note to come back to this review when I finish the others - it seems like we may want to have a `getWalker` function that handles this case (or update `getDocumentWalker` to always the right thing for light dom children). I'm not sure if this happens (or if it will become not a good idea) later in the queue

    
    

    
  
Comment on attachment 8956793 [details]
Bug 1053898 - Update WalkerActor to return ::after,::before in children() of host element;

https://reviewboard.mozilla.org/r/225772/#review236388

::: devtools/server/actors/inspector/walker.js:721
(Diff revision 4)
> +      let {before, after} = this._getBeforeAfterElements(node.rawNode);
>        nodes = [
>          // #shadow-root
>          this._ref(node.rawNode.shadowRoot),
> +        // ::before
> +        ...(before ? [before] : []),

I wonder if ::before should be before the shadow root in the UI. No need to change anything here - that's follow up material.

        Attachment #8956793 -
        Flags: review?(bgrinstead) → review+

    
    

    
  
Comment on attachment 8956785 [details]
Bug 1053898 - Update NodeActor with new properties to detect slotted nodes in markup-view;

https://reviewboard.mozilla.org/r/225756/#review236390

::: devtools/client/inspector/markup/markup.js:1684
(Diff revision 4)
>          }
>  
>          let fragment = this.doc.createDocumentFragment();
>  
>          for (let child of children.nodes) {
> +          let isLightDOMChild = child.isLightDOMChild;

Is the term "light dom" only referring to nodes that are direct children of a shadow host, or does it also refer to any child node (even if there's no CE/SD involved)? i.e. with `<body><span /></body>` is the span "light dom"? We treat the answer to that as "no", so just want to make sure that is accurate.

        Attachment #8956785 -
        Flags: review?(bgrinstead) → review+

    
    

    
  
ni? for Comment 203
Flags: needinfo?(emilio)

    
  

        Attachment #8956789 -
        Flags: review?(bgrinstead) → review?(gl)

    
    

    
  
Comment on attachment 8956785 [details]
Bug 1053898 - Update NodeActor with new properties to detect slotted nodes in markup-view;

https://reviewboard.mozilla.org/r/225756/#review236392

::: devtools/client/inspector/markup/markup.js:1684
(Diff revision 4)
>          }
>  
>          let fragment = this.doc.createDocumentFragment();
>  
>          for (let child of children.nodes) {
> +          let isLightDOMChild = child.isLightDOMChild;

Yeah, light DOM is the normal dom tree too, so `isLightDOMChild` looks somewhat confusing.

You just want something like `isDirectShadowHostChild`, or `isLightDOMChildOfAShadowHost`, or something like that.

::: devtools/server/actors/inspector/node.js:204
(Diff revision 4)
>      return isFragment && this.rawNode.host;
>    },
>  
> +  get isShadowHost() {
> +    let shadowRoot = this.rawNode.shadowRoot;
> +    return shadowRoot && shadowRoot.nodeType === Ci.nsIDOMNode.DOCUMENT_FRAGMENT_NODE;

the document fragment condition is unnecessary I'd say.

If shadowRoot returns something that isn't a doc fragment that's a very severe bug, just `!!this.rawNode.shadowRoot` should do.

Also, this should probably also detect closed shadow roots, is there a followup bug for that? Or does devtools somehow have access to those?
Flags: needinfo?(emilio)

    
    

    
  
Comment on attachment 8956790 [details]
Bug 1053898 - Update inspector to prevent context-menu on slotted elements;

https://reviewboard.mozilla.org/r/225766/#review236394

        Attachment #8956790 -
        Flags: review?(bgrinstead) → review+

    
    

    
  
(In reply to Emilio Cobos Álvarez [:emilio] from comment #205)
> >          for (let child of children.nodes) {
> > +          let isLightDOMChild = child.isLightDOMChild;
> 
> Yeah, light DOM is the normal dom tree too, so `isLightDOMChild` looks
> somewhat confusing.
> 
> You just want something like `isDirectShadowHostChild`, or
> `isLightDOMChildOfAShadowHost`, or something like that.

OK, thanks. Julian - what do you think? Either of those, or `isShadowHostLightDOM` seem pretty clear to me so I'll leave it up to you.

> Also, this should probably also detect closed shadow roots, is there a
> followup bug for that? Or does devtools somehow have access to those?

I don't know if there's already one on file but I'd prefer to handle that in a follow up.

    
    

    
  
Comment on attachment 8952456 [details]
Bug 1053898 - Update framework/selection setNodeFront signature to use object argument;

https://reviewboard.mozilla.org/r/221680/#review236472

        Attachment #8952456 -
        Flags: review?(gl) → review+

        Attachment #8952457 -
        Flags: review?(gl)

    
    

    
  
Comment on attachment 8956786 [details]
Bug 1053898 - Update MarkupView to track the hovered container rather than the hovered nodeFront;

https://reviewboard.mozilla.org/r/225758/#review236474

        Attachment #8956786 -
        Flags: review?(gl) → review+

    
    

    
  
Comment on attachment 8956787 [details]
Bug 1053898 - Add _markContainerAsSelected() API in MarkupView;

https://reviewboard.mozilla.org/r/225760/#review236476

        Attachment #8956787 -
        Flags: review?(gl) → review+

    
    

    
  
Comment on attachment 8956788 [details]
Bug 1053898 - Add setContainer and hasContainer APIs in MarkupView;

https://reviewboard.mozilla.org/r/225762/#review236478

        Attachment #8956788 -
        Flags: review?(gl) → review+

    
    

    
  
Comment on attachment 8956791 [details]
Bug 1053898 - Replace ElementContainer _buildEventTooltipContent() with generic click handler;

https://reviewboard.mozilla.org/r/225768/#review236480

::: devtools/client/inspector/markup/views/element-container.js:56
(Diff revision 4)
> +    }
> +
> +    this._buildEventTooltipContent(event.target);
> +  },
> +
> +  _buildEventTooltipContent: Task.async(function* (target) {

We should use ES6 async await now.

        Attachment #8956791 -
        Flags: review?(gl) → review+

    
    

    
  
Comment on attachment 8956789 [details]
Bug 1053898 - Display slotted nodes in markup view;

https://reviewboard.mozilla.org/r/225764/#review236482

::: devtools/client/inspector/markup/markup.js:485
(Diff revision 4)
>      return promise.all([onShown, this._briefBoxModelPromise]);
>    },
>  
>    /**
>     * Get the MarkupContainer object for a given node, or undefined if
>     * none exists.

I think we want to add @param docs

        Attachment #8956789 -
        Flags: review?(gl) → review+

    
    

    
  
Comment on attachment 8948530 [details]
Bug 1053898 - Remove unused WalkerActor methods parents() and siblings;

https://reviewboard.mozilla.org/r/217944/#review235458

Done, thanks!

    
    

    
  
Comment on attachment 8956782 [details]
Bug 1053898 - Update Walker and Node actors to listen to slotchange events on shadow roots;

https://reviewboard.mozilla.org/r/225750/#review236382

> Should we use the `isNodeDead(this)` from inspector/utils instead? IOW: can rawNode ever be null/undefined?

I don't think rawNode should be null/undefined at any point unless this gets called after the destroy of the NodeActor, but I switched to isNodeDead to be safe, thanks!

> Just making a note to confirm that this will get replaced with a helper function for detecting shadow roots later on

Changed it to use NodeActor::isShadowRoot right away.

    
    

    
  
Comment on attachment 8956783 [details]
Bug 1053898 - Update Walker and Node actors to observe mutations in shadow roots;

https://reviewboard.mozilla.org/r/225752/#review236384

> Is ownerDocument not always right on the shadow root? Seems like we shouldn't need to check for it on the host.

Sure, no need to fetch the host here.

    
    

    
  
Comment on attachment 8956785 [details]
Bug 1053898 - Update NodeActor with new properties to detect slotted nodes in markup-view;

https://reviewboard.mozilla.org/r/225756/#review236392

> the document fragment condition is unnecessary I'd say.
> 
> If shadowRoot returns something that isn't a doc fragment that's a very severe bug, just `!!this.rawNode.shadowRoot` should do.
> 
> Also, this should probably also detect closed shadow roots, is there a followup bug for that? Or does devtools somehow have access to those?

Testing quickly I can fake a shadowRoot property on a DOM element via 
  Object.defineProperty($0, 'shadowRoot', { value: "not a shadow root" });
  $0.shadowRoot; // returns "not a shadow root"  

Based on that I guess I should keep the check. Should I log a bug to change that behavior?

Let's handle closed shadow roots in a follow up (Bug 1449333)

    
    

    
  
Comment on attachment 8956785 [details]
Bug 1053898 - Update NodeActor with new properties to detect slotted nodes in markup-view;

https://reviewboard.mozilla.org/r/225756/#review236390

> Is the term "light dom" only referring to nodes that are direct children of a shadow host, or does it also refer to any child node (even if there's no CE/SD involved)? i.e. with `<body><span /></body>` is the span "light dom"? We treat the answer to that as "no", so just want to make sure that is accurate.

Updated it with isDirectShadowHostChild, thanks!

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

        Attachment #8952457 -
        Flags: review?(gl)

    
    

    
  
Comment on attachment 8948527 [details]
Bug 1053898 - Update browser_markup_anonymous_03 test for new shadowdom display;

https://reviewboard.mozilla.org/r/217938/#review237300

        Attachment #8948527 -
        Flags: review?(bgrinstead) → review+

    
    

    
  
Comment on attachment 8956789 [details]
Bug 1053898 - Display slotted nodes in markup view;

I think Gabe would be a better reviewer for this one

        Attachment #8956789 -
        Flags: review?(bgrinstead) → review?(gl)

    
    

    
  
Comment on attachment 8956789 [details]
Bug 1053898 - Display slotted nodes in markup view;

https://reviewboard.mozilla.org/r/225764/#review237326

        Attachment #8956789 -
        Flags: review?(gl) → review+

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #242)
> Comment on attachment 8956789 [details]
> Bug 1053898 - Display slotted nodes in markup view;
> 
> I think Gabe would be a better reviewer for this one

I thought I already reviewed this.

    
    

    
  
Comment on attachment 8956784 [details]
Bug 1053898 - Update WalkerActor to return light DOM nodes as children of host element;

https://reviewboard.mozilla.org/r/225754/#review237322

Whew, there are some tricky cases to deal with here - thanks.

::: devtools/server/actors/inspector/node.js:200
(Diff revision 5)
>    get isShadowRoot() {
>      let isFragment = this.rawNode.nodeType === Ci.nsIDOMNode.DOCUMENT_FRAGMENT_NODE;
>      return isFragment && this.rawNode.host;
>    },
>  
> +  get isLightDOMChild() {

I see this gets renamed to isDirectShadowHostChild later. Technically we should land the name update in this changeset for blame purposes, but I'm OK to leave it as-is if that'll be easier.

::: devtools/server/actors/inspector/walker.js:437
(Diff revision 5)
> +      // get the shadow host parent.
> +      let walker = node.isLightDOMChild ? this.getLightDOMWalker(node.rawNode)
> +                                        : this.getDocumentWalker(node.rawNode);
> +      parent = walker.parentNode();
> +    } catch (e) {
> +      // When getting the parent node for a child of a non-slotted light DOM element,

Some of the terminology in comments of this changeset should be update to reflect we are only talking about light dom within shadow host. I guess 'non-slotted' implies that but would worth it to be explicit.

::: devtools/server/actors/inspector/walker.js:615
(Diff revision 5)
>      let isShadowHost = !!node.rawNode.shadowRoot;
> +    let isShadowRoot = !!node.rawNode.host;
>  
> -    if (isShadowHost) {
> -      let shadowRoot = this._ref(node.rawNode.shadowRoot);
> -      return {
> +    // Detect special case of unslotted light DOM children that cannot rely on a
> +    // regular anonymous walker.
> +    let isUnslottedLightDOM = false;

Ditto

::: devtools/server/actors/inspector/walker.js:636
(Diff revision 5)
> -      if (isShadowRoot) {
> -        // Do not fetch anonymous children for shadow roots. If the host element has an
> -        // ::after pseudo element, a walker on the last child of the shadow root will
> -        // jump to the ::after element, which is not a child of the shadow root.
> -        // TODO: Should rather use an anonymous walker with a new dedicated filter.
> -        return this.getLightDOMWalker(documentWalkerNode, whatToShow, SKIP_TO_SIBLING);
> +      let skipTo = SKIP_TO_SIBLING;
> +
> +      let useLightDOMWalker = isShadowRoot || isShadowHost || isUnslottedLightDOM;
> +      if (useLightDOMWalker) {
> +        // Do not use an anonymous walker for :
> +        // - shadow roots: if the host element has an ::after pseudo element, a walker on

I now can't remember why we use light dom walker for shadow roots -> don't we need to traverse anon content to show the shadow dom? Or at least to show stuff like ::before attached to elements within the shadow root?

Also: is this a bug on the walker or expected behavior: "if the host element has an ::after pseudo element, a walker on the last child of the shadow root will jump to the ::after element, which is not a child of the shadow root."? Ofc we can use a follow up to fix this if needed.

        Attachment #8956784 -
        Flags: review?(bgrinstead) → review+

    
    

    
  
(In reply to Gabriel [:gl] (ΦωΦ) from comment #244)
> (In reply to Brian Grinstead [:bgrins] from comment #242)
> > Comment on attachment 8956789 [details]
> > Bug 1053898 - Display slotted nodes in markup view;
> > 
> > I think Gabe would be a better reviewer for this one
> 
> I thought I already reviewed this.

Oh, I guess Julian needs to update the reviewer in the commit message.

    
    

    
  
Thanks for the additional reviews!

DAMP shows a 3% slowdown on custom.inspector.expandall.manychildren. Will try to see if I can find the cause of the slowdown, but I will most likely move the investigation to a followup.

(In reply to Brian Grinstead [:bgrins] from comment #245)
> Comment on attachment 8956784 [details]
> Bug 1053898 - Update WalkerActor to return light DOM nodes as children of
> host element;
> 
> https://reviewboard.mozilla.org/r/225754/#review237322
> 
> ::: devtools/server/actors/inspector/walker.js:636
> (Diff revision 5)
> > -      if (isShadowRoot) {
> > -        // Do not fetch anonymous children for shadow roots. If the host element has an
> > -        // ::after pseudo element, a walker on the last child of the shadow root will
> > -        // jump to the ::after element, which is not a child of the shadow root.
> > -        // TODO: Should rather use an anonymous walker with a new dedicated filter.
> > -        return this.getLightDOMWalker(documentWalkerNode, whatToShow, SKIP_TO_SIBLING);
> > +      let skipTo = SKIP_TO_SIBLING;
> > +
> > +      let useLightDOMWalker = isShadowRoot || isShadowHost || isUnslottedLightDOM;
> > +      if (useLightDOMWalker) {
> > +        // Do not use an anonymous walker for :
> > +        // - shadow roots: if the host element has an ::after pseudo element, a walker on
> 
> I now can't remember why we use light dom walker for shadow roots -> don't
> we need to traverse anon content to show the shadow dom? Or at least to show
> stuff like ::before attached to elements within the shadow root?
> 
> Also: is this a bug on the walker or expected behavior: "if the host element
> has an ::after pseudo element, a walker on the last child of the shadow root
> will jump to the ::after element, which is not a child of the shadow root."?
> Ofc we can use a follow up to fix this if needed.

This special walker is only used when fetching the direct children of the shadow root. If you call children() on a child of the shadow root, the regular anonymous walker will be used. I don't think we can have before/after elements as pseudo children of a shadow root, but we might have other anonymous elements. I agree we should use another walker/approach here, I will log it as a follow up.

Re: is this a bug of the walker or not: I don't think so. We initialize a walker on a child of the shadow root. If this walker is anonymous, it makes sense to consider the ::after element as a sibling of the last shadow root child. I had another changeset at some point which was reusing the same walker, created from the initial node. I think that if we create a walker on the shadow root, then call firstChild() and repeatedly nextSibling(), it won't jump to the ::after element. But I am not 100% sure, I would prefer to handle that separately in the followup.

I will try to update the comments in the appropriate changesets as much as I can now, and also do the same to rename getLightDOMWalker to getNonAnonymousWalker.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Here is a relatively green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=795703a0d7d6b5621ddfce631bdd7224cb7c32a2 
The remaining failures are also visible on a push with the central changeset this is based upon.