STRs: - go to https://juliandescottes.github.io/webcomponents-playground/simple/ - open devtools - select element picker - try to pick "after-slot1-container" in the first block => markup view thinks some nodes were hidden (see markup view screenshot)

Made progress here. This bug is a mix of two issues: 1/ creating the form of a shadow host element desynchronizes the nodes maps stored by the inspector front and by the walker actor. This is linked to this other bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1446302. Short summary is that to know the number of children of a shadow host, we call Walker::children(). This will store all the children of the shadow host in the map of "known nodes". But this map should actually only contain nodes that are known AND have been sent to the client. Otherwise the server will skip them during future calls. "This node is already in my map, so the client probably already knows about it, skipping." To address that, either call children() in a special mode that avoids putting the nodes in the actor's map or re-architect the walker actor so that everytime we happen to discover nodes, we make sure to return them as part of the current request. 2/ the walker can fetch children centered around a given node, but this fails in some situations when the node is inside a shadow root. Haven't investigated this part in more details for now.

Can you check if this has any affect on XBL inspection (for better or worse)? For instance, if I go to about:preferences and then try to inspect the "downloads" dropdown (in Files and Applications section) I usually get missing nodes (see also Bug 1360072). Not exactly the same symptom but I wonder if these changes touch the same underlying problems.

I think it definitely improves picking elements on about:preferences! Without the patch: I can't really get the markupview to show incorrect "more" buttons on the current Nightly, but I often pick elements and see no selection in the inspector. Consequently, I often see "empty lines" in the markup view, which is another thing that can happen when a node is desynchronized between the cache of the actor and the cache of the front. With the patch: I was always able to pick the elements I selected in the page. No empty lines either.

