Bug 199396 – Web Inspector: Elements: pasting doesn't work when a comment node is selected

• Component: Web Inspector
• Status: NEW

[Devin Rousso]

# STEPS TO REPRODUCE:
1. inspect any page
2. go to the Elements tab
3. select any node (preferably one that doesn't have any children)
4. copy that node (⌘C)
5. select a non-Element node (e.g. comment or text)
6. paste (⌘V)
 => nothing happens (the outer HTML copied from steps 3 and 4 should be added as a next sibling)

This is because `insertAdjacentHTML` only exists on Element nodes, which comments/text are not.

[Devin Rousso]

Created attachment 373581 [details]
Patch

[Devin Rousso]

Created attachment 374747 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 374747 [details]
Patch

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

Seems good but I didn't understand the `+ 2` which probably means it deserves a comment.

> Source/WebInspectorUI/ChangeLog:14
> +        the functionality of `insertAdjacentHTML` by creating a detached DOM node in the page and
> +        moving each child (`insertBefore`) created from the given HTML to be a child of the parent
> +        of the target DOM node at the right index.

This never defines "target DOM node" and ends up being confusing. May be better with more specifics:

    ... mimic the functionality of `insertAdjacentHTML` by creating a detached DOM node in the page,
    inserting html into that detached node, and transferring the resulting children into the tree at
    the appropriate location.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:558
> +            let object = await WI.RemoteObject.resolveNode(this.parentNode);
> +
> +            function inspectedPage_parent_mimic_insertAdjacentHTML(index, html) {

Style: I'd swap the order of these and then you'd see the 3 lines using `object` right next to each other and it would be easy to understand what is happening.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:562
> +                    this.insertBefore(child, this.childNodes[index + 2]);

Why index + 2?

It seems like in the:
  • "beforebegin" mimic I'd expect insertBefore `children[index]`
  • "afterend" mimic I'd expect insertAfter `children[index]` (or before index+1).

I didn't follow how this became +2.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:576
> +                    } else if (allowed(this.parentNode)) {

For all of these `allowed(this.parentNode)` is it ever possible for a parent to not be an ELEMENT_NODE? I'd expect these to all be just `this.parentNode` checks.

[Joseph Pecoraro]

Comment on attachment 374747 [details]
Patch

In the meantime, to drop it from the queue I'm going to r-.