Brian Grinstead [:bgrins] Reporter
	Description • 10 years ago

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.

	Brian Grinstead [:bgrins] Reporter
	Comment 1 • 10 years ago

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?

	Angelina Fabbro (:angelina)
	Comment 2 • 10 years ago

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.

	Gabor Krizsanits (INACTIVE)
	Comment 3 • 10 years ago

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.

	Brian Grinstead [:bgrins] Reporter
	Comment 4 • 10 years ago

(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>

	Gabor Krizsanits (INACTIVE)
	Comment 5 • 10 years ago

(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)

	Brian Grinstead [:bgrins] Reporter
	Comment 6 • 10 years ago

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.

	Gabor Krizsanits (INACTIVE)
	Comment 7 • 10 years ago

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...

	Gabor Krizsanits (INACTIVE)
	Comment 8 • 10 years ago

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.

	Brian Grinstead [:bgrins] Reporter
	Comment 9 • 10 years ago

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.

	Martijn Wargers (dead)
	Comment 10 • 9 years ago

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?

	Alexandre Poirot [:ochameau]
	Comment 11 • 9 years ago

(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...

	Patrick Brosset <:pbro>
	Comment 12 • 9 years ago

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.

	Jeff Griffiths (:canuckistani) (:⚡︎)
	Comment 14 • 9 years ago

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

https://twitter.com/anatudor/status/628684488470605825

	Gabriel Luong [:gl] (ΦωΦ)
	Comment 15 • 9 years ago

(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.

	Brian Grinstead [:bgrins] Reporter
	Comment 16 • 9 years ago

(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

	Jeff Griffiths (:canuckistani) (:⚡︎)
	Comment 17 • 9 years ago

(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.

	Jeff Griffiths (:canuckistani) (:⚡︎)
	Comment 18 • 9 years ago

From the looks of that pref, bug 811542 should block this I guess.

	Martijn Wargers (dead)
	Comment 19 • 9 years ago

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

	Nicolas Chevobbe [:nchevobbe]
	Comment 20 • 8 years ago

Inspector Bugs Triage - Filter on CLIMBING SHOES

	Patrick Brosset <:pbro>
	Comment 21 • 7 years ago

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.

	Emilio Cobos Álvarez (:emilio)
	Comment 22 • 7 years ago

(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.

	Jessica Jong [:jessica] (inactive)
	Comment 23 • 7 years ago

(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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 24 • 7 years ago

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.

	Jessica Jong [:jessica] (inactive)
	Comment 25 • 7 years ago

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.

	Jessica Jong [:jessica] (inactive)
	Comment 26 • 7 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 27 • 7 years ago

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.

	Patrick Brosset <:pbro>
	Comment 28 • 7 years ago

(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.

	Jessica Jong [:jessica] (inactive)
	Comment 29 • 7 years ago

(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.

	Brian Grinstead [:bgrins] Reporter
	Comment 30 • 6 years ago

Going to spend some time reviving this patch with the current spec

	Julian Descottes [:jdescottes] Assignee
	Comment 42 • 6 years ago

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).

	Patrick Brosset <:pbro>
	Comment 43 • 6 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 44 • 6 years ago

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?

	Code Review Bot [:reviewbot]
	Comment 55 • 6 years ago
mozreview-review

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]

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 56 • 6 years ago

(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

	Julian Descottes [:jdescottes] Assignee
	Comment 63 • 6 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 64 • 6 years ago

(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.

	Julian Descottes [:jdescottes] Assignee
	Comment 65 • 6 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 66 • 6 years ago

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.

	Brian Grinstead [:bgrins] Reporter
	Comment 67 • 6 years ago

(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.

	Brian Grinstead [:bgrins] Reporter
	Comment 68 • 6 years ago
mozreview-review

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)`

	Brian Grinstead [:bgrins] Reporter
	Comment 69 • 6 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 138 • 6 years ago

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 139 • 6 years ago

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

	Emilio Cobos Álvarez (:emilio)
	Comment 140 • 6 years ago

(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.

	Patrick Brosset <:pbro>
	Comment 141 • 6 years ago
mozreview-review

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

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

	Patrick Brosset <:pbro>
	Comment 142 • 6 years ago
mozreview-review

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

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

	Patrick Brosset <:pbro>
	Comment 143 • 6 years ago
mozreview-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?

	Julian Descottes [:jdescottes] Assignee
	Comment 144 • 6 years ago
mozreview-review-reply

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.

	Julian Descottes [:jdescottes] Assignee
	Comment 145 • 6 years ago

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.

	Patrick Brosset <:pbro>
	Comment 146 • 6 years ago

(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).

	Julian Descottes [:jdescottes] Assignee
	Comment 147 • 6 years ago

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?

	Patrick Brosset <:pbro>
	Comment 148 • 6 years ago

(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.

	Gabriel Luong [:gl] (ΦωΦ)
	Comment 149 • 6 years ago
mozreview-review

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.