(In reply to Julian Descottes [:jdescottes][:julian] from comment #10) > I think it definitely improves picking elements on about:preferences! > > Without the patch: I can't really get the markupview to show incorrect > "more" buttons on the current Nightly, but I often pick elements and see no > selection in the inspector. Consequently, I often see "empty lines" in the > markup view, which is another thing that can happen when a node is > desynchronized between the cache of the actor and the cache of the front. > > With the patch: I was always able to pick the elements I selected in the > page. No empty lines either. Nice! I didn't have much luck tracking this down in Bug 1360072 - maybe a fix here can close that one out as well.

Comment on attachment 8987115 [details] Bug 1465873 - part1: Implement walker::countChildren() to count children without updating refMap; https://reviewboard.mozilla.org/r/252344/#review260068 ::: devtools/server/actors/inspector/walker.js:617 (Diff revision 2) > + hasLast, > + nodes: nodes.map(n => this._ref(n)) > + }; > + }, > + > + _getChildren: function(node, options = {}) { This function could use a comment indicating that `nodes` is an array of actual DOM nodes and not of actors

Comment on attachment 8988173 [details] Bug 1465873 - part4: Update document-walker::parentNode() to return shadow roots; https://reviewboard.mozilla.org/r/253380/#review260070

Comment on attachment 8988174 [details] Bug 1465873 - part3: Move generic node utils from Node actor to layout/utils helper; https://reviewboard.mozilla.org/r/253416/#review260076 ::: devtools/server/actors/inspector/walker.js:436 (Diff revision 1) > rawParentNode: function(node) { > + const parentNode = node.rawNode.parentNode; > + if (parentNode && > + parentNode.host && > + parentNode.nodeType === Node.DOCUMENT_FRAGMENT_NODE) { > + // The walker currently does not return shadowRoot elements as parentNodes. This feels like something that should live in `DocumentWalker.parentNode()`. Is there a reason why we wouldn't always want the parent node to return the shadow root element?

Comment on attachment 8988175 [details] Bug 1465873 - part6: Rename isDirectShadowHostChild to isShadowHostChild; https://reviewboard.mozilla.org/r/253418/#review260092

Comment on attachment 8988174 [details] Bug 1465873 - part3: Move generic node utils from Node actor to layout/utils helper; https://reviewboard.mozilla.org/r/253416/#review260094 ::: devtools/server/actors/inspector/walker.js:436 (Diff revision 1) > rawParentNode: function(node) { > + const parentNode = node.rawNode.parentNode; > + if (parentNode && > + parentNode.host && > + parentNode.nodeType === Node.DOCUMENT_FRAGMENT_NODE) { > + // The walker currently does not return shadowRoot elements as parentNodes. Also, I don't love that we are essentially duplicating logic from the NodeActor `isShadowRoot` / `isBeforePseudoElement` / `isAfterPseudoElement` functions in this patch series. It happens that these are pretty trivial checks so I'm OK to make the change to fix this bug. But we could also think about flipping this around so these types of checks live in devtools/shared/layout/utils like `is*Anonymous` functions that take in a raw node, and then either: 1) Remove the getters on the NodeActor and update any server-side callers with utility function calls 2) Replace the implementation of these getters to call along to the utility function. There's a whole set of `is*` getters that could be switched to this pattern, rather than doing it one by one as we want to use them for rawNodes: https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/devtools/server/actors/inspector/node.js#120-128 I'm not sure if we should make this change, so looking for your opinion on it.

Comment on attachment 8988176 [details] Bug 1465873 - part5: Add mochitest for hovering shadowdom with element-picker; https://reviewboard.mozilla.org/r/253420/#review260112

I'm going to tentatively mark parts 2 and 3 as r+ as I'm going to be away the rest of this week and it seems fine to do some of this as follow-ups, but please see Comment 16 before landing.

(In reply to Brian Grinstead [:bgrins] from comment #18) > parts 2 and 3 Meant to say: parts 2 and 4

Comment on attachment 8988172 [details] Bug 1465873 - part2: Update walker::children() to avoid updating refMap to detect pseudo elements; https://reviewboard.mozilla.org/r/253378/#review260138

Comment on attachment 8988174 [details] Bug 1465873 - part3: Move generic node utils from Node actor to layout/utils helper; https://reviewboard.mozilla.org/r/253416/#review260142

(In reply to Brian Grinstead [:bgrins] from comment #16) > Comment on attachment 8988174 [details] > Bug 1465873 - part4: Update walker::rawParentNode() to return shadowRoot > parents; > > https://reviewboard.mozilla.org/r/253416/#review260094 > > ::: devtools/server/actors/inspector/walker.js:436 > (Diff revision 1) > > rawParentNode: function(node) { > > + const parentNode = node.rawNode.parentNode; > > + if (parentNode && > > + parentNode.host && > > + parentNode.nodeType === Node.DOCUMENT_FRAGMENT_NODE) { > > + // The walker currently does not return shadowRoot elements as parentNodes. > > Also, I don't love that we are essentially duplicating logic from the > NodeActor `isShadowRoot` / `isBeforePseudoElement` / `isAfterPseudoElement` > functions in this patch series. > I agree! I did this as a shortcut, but I'll try to clean this up in this patch and move as much as I need to layout/utils. As a follow up I think it's worth checking the platform walker could return the shadowRoot when we fetch the parentNode of a node that _has_ the shadowRoot as its parentNode. After all we set all { showAnonymousContent, showSubDocuments, showDocumentsAsNodes } flags to true in this case for our walker, so if it's not too complex to implement I think it would make sense for the deep tree walker to return the shadow root in such a case.

(In reply to Brian Grinstead [:bgrins] from comment #14) > Comment on attachment 8988174 [details] > Bug 1465873 - part5: Move generic node utils from Node actor to layout/utils > helper; > > https://reviewboard.mozilla.org/r/253416/#review260076 > > ::: devtools/server/actors/inspector/walker.js:436 > (Diff revision 1) > > rawParentNode: function(node) { > > + const parentNode = node.rawNode.parentNode; > > + if (parentNode && > > + parentNode.host && > > + parentNode.nodeType === Node.DOCUMENT_FRAGMENT_NODE) { > > + // The walker currently does not return shadowRoot elements as parentNodes. > > This feels like something that should live in `DocumentWalker.parentNode()`. > Is there a reason why we wouldn't always want the parent node to return the > shadow root element? Done! This actually makes the whole thing much simpler as we no longer need to extract rawParentNode() from parentNode(), we don't have to change the ensurePathToRoot logic either. Thanks for the suggestion!

Regarding part7, I added this new parentOrHost() method, which I believe we should probably use in more and more spots. But if that's ok I'd like to leave that to Bug 1472142 because the patch queue here starts to grow a bit big.

Comment on attachment 8988173 [details] Bug 1465873 - part4: Update document-walker::parentNode() to return shadow roots; https://reviewboard.mozilla.org/r/253380/#review260810 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/document-walker.js:76 (Diff revision 3) > set currentNode(val) { > this.walker.currentNode = val; > }, > > parentNode: function() { > + if (isShadowRoot(this.currentNode)) { Error: 'isshadowroot' is not defined. [eslint: no-undef]

Comment on attachment 8988174 [details] Bug 1465873 - part3: Move generic node utils from Node actor to layout/utils helper; https://reviewboard.mozilla.org/r/253416/#review261070 ::: devtools/server/actors/inspector/node.js:19 (Diff revision 3) > > loader.lazyRequireGetter(this, "getCssPath", "devtools/shared/inspector/css-logic", true); > loader.lazyRequireGetter(this, "getXPath", "devtools/shared/inspector/css-logic", true); > loader.lazyRequireGetter(this, "findCssSelector", "devtools/shared/inspector/css-logic", true); > > +loader.lazyRequireGetter(this, "isAfterPseudoElement", "devtools/shared/layout/utils", true); This makes me think we should update lazyRequireGetter to optionally take array of names as a second parameter (like XPCOMUtils.defineLazyScriptGetter). That would be follow up material, but what do you think?

Are you planning to fold some of these together before landing? Comment 37 seems to highlight that one of the utility functions in part 3 isn't introduced until part 5.

Comment on attachment 8988455 [details] Bug 1465873 - part6: Allow selection and breadcrumbs to walk from shadowRoot to host element; https://reviewboard.mozilla.org/r/253730/#review261072 ::: commit-message-947c8:3 (Diff revision 2) > +Bug 1465873 - part7: Allow selection and breadcrumbs to walk from shadowRoot to host element;r=bgrins > + > +By returning the shador root as the parentNode of some elements, the breadcrumbs could no longer Typo: "shadow" ::: devtools/shared/fronts/node.js:171 (Diff revision 2) > const parentNodeFront = ctx.marshallPool().ensureParentFront(form.parent); > this.reparent(parentNodeFront); > } > > + if (form.host) { > + this.host = ctx.marshallPool().get(form.host); I haven't looked into this in detail so don't know the answer to the question: do we need to call ensureParentFront on form.host? Why or why not?

Comment on attachment 8988175 [details] Bug 1465873 - part6: Rename isDirectShadowHostChild to isShadowHostChild; https://reviewboard.mozilla.org/r/253418/#review261084 Renaming is generally fine with me - but I'd like to know the reasoning behind changing it (since we discussed the original name at https://bugzilla.mozilla.org/show_bug.cgi?id=1053898#c205, https://bugzilla.mozilla.org/show_bug.cgi?id=1053898#c207, and https://bugzilla.mozilla.org/show_bug.cgi?id=1053898#c218).

Comment on attachment 8988173 [details] Bug 1465873 - part4: Update document-walker::parentNode() to return shadow roots; https://reviewboard.mozilla.org/r/253380/#review261086 I think this is fine, but it's hard to follow - I think this is using the new isShadowRoot helper for one of the checks but not for the parentNode check. So I'd like to take another look at this before landing. ::: devtools/server/actors/inspector/document-walker.js:76 (Diff revision 3) > set currentNode(val) { > this.walker.currentNode = val; > }, > > parentNode: function() { > + if (isShadowRoot(this.currentNode)) { As I mentioned in Comment 39 - I think this part needs to be folded together or added after part 5.

(In reply to Brian Grinstead [:bgrins] from comment #38) > Comment on attachment 8988174 [details] > Bug 1465873 - part5: Move generic node utils from Node actor to layout/utils > helper; > > https://reviewboard.mozilla.org/r/253416/#review261070 > > ::: devtools/server/actors/inspector/node.js:19 > (Diff revision 3) > > > > loader.lazyRequireGetter(this, "getCssPath", "devtools/shared/inspector/css-logic", true); > > loader.lazyRequireGetter(this, "getXPath", "devtools/shared/inspector/css-logic", true); > > loader.lazyRequireGetter(this, "findCssSelector", "devtools/shared/inspector/css-logic", true); > > > > +loader.lazyRequireGetter(this, "isAfterPseudoElement", "devtools/shared/layout/utils", true); > > This makes me think we should update lazyRequireGetter to optionally take > array of names as a second parameter (like > XPCOMUtils.defineLazyScriptGetter). > > That would be follow up material, but what do you think? I agree! Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1472821. (In reply to Brian Grinstead [:bgrins] from comment #40) > Comment on attachment 8988455 [details] > Bug 1465873 - part7: Allow selection and breadcrumbs to walk from shadowRoot > to host element; > > https://reviewboard.mozilla.org/r/253730/#review261072 > > ::: commit-message-947c8:3 > (Diff revision 2) > > +Bug 1465873 - part7: Allow selection and breadcrumbs to walk from shadowRoot to host element;r=bgrins > > + > > +By returning the shador root as the parentNode of some elements, the breadcrumbs could no longer > > Typo: "shadow" > > ::: devtools/shared/fronts/node.js:171 > (Diff revision 2) > > const parentNodeFront = ctx.marshallPool().ensureParentFront(form.parent); > > this.reparent(parentNodeFront); > > } > > > > + if (form.host) { > > + this.host = ctx.marshallPool().get(form.host); > > I haven't looked into this in detail so don't know the answer to the > question: do we need to call ensureParentFront on form.host? Why or why not? It makes sense to use ensureParentFront (maybe we should rename it to a more generic ensureDomNodeFront in this case). (In reply to Brian Grinstead [:bgrins] from comment #41) > Comment on attachment 8988175 [details] > Bug 1465873 - part6: Rename isDirectShadowHostChild to isShadowHostChild; > > https://reviewboard.mozilla.org/r/253418/#review261084 > > Renaming is generally fine with me - but I'd like to know the reasoning > behind changing it (since we discussed the original name at > https://bugzilla.mozilla.org/show_bug.cgi?id=1053898#c205, > https://bugzilla.mozilla.org/show_bug.cgi?id=1053898#c207, and > https://bugzilla.mozilla.org/show_bug.cgi?id=1053898#c218). I actually forgot we discussed so much about this one! While using the method once again here, my thinking was that "direct child" was a bit redundant, since a child is by definition always "direct" - otherwise it's a descendant. I guess here we used it to emphasize the difference with children in the shadowDOM. I'll drop the commit for now.

Comment on attachment 8988173 [details] Bug 1465873 - part4: Update document-walker::parentNode() to return shadow roots; https://reviewboard.mozilla.org/r/253380/#review261164 ::: devtools/server/actors/inspector/document-walker.js:83 (Diff revision 4) > + this.currentNode = this.currentNode.host; > + return this.currentNode; > + } > + > + const parentNode = this.currentNode.parentNode; > + // deep-tree-walker currently does not return shadowRoot elements as parentNodes. This should use isShadowRoot(parentNode), right?

Comment on attachment 8988455 [details] Bug 1465873 - part6: Allow selection and breadcrumbs to walk from shadowRoot to host element; https://reviewboard.mozilla.org/r/253730/#review261168 ::: devtools/shared/fronts/inspector.js:89 (Diff revision 4) > - * by the client in either a previous or the current request. > + * be seen by the client in either a previous or the current request. > * So if we've already seen this parent return it, otherwise create > * a bare-bones stand-in node. The stand-in node will be updated > * with a real form by the end of the deserialization. > */ > - ensureParentFront: function(id) { > + ensureDomNodeFront: function(id) { Nit: DOMNode (capitalization)

Comment on attachment 8988173 [details] Bug 1465873 - part4: Update document-walker::parentNode() to return shadow roots; https://reviewboard.mozilla.org/r/253380/#review261654 Marking r+ but please change to use the helper function, unless if there's some reason not to

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 90d35229eb6af85b970d9119e980a7fe0ee00126 -d 6abc5dc6baaf: rebasing 471237:90d35229eb6a "Bug 1465873 - part1: Implement walker::countChildren() to count children without updating refMap;r=bgrins" merging devtools/server/actors/inspector/walker.js rebasing 471238:b6071adaebb2 "Bug 1465873 - part2: Update walker::children() to avoid updating refMap to detect pseudo elements;r=bgrins" merging devtools/server/actors/inspector/walker.js rebasing 471239:9f28760a2600 "Bug 1465873 - part3: Move generic node utils from Node actor to layout/utils helper;r=bgrins" merging devtools/server/actors/inspector/walker.js rebasing 471240:e2b34dff5289 "Bug 1465873 - part4: Update document-walker::parentNode() to return shadow roots;r=bgrins" merging devtools/server/actors/inspector/walker.js rebasing 471241:34144b210184 "Bug 1465873 - part5: Add mochitest for hovering shadowdom with element-picker;r=bgrins" merging devtools/client/inspector/markup/test/browser.ini warning: conflicts while merging devtools/client/inspector/markup/test/browser.ini! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/91533814fc20 part1: Implement walker::countChildren() to count children without updating refMap;r=bgrins https://hg.mozilla.org/integration/autoland/rev/69e0f9c131f2 part2: Update walker::children() to avoid updating refMap to detect pseudo elements;r=bgrins https://hg.mozilla.org/integration/autoland/rev/86903e1a3a89 part3: Move generic node utils from Node actor to layout/utils helper;r=bgrins https://hg.mozilla.org/integration/autoland/rev/dbbfca659b32 part4: Update document-walker::parentNode() to return shadow roots;r=bgrins https://hg.mozilla.org/integration/autoland/rev/8de6309544e5 part5: Add mochitest for hovering shadowdom with element-picker;r=bgrins https://hg.mozilla.org/integration/autoland/rev/477a745ebddc part6: Allow selection and breadcrumbs to walk from shadowRoot to host element;r=bgrins

https://hg.mozilla.org/mozilla-central/rev/91533814fc20 https://hg.mozilla.org/mozilla-central/rev/69e0f9c131f2 https://hg.mozilla.org/mozilla-central/rev/86903e1a3a89 https://hg.mozilla.org/mozilla-central/rev/dbbfca659b32 https://hg.mozilla.org/mozilla-central/rev/8de6309544e5 https://hg.mozilla.org/mozilla-central/rev/477a745ebddc

I have reproduced this issue using Firefox 62.0a1 (2018.05.31) on Windows 10 x64. I can confirm this issue is fixed, I verified using Firefox 63.0b3 on Ubuntu 16.04 x64, Windows 10 x64 and Mac OS X 10.13.