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".
Created attachment 325877 [details] Patch
Created attachment 325878 [details] [Image] After Patch is applied
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
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
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.
Created attachment 326013 [details] Patch
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?
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.
Created attachment 326023 [details] Patch
Comment on attachment 326023 [details] Patch Clearing flags on attachment: 326023 Committed r224456: <https://trac.webkit.org/changeset/224456>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35567688>