Summary: | Web Inspector: Elements: pasting doesn't work when a comment node is selected | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||
Status: | NEW --- | ||||||||
Severity: | Normal | CC: | hi, inspector-bugzilla-changes, joepeck | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | 199182 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Devin Rousso
2019-07-01 23:47:49 PDT
Created attachment 373581 [details]
Patch
Created attachment 374747 [details]
Patch
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 on attachment 374747 [details]
Patch
In the meantime, to drop it from the queue I'm going to r-.
|