Bug 179042 – Web Inspector: add contextmenu item to arbitrarily add HTML/Child to DOMTree

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

It would be useful to have an "Add > HTML" button that lets the user input whatever HTML they want.  The same is true of "Add > Previous Sibling" and "Add > Next Sibling".

[Devin Rousso]

Created attachment 325877 [details]
Patch

[Devin Rousso]

Created attachment 325878 [details]
[Image] After Patch is applied

[Build Bot]

Comment on attachment 325877 [details]
Patch

Attachment 325877 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5088065

New failing tests:
imported/blink/fast/text/international-iteration-simple-text.html

[Build Bot]

Created attachment 325879 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Comment on attachment 325877 [details]
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.

[Devin Rousso]

Created attachment 326013 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 326013 [details]
Patch

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

Nice!

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:515
> +            DOMAgent.markUndoableState();
> +            object.callFunction(inspectedPage_node_insertAdjacentHTML, [position, html]);
> +            object.release();

All of the other cases seem to set the undoable state AFTER applying the change. So I think this should be:

    object.callFunction(inspectedPage_node_insertAdjacentHTML, [position, html], () => {
        DOMAgent.markUndoableState();
        object.release();
    });

What do you think?

[Joseph Pecoraro]

Comment on attachment 326013 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:515
>> +            object.release();
> 
> All of the other cases seem to set the undoable state AFTER applying the change. So I think this should be:
> 
>     object.callFunction(inspectedPage_node_insertAdjacentHTML, [position, html], () => {
>         DOMAgent.markUndoableState();
>         object.release();
>     });
> 
> What do you think?

Err I suppose the object.release() can still happen like it currently does, but I still think the mark undoable state should happen AFTER the change.

[Devin Rousso]

Created attachment 326023 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 326023 [details]
Patch

Clearing flags on attachment: 326023

Committed r224456: <https://trac.webkit.org/changeset/224456>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35567688>