Closed Bug 1449968 Opened 6 years ago Closed 6 years ago

Allow to use inspect element on elements in the shadow dom

Categories

(DevTools :: Inspector, enhancement, P2)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox63 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- verified

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(1 file)

Bug 1449968 - Add support for Inspect Element in Shadow DOM;
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
Follow up to Bug 1053898. 

Using context menu > inspect element on shadow dom node will only select the corresponding host element.
STR

- Open https://shop.polymer-project.org/
- Right click one of the elements on the page

AR: Only<shop-app>, the top host element, gets selected
ER: Select the right element
From the original bug

> - 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.
Product: Firefox → DevTools
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P3 → P2
Comment on attachment 8990279 [details]
Bug 1449968 - Add support for Inspect Element in Shadow DOM;

https://reviewboard.mozilla.org/r/255302/#review263734

I think you mentioned when we chatted that this wasn't ready for review, so clearing it. Feel free to re-flag me if it is.
Attachment #8990279 - Flags: review?(bgrinstead)
Emilio, for this patch we need to build all the CSS selectors that lead us from the root document of a page to a given node. This means we need to traverse frames and shadowRoots. For a given node, I need to know if the selector should be created from its ownerDocument or from it's parent shadow root.

Right now I use the following snippet to detect if a node is under a shadow root:

  const parent = doc.getBindingParent(node);
  const shadowRoot = parent && parent.openOrClosedShadowRoot;
  if (shadowRoot && shadowRoot.contains(node)) {
    return shadowRoot;
  }

Is there a better way to do this?
Flags: needinfo?(emilio)
Comment on attachment 8990279 [details]
Bug 1449968 - Add support for Inspect Element in Shadow DOM;

https://reviewboard.mozilla.org/r/255302/#review263980

::: toolkit/modules/css-selector.js:40
(Diff revision 3)
>  /**
> + * Return the document containing the provided Node. If a node is under a shadowRoot,
> + * node.ownerDocument.contains(node) is false, the document we should use is the shadow
> + * root for this node.
> + */
> +function getElementDocument(node) {

Well, this is not returning a document, but a `DocumentOrShadowRoot`, effectively, right? I think this name is really confusing.

::: toolkit/modules/css-selector.js:48
(Diff revision 3)
> +    return null;
> +  }
> +
> +  // XXX: Using getBindingParent to reach the host of the node's shadow root,
> +  // if applicable. Maybe ask for a direct reference, eg ownerShadowRoot?.
> +  const parent = doc.getBindingParent(node);

So, what you want to get is the containing shadow of an element, right? What you have should just work.

We could expose the containing shadow to chrome I guess, which could be a bit more straight-forward:

  https://searchfox.org/mozilla-central/rev/b0275bc977ad7fda615ef34b822bba938f2b16fd/dom/base/nsIContent.h#487
  
But not sure it's completely worth it? Should be equivalent.

Also, I just realized that we don't optimize Contains for ShadowRoots, which we do for documents for example. We should probably do that, I'll file a bug for that.
Which CSS selectors? I don't see any in your snippet. But what you do there should work.
Flags: needinfo?(emilio)
(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)
> Comment on attachment 8990279 [details]
> Bug 1449968 - Add support for Inspect Element in Shadow DOM;
> 
> Well, this is not returning a document, but a `DocumentOrShadowRoot`,
> effectively, right? I think this name is really confusing.
> 

Yes, that was just a work in progress, my new patches no longer use this.

> ::: toolkit/modules/css-selector.js:48
> (Diff revision 3)
> > +    return null;
> > +  }
> > +
> > +  // XXX: Using getBindingParent to reach the host of the node's shadow root,
> > +  // if applicable. Maybe ask for a direct reference, eg ownerShadowRoot?.
> > +  const parent = doc.getBindingParent(node);
> 
> So, what you want to get is the containing shadow of an element, right? What
> you have should just work.
> 
> We could expose the containing shadow to chrome I guess, which could be a
> bit more straight-forward:
> 
>  
> https://searchfox.org/mozilla-central/rev/
> b0275bc977ad7fda615ef34b822bba938f2b16fd/dom/base/nsIContent.h#487
>   
> But not sure it's completely worth it? Should be equivalent.

