Closed Bug 1078374 Opened 10 years ago Closed 6 years ago

Inspector does not show content of <template> tags

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox63 verified)

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

People

(Reporter: bugzilla.mozilla.org, Assigned: jdescottes)

References

(
URL
)

Details

(Keywords: parity-chrome, Whiteboard: [polish-backlog])

Attachments

(4 files)

Bug 1078374 - Show content of template tags in markup view;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1078374 - Return empty string in getPath/Selector methods for nodes outside document;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1078374 - Add mochitest for markup view with template tag;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1078374 - Move helper_shadowdom in markup-view head.js;
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
The content of HTML5 <template> tags is not part of the dom tree and thus doesn't show up in the inspector.

I think it would be useful to show the documentfragment inside the template tag.
Can confirm this issue persists. 

Creating a template html document and opening it up in DevEdition shows template tags but not contents inside:

<html>
<head>
</head>
<body>
    <template>
      <p>I'm coming from a template</p>
    </template>
   
   <div></div>
   
    <script>
      var template = document.querySelector('template');
      var clone = document.importNode(template.content, true);
      document.querySelector('div').appendChild(clone);
    </script>
  </body>
</html>


User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:41.0) Gecko/20100101 Firefox/41.0
Status: UNCONFIRMED → NEW
Ever confirmed: true
See Also: → 1053898
Whiteboard: [parity]
ni? to bgrins and jeff per his request.
Flags: needinfo?(jgriffiths)
Brian - how is this related to the shadow root bug? What blocks this?
Flags: needinfo?(jgriffiths) → needinfo?(bgrinstead)
Priority: -- → P2
Whiteboard: [parity] → [polish-backlog][chrome-parity]
STR: Without e10s on, open data:text/html,<html><head></head><body><template><p>I'm%20coming%20from%20a%20template</p></template><div></div><script>var%20template%20=%20document.querySelector('template');var%20clone%20=%20document.importNode(template.content,%20true);document.querySelector('div').appendChild(clone);</script></body></html>

Open Browser Console and run this code:

  var walker = Cc["@mozilla.org/inspector/deep-tree-walker;1"].createInstance(Ci.inIDeepTreeWalker);
  walker.showAnonymousContent = true;
  walker.showSubDocuments = true;
  walker.showDocumentsAsNodes = true;
  walker.init(content.document, Ci.nsIDOMNodeFilter.SHOW_ALL);
  
  walker.currentNode = content.document.body;
  var template = walker.firstChild(); // <template>
  var p = walker.firstChild(); // null, but expecting this to be <p>I'm coming from...</p>
  console.log(template.innerHTML, p); // logs '<p>I'm coming from a template</p>', null

Gabor, do you have any idea why the inIDeepTreeWalker would be ignoring the children of a <template> element in this case?
Flags: needinfo?(bgrinstead) → needinfo?(gkrizsanits)
(In reply to Brian Grinstead [:bgrins] from comment #4)
> Gabor, do you have any idea why the inIDeepTreeWalker would be ignoring the
> children of a <template> element in this case?

Yeah... So children of a template element are not children in the DOM sense, they define a document fragment and those have to be enumerated differently. Once you instantiate a template the nodes should be visible. I think the solution here would be to add yet another flag, let's say showDocumentFragments and then instead of GetChildren here http://mxr.mozilla.org/mozilla-central/source/layout/inspector/inDeepTreeWalker.cpp#181 we should do something like GetFirstChildOfTemplateOrNode http://mxr.mozilla.org/mozilla-central/source/dom/base/nsNodeUtils.cpp#677
Flags: needinfo?(gkrizsanits)
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-chrome
Whiteboard: [polish-backlog][chrome-parity] → [polish-backlog]
(In reply to Gabor Krizsanits (INACTIVE) from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > Gabor, do you have any idea why the inIDeepTreeWalker would be ignoring the
> > children of a <template> element in this case?
> 
> Yeah... So children of a template element are not children in the DOM sense,
> they define a document fragment and those have to be enumerated differently.
> Once you instantiate a template the nodes should be visible. I think the
> solution here would be to add yet another flag, let's say
> showDocumentFragments and then instead of GetChildren here
> http://mxr.mozilla.org/mozilla-central/source/layout/inspector/
> inDeepTreeWalker.cpp#181 we should do something like
> GetFirstChildOfTemplateOrNode
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsNodeUtils.cpp#677

Emilio: how do you feel about the statement above? Should the inIDeepTreeWalker be modified to return content document-fragment as a child of a <template> element, or should we do something similar to what we did for shadow-roots (ie, detect that we are on a shadow host and build the list of children ourselves in DevTools).
Flags: needinfo?(emilio)
OS: Linux → Unspecified
Hardware: x86_64 → Unspecified
I'm somewhat ambivalent, both would work fine.

Seems easy to poke at template.content if the element is a <template>, and I'm not in love on making inIDeepTreeWalker more of a beast, but arguably <template> is simpler than Shadow DOM so it may be easier to hook into the existing machinery...

Note on the other hand, <template>s contents are owned by a different document, so fully supporting <template> children may be a bit more tricky than what it seems.

I'd say whatever's easier for devtools.
Flags: needinfo?(emilio)
Product: Firefox → DevTools
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> I'm somewhat ambivalent, both would work fine.
> 
> Seems easy to poke at template.content if the element is a <template>, and
> I'm not in love on making inIDeepTreeWalker more of a beast, but arguably
> <template> is simpler than Shadow DOM so it may be easier to hook into the
> existing machinery...
> 

Thanks for the feedback. I think we can start by adding this to DevTools directly.

> Note on the other hand, <template>s contents are owned by a different
> document, so fully supporting <template> children may be a bit more tricky
> than what it seems.
> 

For now, let's just to display the tree correctly in the markupview. The rest of the UI remains empty (ie, "no element selected") but I think it's good enough as a first step. 

talos: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=41594108d62f78035ebbde9dacba8011b252da36&newProject=try&newRevision=f51600a4c5ca9ab23fd32e363c0407c7aa6326aa&framework=1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24d5db682e0d34f951ddc95c8a4af6a050dd2dbd
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8986723 [details]
Bug 1078374 - Return empty string in getPath/Selector methods for nodes outside document;

https://reviewboard.mozilla.org/r/252016/#review258828

What happens with XBL and Shadow DOM anon content is that we copy a selector to the parent node (the host element) via the getRootBindingParent call at https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/toolkit/modules/css-selector.js#53.

I believe this patch will actually change that behavior to return an empty string for those cases instead. I'd prefer either to update the arg to the helper functions or the helper functions themselves to special case content in `<template>` tags so that the selector that gets returned is to `<template>`, or to make `isInDocumentFragment` helper to be more targeted as `isInTemplate` or something so we continue to get selectors to host elements for XBL/SD. Whatever is simpler would be fine with me.

::: devtools/server/actors/inspector/node.js:585
(Diff revision 2)
>      return getXPath(this.rawNode);
>    },
>  
> +  isInDocumentFragment: function() {
> +    const doc = this.rawNode.ownerDocument;
> +    return doc && !doc.contains(this.rawNode);

This isn't just checking if it's in a document fragment. For instance:

var node = document.createElement("div");
node.ownerDocument.contains(node); // false

Ofc we shouldn't really be getting detached nodes in the markup view but who knows (maybe one could have gotten removed after the user opens the context menu or something).

Regardless, as mentioned above it would be nice if we could target the special case here just to content inside of <template> rather than all doc fragments or all detached nodes.
Attachment #8986723 - Flags: review?(bgrinstead)
Comment on attachment 8986825 [details]
Bug 1078374 - Add mochitest for markup view with template tag;

https://reviewboard.mozilla.org/r/252068/#review258836

Looks like the test didn't get added to hg
Attachment #8986825 - Flags: review?(bgrinstead)
Comment on attachment 8986826 [details]
Bug 1078374 - Move helper_shadowdom in markup-view head.js;

https://reviewboard.mozilla.org/r/252070/#review258838

What do you think about deleting helper_assert_tree and moving everything in it into head.js (and/or subscript load it from head.js unconditionally)? I don't feel too strongly about it but it seems like a more general utility function now that we may not want tests to have to opt-in to use.
I actually wonder if there's a good reason to throw in the get css selector / xpath functions on detached nodes:

- https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/toolkit/modules/css-selector.js#56
- https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/devtools/shared/inspector/css-logic.js#381
- https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/devtools/shared/inspector/css-logic.js#430

It seems like if we instead returned an empty string in that case we wouldn't need any actor changes or special casing for template tags.
(In reply to Brian Grinstead [:bgrins] from comment #22)
> I actually wonder if there's a good reason to throw in the get css selector
> / xpath functions on detached nodes:
> 
> -
> https://searchfox.org/mozilla-central/rev/
> 93d2b9860b3d341258c7c5dcd4e278dea544432b/toolkit/modules/css-selector.js#56
> -
> https://searchfox.org/mozilla-central/rev/
> 93d2b9860b3d341258c7c5dcd4e278dea544432b/devtools/shared/inspector/css-logic.
> js#381
> -
> https://searchfox.org/mozilla-central/rev/
> 93d2b9860b3d341258c7c5dcd4e278dea544432b/devtools/shared/inspector/css-logic.
> js#430
> 
> It seems like if we instead returned an empty string in that case we
> wouldn't need any actor changes or special casing for template tags.

This check was added at https://bugzilla.mozilla.org/show_bug.cgi?id=1002280#c18.
Reading your statement I guess there was no strong reason for throwing versus returning, so we might as well do that.
Comment on attachment 8984502 [details]
Bug 1078374 - Show content of template tags in markup view;

https://reviewboard.mozilla.org/r/250346/#review258846

Nice and simple, thanks

::: devtools/server/actors/inspector/node.js:201
(Diff revision 3)
>  
>      const parentNode = this.rawNode.parentNode;
>      return parentNode && !!parentNode.shadowRoot;
>    },
>  
> +  get isTemplateElement() {

Maybe there's a slightly simpler check we could do? Something like:

this.rawNode instanceof this.rawNode.ownerGlobal.HTMLTemplateElement

::: devtools/server/tests/mochitest/test_inspector-template.html:51
(Diff revision 3)
> +
> +    children = await gWalker.children(docFragment);
> +    ok(children.nodes.length === 1, "Found one child under the template element");
> +
> +    const p = children.nodes[0];
> +    ok(p.rawNode.nodeName = "p");

unintended assignment here :). Could also use the `is` helper.
Attachment #8984502 - Flags: review?(bgrinstead) → review+
Comment on attachment 8986723 [details]
Bug 1078374 - Return empty string in getPath/Selector methods for nodes outside document;

https://reviewboard.mozilla.org/r/252016/#review258828

Indeed good call! Thanks for spotting this. I switched the getSelector/Path methods to return an empty string instead of throwing.
Comment on attachment 8984502 [details]
Bug 1078374 - Show content of template tags in markup view;

https://reviewboard.mozilla.org/r/250346/#review258846

Thanks for the review, applied your suggestions.

> Maybe there's a slightly simpler check we could do? Something like:
> 
> this.rawNode instanceof this.rawNode.ownerGlobal.HTMLTemplateElement

Thanks, updated!

> unintended assignment here :). Could also use the `is` helper.

Oops, switched to `is`, also the check should have been against p.nodeName and not p.rawNode.nodeName!
Comment on attachment 8986723 [details]
Bug 1078374 - Return empty string in getPath/Selector methods for nodes outside document;

https://reviewboard.mozilla.org/r/252016/#review259328
Attachment #8986723 - Flags: review?(bgrinstead) → review+
Comment on attachment 8986826 [details]
Bug 1078374 - Move helper_shadowdom in markup-view head.js;

https://reviewboard.mozilla.org/r/252070/#review259330

::: devtools/client/inspector/markup/test/helper_assert_tree.js:11
(Diff revision 4)
>  
>  /* import-globals-from head.js */
>  
>  "use strict";
>  
> -async function checkTreeFromRootSelector(tree, selector, inspector) {
> +/**

Shouldn't this file just be removed? These functions are now in head.js
Attachment #8986826 - Flags: review?(bgrinstead) → review+
Comment on attachment 8986825 [details]
Bug 1078374 - Add mochitest for markup view with template tag;

https://reviewboard.mozilla.org/r/252068/#review259332
Attachment #8986825 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #35)
> Comment on attachment 8986826 [details]
> Bug 1078374 - Move helper_shadowdom in markup-view head.js;
> 
> https://reviewboard.mozilla.org/r/252070/#review259330
> 
> ::: devtools/client/inspector/markup/test/helper_assert_tree.js:11
> (Diff revision 4)
> >  
> >  /* import-globals-from head.js */
> >  
> >  "use strict";
> >  
> > -async function checkTreeFromRootSelector(tree, selector, inspector) {
> > +/**
> 
> Shouldn't this file just be removed? These functions are now in head.js

Good catch, thought I removed it!

Also rebased patches, will try to land now.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/12483f0c5582
Show content of template tags in markup view;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/5852cb4da679
Return empty string in getPath/Selector methods for nodes outside document;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/51b3ee76fb25
Move helper_shadowdom in markup-view head.js;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/168fe4bda1bd
Add mochitest for markup view with template tag;r=bgrins
I have reproduced this issue using Firefox 61.0a1 (2018.05.01) 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: