Bug 199396 - Web Inspector: Elements: pasting doesn't work when a comment node is selected
Summary: Web Inspector: Elements: pasting doesn't work when a comment node is selected
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords:
Depends on: 199182
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-01 23:47 PDT by Devin Rousso
Modified: 2019-08-02 20:40 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.17 KB, patch)
2019-07-06 14:07 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (6.40 KB, patch)
2019-07-23 18:06 PDT, Devin Rousso
joepeck: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-07-01 23:47:49 PDT
# 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.
Comment 1 Devin Rousso 2019-07-06 14:07:01 PDT
Created attachment 373581 [details]
Patch
Comment 2 Devin Rousso 2019-07-23 18:06:32 PDT
Created attachment 374747 [details]
Patch
Comment 3 Joseph Pecoraro 2019-08-02 20:39:48 PDT
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.
Comment 4 Joseph Pecoraro 2019-08-02 20:40:12 PDT
Comment on attachment 374747 [details]
Patch

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