Bug 201757 – Web Inspector: HTML Formatter - better handling for HTML specific tag cases (<p>/<li>)

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

HTML Formatter - better handling for HTML specific tag cases (<p>/<li>)

Cases like:

    <ul>
        <li>one
        <li>two
        <li>Three
    </ul>

Or:

    <div>
        <p>Test
        <p>Test
    </div>

Currently display poorly

[Joseph Pecoraro]

Created attachment 378766 [details]
[PATCH] Proposed Fix

Blocked by other patches.

[Joseph Pecoraro]

Created attachment 378772 [details]
[PATCH] Proposed Fix

Unblocked.

[Joseph Pecoraro]

Comment on attachment 378772 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=378772&action=review

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:137
> +    _autoCloseHTMLNodesForOpenTag(parserNode, node)

I've considered renaming "autoClose" to "implicitlyClose" everywhere. I'm fine with both names though.

[Devin Rousso]

Comment on attachment 378766 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=378766&action=review

> LayoutTests/inspector/formatting/resources/html-tests/auto-close-special.html:7
> +    <option>0

Can we add a second sibling `<option>` here as well? So it’s tested at both nesting levels.

> LayoutTests/inspector/formatting/resources/html-tests/list.html:7
> +<!-- Nested -->

What about nested of the same type?

> LayoutTests/inspector/formatting/resources/html-tests/p-expected.html:13
> +        <p>1

Ditto for another sibling

> Source/WebInspectorUI/ChangeLog:10
> +        Handle a closing tag with different text than the opening tag.

When can this happen?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:98
> +                containerNode = this._stackOfOpenElements[this._stackOfOpenElements.length - 1];

`this._stackOfOpenElements.lastValue`?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:137
> +    _autoCloseHTMLNodesForOpenTag(parserNode, node)

We only seem to use `node` for the `lowercaseName`. Could we just pass that in instead and make the signature slightly easier to read?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:139
> +        if (parserNode.closed)

Ditto

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:172
> +            this._autoCloseNodesExistingOpenTagWithName(["tr"], ["table"]);

Shouldn’t this also close td/th too?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:179
> +            this._autoCloseNodesExistingOpenTagWithName(["thead"], ["table"]);

Ditto, but also with tr

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:182
> +            this._autoCloseNodesExistingOpenTagWithName(["tbody"], ["table"]);

Ditto, but also with tr

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:232
> +    _autoCloseNodesExistingOpenTagWithName(tagNames, containerTagNames)

This name could be clearer to the better explain the difference between the two arguments. I had to read this patch twice before I understood that the second parameter is like a “scope” that limits when the first parameter is applied.

`_autoCloseTagNamesInsideParentTagNames`

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:240
> +            for (let i = existingOpenTagIndex + 1; i < this._stackOfOpenElements.length; ++i) {

Shouldn’t we be searching for nodes _before_ `existingOpenTagIndex`. I thought this was a stack, so wouldn’t the container node be at an earlier/smaller index?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:242
> +                console.assert(!this._xml);

Can we move this assert outside?

[Joseph Pecoraro]

Comment on attachment 378766 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=378766&action=review

>> Source/WebInspectorUI/ChangeLog:10
>> +        Handle a closing tag with different text than the opening tag.
> 
> When can this happen?

Example `<div>Test</DIV>`. In HTML the opening div gets closed. In xml it is case sensitive so behaves differently.

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:74
> +            if (!this._xml)
> +                this._autoCloseHTMLNodesForOpenTag(parserNode, node);

I actually suspect this can't happen because there is no parent to auto-close at this level. So I should be able to remove this.

>> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:98
>> +                containerNode = this._stackOfOpenElements[this._stackOfOpenElements.length - 1];
> 
> `this._stackOfOpenElements.lastValue`?

Sure. FormatterWorker has WorkerUtilities.js which has "lastValue". I assumed Workers didn't have the Utilities.js equivalents!

>> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:137
>> +    _autoCloseHTMLNodesForOpenTag(parserNode, node)
> 
> We only seem to use `node` for the `lowercaseName`. Could we just pass that in instead and make the signature slightly easier to read?

I'd rather we pass both nodes since the nodes are the entire context.

>> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:172
>> +            this._autoCloseNodesExistingOpenTagWithName(["tr"], ["table"]);
> 
> Shouldn’t this also close td/th too?

Good point. I've made this like `optgroup`. First attempt to close a `tr` scoped to a `table`. If that did not close something, close `th`/`td` within the `table`.

>> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:240
>> +            for (let i = existingOpenTagIndex + 1; i < this._stackOfOpenElements.length; ++i) {
> 
> Shouldn’t we be searching for nodes _before_ `existingOpenTagIndex`. I thought this was a stack, so wouldn’t the container node be at an earlier/smaller index?

The efficient way to use an Array as a stack is to use the end of the stack as the top of the stack. `pop` is much cheaper than `unshift` since no elements in the array need to move on every operation.

>> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:242
>> +                console.assert(!this._xml);
> 
> Can we move this assert outside?

I wanted the assert next to the `lowercaseName` call so that it is obvious to anyone reading it that this lowercaseName call is okay because we are not XML. I'll move the assert and put a message in the assertion instead.

[Joseph Pecoraro]

Created attachment 378877 [details]
[PATCH] Proposed Fix

[Radar WebKit Bug Importer]

<rdar://problem/55409987>

[Devin Rousso]

Comment on attachment 378877 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=378877&action=review

r=me, nice work!

> LayoutTests/inspector/formatting/resources/html-tests/auto-close-special.html:2
> +<head><title>Title</title>

Can we omit the `</title>` to ensure that the `<body>` still closes the `<head>` even if there are other open tags "in between"?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/HTMLTreeBuilderFormatter.js:241
> +        console.assert(!this._isXML, "Implicitly closing only happens in HTML. Also, names are compared case insensitively which would be invalid for XML");

NIT: missing "." at the end :(

[Joseph Pecoraro]

<https://trac.webkit.org/r249969>