820926 - [markup view] don't close self-closing tags (br, img, …) if the page is an HTML page (not X(HT)ML)

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P2)

Description

[rokeefe1]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20121119183901

Steps to reproduce:

I used the DOM inspector to look at some of my HTML.


Actual results:

I noticed that the DOM window displayed a break tag like

<br></br>


Expected results:

Break tags are self closing according to the W3C spec (http://www.w3.org/TR/html401/struct/text.html#edef-BR).  The tag should have displayed as <br/>

Comment 1

[Ioana (away)]

Bug reproducible on the 12/17 Nightly. 

Although I clearly had "<br/>" in my code, the Inspector (Ctrl+Shift+I) displayed it as "<br ></br>".

Comment 2

[Rob Campbell [:rc] (:robcee)]

We're not really showing the tags "as you wrote them". We show markup as it is stored internally.

Firebug shows them as <br>.

Comment 3

[Paul Rouget [:paul]]

Self closing tags should not include the closing tag if the page is an HTML page. We should show the closing tag only in XHTML mode.

Comment 7

[John Karahalis [:openjck]]

I can confirm. The Inspector still shows closing tags for void elements like <br>, <img>, and <col>. Chrome correctly shows only the opening tag (attachment 8728589 [details]).

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Attachment: self-closing.patch

This patch fix the bug but I'm unsure of how I handle this.
I think having an easy way to get if the inspected document is HTML is good ( and could be used for getting the correct casing in the view, which is for now limited to svg - https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/markup/markup.js#2702 ).
But does adding the `isHTMLDocument` to the WalkerFront is okay, or should I do this in another way ?
I prefer asking for feedback before going further and adding the proper tests for this. 

Thanks

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8742976 [details] [diff] [review]
self-closing.patch

Review of attachment 8742976 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this Nicolas!

I agree we should be able to get the information about the document's type from the actor.
However I think this should be forwarded on every nodeFront and not globally for the walker.

If a page displays another page in an iframe we could have a mix of html and xhtml documents. Would be nice if the markup view would render the markup of each iframe properly. So we need to know for any node if it's inside an HTML or XHTML document.

Maybe another way to get the type of the document: using the contentType. 
It's gecko only, but I think this is fine. Here XHTML documents should have : "application/xhtml+xml", and HTML documents should have "text/html".
It looks like this information is already used internally: https://dxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp#544

Then we can add the following to the NodeActor form : 
documentContentType: this.rawNode.ownerDocument &&
        this.rawNode.ownerDocument.contentType,

::: devtools/client/inspector/markup/markup.js
@@ +2668,5 @@
>    let tagName = this.getTagName(this.node);
>    this.tag.textContent = tagName;
> +
> +  let isSelfClosingElement = (
> +    this.doc.createElement(tagName).outerHTML.includes("><") === false);

I would ask a second opinion here. I think a static list containing tag names of self-closing elements could also work nicely. I get the feeling self closing elements are not changing that frequently.

FWIW, I checked Chrome devtools current implementation and they seem to rely on a static list too. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js&q=WebInspector.ElementsTreeElement.ForbiddenClosingTagElements&sq=package:chromium&type=cs&l=59

@@ +2670,5 @@
> +
> +  let isSelfClosingElement = (
> +    this.doc.createElement(tagName).outerHTML.includes("><") === false);
> +  if (this.markup.walker.isHTMLDocument && isSelfClosingElement) {
> +    this.closeTag.parentElement.style.display = "none";

Maybe make sure that the element cannot be expanded either. 

This might be a bug, but Firefox seems to insert a ::before element inside an <img> tag that has no src. Really an edge case though.

Comment 10

[Julian Descottes [:jdescottes]]

Forwarding the ni? to Tom who might know more.

Tom, any idea what would be the best way to find out if a document is HTML or XHTML? 
I was suggesting to use document.contentType and compare this to "application/xhtml+xml" and "text/html", but maybe there's a better way?

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

For the self-closing tag part, I think we could use the list of void elements ( https://www.w3.org/TR/html5/syntax.html#void-elements ) , which is defined in Firefox :  https://dxr.mozilla.org/mozilla-central/source/dom/base/FragmentOrElement.cpp#2094 .
I'll see how this list can be retrieved in the devtools

Comment 12

[Tom Tromey :tromey]

(In reply to Julian Descottes [:jdescottes] from comment #10)
> Forwarding the ni? to Tom who might know more.
> 
> Tom, any idea what would be the best way to find out if a document is HTML
> or XHTML? 
> I was suggesting to use document.contentType and compare this to
> "application/xhtml+xml" and "text/html", but maybe there's a better way?

I'm afraid I don't know, sorry :(

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Julian Descottes [:jdescottes] from comment #9)
> This might be a bug, but Firefox seems to insert a ::before element inside
> an <img> tag that has no src. Really an edge case though.

I don't know if it's a bug or not, but people are using it to style broken `<img>` : http://bitsofco.de/styling-broken-images/  .
So, should we still show the closing tag if the node has pseudo-elements ( and thus, can be expanded ) ?

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Attachment: markup.png

So here's what we can do. 
If we are in an HTML document, don't show closing tag on void element when the tag is collapsed.
If the tag is expanded ( a void element can be expanded if it has pseudo-elements attached to it ), show the closing tag.
If we are in an XHTML document ( or SVG, or XML, ... ) always show the closing tag.

Comment 15

[Julian Descottes [:jdescottes]]

(In reply to Nicolas Chevobbe from comment #14)
> Created attachment 8744016 [details]
> markup.png
> 
> So here's what we can do. 
> If we are in an HTML document, don't show closing tag on void element when
> the tag is collapsed.
> If the tag is expanded ( a void element can be expanded if it has
> pseudo-elements attached to it ), show the closing tag.
> If we are in an XHTML document ( or SVG, or XML, ... ) always show the
> closing tag.

Seems reasonable, looks good to me.

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Attachment: void elements

I followed jdescottes advices and it works quite well.
I pulled the void element list from
https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/html/resources/common.js#29 .
https://bugzilla.mozilla.org/attachment.cgi?id=8744016 is an example of what it looks like.

Julian, both Patrick and Brian don't accept feedback request this week, so I'm asking to you :) Eventually, I'll ask one of them to review the full patch ( with tests ), if you don't feel you can do it. 
Thanks !

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8744019 [details] [diff] [review]
void elements

Review of attachment 8744019 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

I see you opted for a static list of tag names for now. Do you still want to investigate how to handle this more dynamically? 
I think it's fine as it is, just want to make sure.

As we discussed I prefer :pbro or :bgrins to handle the final review, but I think you can move forward and have a look at tests now.

Thanks!

::: devtools/client/themes/markup.css
@@ +192,5 @@
>  .child.collapsed .close {
>    display: inline;
>  }
>  
> +.child.collapsed > .tag-line .void-element .close {

Maybe add a comment for this rule?

::: devtools/server/actors/inspector.js
@@ +276,5 @@
>        isNativeAnonymous: isNativeAnonymous(this.rawNode),
>        isXBLAnonymous: isXBLAnonymous(this.rawNode),
>        isShadowAnonymous: isShadowAnonymous(this.rawNode),
>        pseudoClassLocks: this.writePseudoClassLocks(),
> +      documentContentType: (this.rawNode.ownerDocument ? this.rawNode.ownerDocument.contentType : null),

nits: 
- this property could sit next to namespaceURI since they are related
- no need for the wrapping parens here
- wrap at 80 chars

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Attachment: MozReview Request: Bug 820926 - Hide void-element's closing tag if the page is an HTML page. r=pbro

Though, if the element has pseudo-elements, and thus can be expanded, show
the closing tag when the void element is expanded.

Review commit: https://reviewboard.mozilla.org/r/48631/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48631/

Comment 19

[Patrick Brosset <:pbro>]

Comment on attachment 8744846 [details]
MozReview Request: Bug 820926 - Hide void-element's closing tag if the page is an HTML page. r=pbro

https://reviewboard.mozilla.org/r/48631/#review45513

::: devtools/client/inspector/markup/markup.js:26
(Diff revision 1)
>  const DRAG_DROP_MIN_INITIAL_DISTANCE = 10;
>  const AUTOCOMPLETE_POPUP_PANEL_ID = "markupview_autoCompletePopup";
>  const ATTR_COLLAPSE_ENABLED_PREF = "devtools.markup.collapseAttributes";
>  const ATTR_COLLAPSE_LENGTH_PREF = "devtools.markup.collapseAttributeLength";
>  
> +const HTML_VOID_ELEMENTS = [ "area", "base", "br", "col", "command", "embed",

Please add a comment above this list to explain that this is supposed to only contain tags that are explicitely not allowed to be closed in HTML documents.

::: devtools/client/inspector/markup/markup.js:2687
(Diff revision 1)
>    let tagName = this.getTagName(this.node);
>    this.tag.textContent = tagName;
>    this.closeTag.textContent = tagName;
>  
> +  let isVoidElement = HTML_VOID_ELEMENTS.includes(tagName);
> +  let isHTMLDocument = node.documentContentType === "text/html";

Old debuggees are not going to have this property, so it's going to return undefined, which is fine, this is just going to work with new debuggees.

::: devtools/client/inspector/markup/test/browser_markup_void_elements.js:1
(Diff revision 1)
> +/* vim: set ts=2 et sw=2 tw=80: */

Please split this test into 2 tests.
It works fine this way, but will be more prone to timeouts under slow test environments.
Also, the toolbox in the first tab isn't being closed when the test ends, only the tab is.
That's fine, but I think checking that the toolbox is destroyed when the tab is belongs in other tests (which we already have), and I'd rather not do this here.

::: devtools/client/inspector/markup/test/browser_markup_void_elements.js:12
(Diff revision 1)
> +// Test void element display in the markupview.
> +const HTML_TEST_URL = URL_ROOT + "doc_markup_void_elements.html";
> +const XHTML_TEST_URL = URL_ROOT + "doc_markup_void_elements.xhtml";
> +
> +add_task(function* () {
> +  let {inspector} = yield openInspectorForURL(HTML_TEST_URL);

You could also init a variable called `win` here to avoid having to get it several times below:

```
let {win} = inspector.markup;
```

::: devtools/client/inspector/markup/test/browser_markup_void_elements.js:38
(Diff revision 1)
> +  let onChildren = waitForChildrenUpdated(inspector);
> +  let onUpdated = inspector.once("inspector-updated");
> +  EventUtils.synthesizeMouseAtCenter(container.expander, {},
> +    inspector.markup.doc.defaultView);
> +  yield onChildren;
> +  yield onUpdated;

Here you have to wait for inspector-updated because you simulate a click on the expander element, and so the node ends up being selected.
You can avoid this and just expand the node only without selecting. Looking at other tests, it looks like they always do:

```
yield inspector.markup.expandNode(nodeFront);
yield waitForMultipleChildrenUpdates(inspector);
```

::: devtools/server/actors/inspector.js:264
(Diff revision 1)
>        baseURI: this.rawNode.baseURI,
>        parent: parentNode ? parentNode.actorID : undefined,
>        nodeType: this.rawNode.nodeType,
>        namespaceURI: this.rawNode.namespaceURI,
> +      documentContentType: this.rawNode.ownerDocument ?
> +        this.rawNode.ownerDocument.contentType : null,

https://developer.mozilla.org/fr/docs/Web/API/Document/contentType says this is non-standard and gecko only, but I found it to work in Chrome too. 

I'm not sure what to think of this.
We need to keep in mind that one of our goals is to be able to debug other things than gecko, so potentially the toolbox could be connecting to other browser engines (through valence for instance). In this situation, we'd have to be able to provide this information somehow. Looking at Chrome's debugger protocol, it seems like they are not exposing this information.
So we'd have to find it another way.

I'm thinking that there may be a way to avoid introducing this new property by using the walker to find out if a DOCTYPE node exists, and use it to find the mime type, maybe with a combination of using the node's namespaceURI. I'm unsure about all the different edge cases here, and document.contentType sounds way simpler.

I don't know if we should worry about this now or not.

Comment 20

[Patrick Brosset <:pbro>]

Pinging Brian about my last comment in comment 19.
Brian, do you think this is a problem worth investigating now?

Comment 21

[Brian Grinstead [:bgrins]]

I think we should do the simplest thing that renders HTML documents correctly for gecko, and not worry about using the non-standard property in this case.  As long we don't throw on falsy values of this property from the frontend, then worst case for something like servo / valence is that we'll be treating all tags in the UI as we do currently.

Related to document.contentType: I did a bit of searching and it does look like DOMInspector is also using it for detection in a few places: http://mxr.mozilla.org/comm-central/search?find=%2Fmozilla%2Fextensions%2Finspector%2F&string=contenttype

Comment 22

[Tim Nguyen :ntim]

(In reply to Nicolas Chevobbe from comment #14)
> Created attachment 8744016 [details]
> markup.png
> 
> So here's what we can do. 
> If we are in an HTML document, don't show closing tag on void element when
> the tag is collapsed.
> If the tag is expanded ( a void element can be expanded if it has
> pseudo-elements attached to it ), show the closing tag.
> If we are in an XHTML document ( or SVG, or XML, ... ) always show the
> closing tag.

Note that XHTML supports this form as well: <img />. I think it should just be easier to show that form all the time, since HTML supports it as well. That form is also less confusing, since it emphatises the fact that void elements have the same behaviour (adding child nodes does nothing, pseudo elements don't work, ...) for HTML and XHTML.

Comment 23

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim from comment #22)
> That form is also less confusing, since it emphatises the fact that void
> elements have the same behaviour (adding child nodes does nothing, pseudo
> elements don't work, ...) for HTML and XHTML.

I was wrong about this part :)
While those 2 render the same visual result:
data:application/xhtml+xml,<html xmlns="http://www.w3.org/1999/xhtml"><img>foo</img><style>img::after {content: 'foo'}</style></html>
data:text/html,<html><img>foo</img><style>img::after {content: 'foo'}</style></html>

XHTML computes to: <img>foo</img>
while HTML computes to: <img /> foo.

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Tim Nguyen :ntim from comment #22)
> (In reply to Nicolas Chevobbe from comment #14)
> > Created attachment 8744016 [details]
> > markup.png
> > 
> > So here's what we can do. 
> > If we are in an HTML document, don't show closing tag on void element when
> > the tag is collapsed.
> > If the tag is expanded ( a void element can be expanded if it has
> > pseudo-elements attached to it ), show the closing tag.
> > If we are in an XHTML document ( or SVG, or XML, ... ) always show the
> > closing tag.
> 
> Note that XHTML supports this form as well: <img />. I think it should just
> be easier to show that form all the time, since HTML supports it as well.
> That form is also less confusing, since it emphatises the fact that void
> elements have the same behaviour (adding child nodes does nothing, pseudo
> elements don't work, ...) for HTML and XHTML.

I don't think we should show a self-closing tag in the HTML document, it can gives the user the false impression that you *have to* close tags in order to have valid HTML. I think this is a misconception that's still quite widespread, due to XHTML habits.
However, it would be nice to have the self closing tag in non-HTML document. We could detect if a node has content, and if not, display the self-closing version. It may even not be restricted to void elements.


(In reply to Brian Grinstead [:bgrins] from comment #21)
> I think we should do the simplest thing that renders HTML documents
> correctly for gecko, and not worry about using the non-standard property in
> this case.  As long we don't throw on falsy values of this property from the
> frontend, then worst case for something like servo / valence is that we'll
> be treating all tags in the UI as we do currently.
> 
> Related to document.contentType: I did a bit of searching and it does look
> like DOMInspector is also using it for detection in a few places:
> http://mxr.mozilla.org/comm-central/
> search?find=%2Fmozilla%2Fextensions%2Finspector%2F&string=contenttype

So if I understand well, we can keep using document.contentType for now. FWIW, in a previous patch I was doing something like this server-side : 

`let isHTMLDocument = this.rootDoc.createElement("br").outerHTML === "<br>"`

Could this be a simple solution to handle both gecko and non-gecko browser engines ?

Comment 25

[Julian Descottes [:jdescottes]]

Just to give some background, document.contentType is also used in https://dxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp#545 to check if the document is HTML or XHTML. 

I think we should avoid creating elements on the debuggee on the fly, keeping in mind this needs to be done for each node actor we send down the wire. 
But using a simple boolean "isInHTMLDocument" in the node actor is a very good point. It avoids leaking the Gecko API we use behind the scenes, and will make it easier to build wrappers/create default values etc...

Comment 26

[Patrick Brosset <:pbro>]

Let's just go with your solution (using document.contentType), and rename documentContentType to isInHTMLDocument in the Actor's form.
Thanks!

Comment 27

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8744846 [details]
MozReview Request: Bug 820926 - Hide void-element's closing tag if the page is an HTML page. r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48631/diff/1-2/

Comment 28

[Patrick Brosset <:pbro>]

Comment on attachment 8744846 [details]
MozReview Request: Bug 820926 - Hide void-element's closing tag if the page is an HTML page. r=pbro

https://reviewboard.mozilla.org/r/48631/#review46037

Ok, this seems good to me, granted TRY is green.

Comment 29

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8744846 [details]
MozReview Request: Bug 820926 - Hide void-element's closing tag if the page is an HTML page. r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48631/diff/2-3/

Comment 30

[Nicolas Chevobbe [:nchevobbe]]

TRY (https://treeherder.mozilla.org/#/jobs?repo=try&revision=96fa6750edfa) is green (except 2 known intermittent)

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/84f5f945a868

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/84f5f945a868

Comment 33

[Tanvir Rahman]

I have reproduced this bug with nightly 20.0a1 (2012/12/12) on Windows 10, 64 bit!

The Bug's fix is now verified on Latest Aurora 49.0a2.

Build ID 	20160720004018
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

[bugday-20160720]