[webkit-reviews] review denied: [Bug 179042] Web Inspector: add contextmenu item to arbitrarily add HTML/Child to DOMTree : [Attachment 325877] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 179042: Web Inspector: add contextmenu item to arbitrarily add HTML/Child
to DOMTree
https://bugs.webkit.org/show_bug.cgi?id=179042

Attachment 325877: Patch

https://bugs.webkit.org/attachment.cgi?id=325877&action=review




--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 325877
  --> https://bugs.webkit.org/attachment.cgi?id=325877
Patch

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

r- for a couple reasons:

  • Should mark an undoable state somewhere to work well with undo/redo
  • If you have a <h3>Test</h3> and "add child" the element expands but still
has the inline text content, this should probably be broken out like a normal
element with non-1-inline-text-child

Other than those, this feels really great!

---

Enhancement ideas: (could happen now, or later, or never, up to you)

Since `Esc` will cause editing to cancel easily, I think we can provide some
initial values to save users some typing.

What do you think of these simple starters:

  • Insert child of <ul>/<ol> could pre-populate with "<li>|</li>"
  • Insert sibling of <li> could pre-populate with "<li>|</li>"
  • Insert child of <tr> could pre-populate with "<td>|</td>"
  • Insert sibling of <td> could pre-populate with "<td>|</td>"
  • Insert child of <table> could pre-populate with "<tr>|</tr>"

We could even show that in the menu items like:

  Add > Child (<li>)
	Previous Sibling
	Next Sibling

We could event extend to detecting a near-by pattern:

  Add > Child (<div class="section">)
	Previous Sibling
	Next Sibling

  Add > Child
	Previous Sibling (<li class="item">)
	Next Sibling  (<li class="item">)

For example if the element has >2 siblings and >50% of the element's siblings
have a common class name.

Other potential improvements:

  • Disallow adding a child of <script>, <style> as that is probably not what
users intend
  • Disallow adding a child of <link>, <img>, <hr>, <br> and other elements
expecting to not have children

At some point this gets to be "too smart" and ends up hurting usability. But I
think keeping it simple will save developers some time.

> Source/WebInspectorUI/ChangeLog:3
> +	   Web Inspector: add contextmenu item to arbitrarily add HTML/Child to
DOMTree

Maybe "Add" should now be "Append"? That seems more natural for the majority of
the items (Child, Sibling) and isn't too bad for the remaining (Attribute). But
"Add" seems fine if you don't want to change.

> Source/WebInspectorUI/ChangeLog:13
> +	   Call-through for `insertAdjacentHTML` on the corresponding node in
the inspected page.

Typo?: "Call-through for" => "Call-through to"

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:508
> +	   WI.RemoteObject.resolveNode(this).then((object) => {

r- because I think this won't work as expected with undo, but that should be
easy to fix.

I think you want to be doing _makeUndoableCallback somewhere around here, or at
least marking an undoable state.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:509
> +	       function inspectedPage_node_insertAdjacentHTML(position, html) {

\o/ great naming.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1602
> +	   if (position === "afterbegin" || position === "beforeend") {
> +	       this.hasChildren = true;
> +	       this.shouldRefreshChildren = true;
> +	       this.expand();
> +	   }

You might have to updateTitle around here if you have an element currently
displaying with inline text. For example <h3>Test</h3> and you want to insert
adjacent HTML to it.