* SUMMARY Edit as HTML not working in DOM Tree. * STEPS TO REPRODUCE 1. Inspector <body> on about:blank 2. Right click "Edit as HTML" for <body> element 3. Commit "<body><h1>Test</h1></body>" => DOM is unchanged, expected <h1>Test</h1> but it doesn't exist * NOTES - Regressed with <http://trac.webkit.org/changeset/173684>
<rdar://problem/22364817>
I'm going to write at least a basic test for DOM.setOuterHTML to make sure we don't break it again!
Created attachment 259546 [details] [PATCH] Proposed Fix
Created attachment 259559 [details] [PATCH] Proposed Fix Removed some trailing whitespace.
Is it a dupe of https://bugs.webkit.org/show_bug.cgi?id=147736? (I haven't looked closely into the issue.)
Comment on attachment 259559 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259559&action=review > Source/WebCore/inspector/DOMPatchSupport.cpp:-377 > - Node* node = parentNode->firstChild(); Could you explain what it wrong with the optimization? Why is this not equivalent? The Changelog doesn't say anything.
*** Bug 147736 has been marked as a duplicate of this bug. ***
Comment on attachment 259559 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259559&action=review The setOuterHTML test is making me happy. I think our testing-fu is increasing. Please address Chris' question, though. It seems to be the meat of the fix, but I also have no idea why it is correct without spending a lot of time reading DOMPatch. That code is pretty gnarly, I'm surprised the Googlers didn't add tests. (edit: they did write tests, lots of them. In particular, they also tested undo of setOuterHTML. You can see all of their tests here: http://trac.webkit.org/browser/trunk/LayoutTests/inspector/elements?rev=107399&order=name) > LayoutTests/inspector/dom/getOuterHTML.html:20 > + InspectorTest.log(outerHTML); I would like us to get in the habit of labelling stuff that we dump. Here, the expected file would be more comprehensible if it said above: "Dumping outerHTML for element div#x:" > LayoutTests/inspector/dom/setOuterHTML.html:31 > +</div>`.trim(); awesome. > LayoutTests/inspector/dom/setOuterHTML.html:66 > + InspectorTest.expectThat(outerHTML === replacementOuterHTML, "The outerHTML should be what was just set"); Nit: period > LayoutTests/inspector/dom/setOuterHTML.html:77 > + InspectorTest.assert(domNode, "DOMNode exists"); Nit: period
Comment on attachment 259559 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259559&action=review >> Source/WebCore/inspector/DOMPatchSupport.cpp:-377 >> - Node* node = parentNode->firstChild(); > > Could you explain what it wrong with the optimization? Why is this not equivalent? The Changelog doesn't say anything. The optimization changes were wrong for a few reasons: - The loop would break out when we reached the end of the list of children (e.g. if !node) but that is fine if newMap/newList wanted to add a new node - `node` is not longer at index `i` when insertBefore inserts a node before it. At that point, `node` would be at index `i + 1`, so future uses of `node` would likely be off-shifted. I could, and did try correcting while keeping the optimizations but reading through DOMPatchSupport I couldn't be sure I wasn't overlooking any particular cases without writing tons more tests. Also, the loop right after this does the same thing without this attempt at an optimization, so this at least keeps them consistent.
Comment on attachment 259559 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259559&action=review r=me for the C++ part. I trust you and Brian for the inspector tests. >>> Source/WebCore/inspector/DOMPatchSupport.cpp:-377 >>> - Node* node = parentNode->firstChild(); >> >> Could you explain what it wrong with the optimization? Why is this not equivalent? The Changelog doesn't say anything. > > The optimization changes were wrong for a few reasons: > > - The loop would break out when we reached the end of the list of children (e.g. if !node) but that is fine if newMap/newList wanted to add a new node > - `node` is not longer at index `i` when insertBefore inserts a node before it. At that point, `node` would be at index `i + 1`, so future uses of `node` would likely be off-shifted. > > I could, and did try correcting while keeping the optimizations but reading through DOMPatchSupport I couldn't be sure I wasn't overlooking any particular cases without writing tons more tests. Also, the loop right after this does the same thing without this attempt at an optimization, so this at least keeps them consistent. Ok, so we're basically modifying the children as we iterate over them :/
Created attachment 259636 [details] [PATCH] For Landing Addressed comments.
Comment on attachment 259636 [details] [PATCH] For Landing Clearing flags on attachment: 259636 Committed r188768: <http://trac.webkit.org/changeset/188768>
All reviewed patches have been landed. Closing bug.