If it is easy for you to expose to chrome, that would simplify the code here, and I suppose reduce duplication. It is not completely straightforward to me that getBindingParent should return the host of the containing shadow, so I feel compelled to comment around this. A `node.containingShadow` would be more self explanatory.

For the moment I will move forward with the current implementation since it is equivalent. 

> 
> Also, I just realized that we don't optimize Contains for ShadowRoots, which
> we do for documents for example. We should probably do that, I'll file a bug
> for that.

Thanks!

> Which CSS selectors? I don't see any in your snippet. But what you do there 
> should work.

Yes the snippet is just about "getting the containing shadow", the CSS selector part is handled somewhere else and we are mostly reusing the logic we had before, simply making it aware of shadow roots.
Comment on attachment 8990279 [details]
Bug 1449968 - Add support for Inspect Element in Shadow DOM;

https://reviewboard.mozilla.org/r/255302/#review264064

Looks fine with my very limited understanding and knowledge of this code. But I don't think I should be the one to stamp it :)

::: toolkit/modules/css-selector.js:32
(Diff revision 4)
> +
> +  if (getShadowRoot(node)) {
> +    // If the node is under a shadow root, the shadow host is the "binding
> +    // parent" but we can create a CSS selector between the shadow root and the
> +    // node, so return immediately.
> +    return node;

There's a gotcha which is that selectors don't pierce through the shadow tree. But as long as those  are only used for the breadcrumbs on the inspector or such looks fine.

::: toolkit/modules/css-selector.js:99
(Diff revision 4)
>   * and ele.ownerDocument.querySelectorAll(reply).length === 1
>   */
>  const findCssSelector = function(ele) {
>    ele = getRootBindingParent(ele);
> -  let document = ele.ownerDocument;
> -  if (!document || !document.contains(ele)) {
> +
> +  let parent = getDocumentOrShadowRoot(ele);

I'd call this `containingDocOrShadow` or something of the sort, otherwise the `parent !== ele.parentNode` comparison reads really weird.
Attachment #8990279 - Flags: review?(emilio)
Comment on attachment 8990279 [details]
Bug 1449968 - Add support for Inspect Element in Shadow DOM;

https://reviewboard.mozilla.org/r/255302/#review264142

Thanks for figuring this out

::: toolkit/modules/css-selector.js:54
(Diff revision 4)
> +    return null;
> +  }
> +
> +  const parent = doc.getBindingParent(node);
> +  const shadowRoot = parent && parent.openOrClosedShadowRoot;
> +  if (shadowRoot && shadowRoot.contains(node)) {

In what case is there a shadowRoot but it doesn't contain the node? I see we do that in isShadowAnonymous as well but I can't remember why.
Attachment #8990279 - Flags: review?(bgrinstead) → review+
Blocks: 1449972
Comment on attachment 8990279 [details]
Bug 1449968 - Add support for Inspect Element in Shadow DOM;

https://reviewboard.mozilla.org/r/255302/#review264064

Thanks for having a look, and don't worry I always wait for all reviewers to grant their approval before landing :)

> I'd call this `containingDocOrShadow` or something of the sort, otherwise the `parent !== ele.parentNode` comparison reads really weird.

Fixed, thanks!
Comment on attachment 8990279 [details]
Bug 1449968 - Add support for Inspect Element in Shadow DOM;

https://reviewboard.mozilla.org/r/255302/#review264142

Thanks for the review!

> In what case is there a shadowRoot but it doesn't contain the node? I see we do that in isShadowAnonymous as well but I can't remember why.

I don't really know, I was just cautious because the getBindingParent() / containingShadow was not really clear to me. Dropped it for now, hopefully we can get access to containingShadow later on to remove this code.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3b74899a388
Add support for Inspect Element in Shadow DOM;r=bgrins
Attachment #8990279 - Flags: review?(emilio)
https://hg.mozilla.org/mozilla-central/rev/e3b74899a388
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
I have reproduced this issue using Firefox 61.0a1 (2018.05.02) 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.
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
Flags: qe-verify+
Flags: needinfo?(timea.zsoldos)
Flags: qe-verify+
Flags: needinfo?(timea.zsoldos)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: