998933 - [markup view] undo fails after Edit as HTML

Status: NEW

(DevTools :: Inspector, defect, P2)

Description

[Brian Grinstead [:bgrins]]

As reported in https://bugzilla.mozilla.org/show_bug.cgi?id=993416#c15

Right click on a node -> Edit as HTML
Make some changes and save them
Press cmd+z
See an error in the console like this:

console.error: 
  Message: TypeError: node is null
  Stack:
    WalkerActor<.setOuterHTML<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:1727:1
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:903:1
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1110:9
LDT_send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/server/transport.js:258:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:7

Comment 1

[Brian Grinstead [:bgrins]]

The error is because aNode isn't being retained inside of updateNodeOuterHTML.  Retaining the node prevents the error, but doesn't give the expected behavior (because the reference to the node is old after setting outer HTML).

var el = document.querySelector("button");
el.outerHTML; // <button>hi</button>
el.outerHTML = "<button>bye</button>";
el.outerHTML; // <button>hi</button>

One way we could deal with this would be to modify the server so instead of just setting rawNode.outerHTML = value, we could use dom manipulation and return a reference to the most likely similar node for the ability to undo.

The weirdness here is that undo would not restore the state to before:
Original outerHTML: "<button>hi</button>"
Set to "<button>bye</button><h1>new</h1>"
Undo would leave DOM like:
"<button>hi</button><h1>new</h1>"

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: edit-outer-undo-WIP.patch

Just a prototype of what changes in comment 1 could look like

Comment 3

[Brian Grinstead [:bgrins]]

Another alternative would be to rip out the undo functionality for edit as html entirely for now, since it is broken.

Comment 4

[Patrick Brosset <:pbro>]

What about this approach:

- In the setOuterHTML actor method, remember the child index of the element that's about to be changed.
- Then create a MutationObserver just before setting the outerHTML, and make it observe childList changes on the element's parent.
- After outerHTML is set, the observer is going to be called with a single mutation record containing a removed node and (possibly) a list of added nodes.
- Keep these node references around.
- Expose a new actor method, say undoLastOuterHTMLChange, and let the client call it whenever the user undoes the change.
- When called, the method should basically remove the nodes that were previously added and add back the one that was removed (at the recorded child index).

For info, I'm working on bug 917696 which is about remoting the edit tagname functionality and as part of that bug, I'm changing quite a lot of the setOuterHTML front-end code. So I will use the bug to completely remove the outerHTML undo feature for now. This will avoid exceptions.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #4)
> What about this approach:
> 
> - In the setOuterHTML actor method, remember the child index of the element
> that's about to be changed.
> - Then create a MutationObserver just before setting the outerHTML, and make
> it observe childList changes on the element's parent.
> - After outerHTML is set, the observer is going to be called with a single
> mutation record containing a removed node and (possibly) a list of added
> nodes.
> - Keep these node references around.
> - Expose a new actor method, say undoLastOuterHTMLChange, and let the client
> call it whenever the user undoes the change.
> - When called, the method should basically remove the nodes that were
> previously added and add back the one that was removed (at the recorded
> child index).

I'm not sure if we have guaranteed ordering on the mutation observer events (like if a mutation had queued up before this function has called are we sure to get the mutations that are a result of the call to outerHTML).  If it does end up working this way then it sounds like a good way to handle the problem - even though there will still be edge cases with siblings being prepended to the parent it should work in most cases.

> For info, I'm working on bug 917696 which is about remoting the edit tagname
> functionality and as part of that bug, I'm changing quite a lot of the
> setOuterHTML front-end code. So I will use the bug to completely remove the
> outerHTML undo feature for now. This will avoid exceptions.

Sounds good

Comment 6

[Patrick Brosset <:pbro>]

I've looked at the code in more details and I think we could use this bug to simplify it/make it more generic.
The undo.js shared lib is used in only 4 places, these places are all in the markup-view and are all to deal with adding/changing/deleting attributes or nodes.
One of them relies on walker.retainNode to keep the node around in the actor and re-insert it later when needed.

I would like to propose that we:
- simplify the code of markup-view.js so that it only sends walker.undo/redo requests when ctrl+Z/Y is pressed
- completely remove the whole retain/unretainNode mechanism from the walker
- move the undo.js shared lib to be used on the server instead so that methods that actually want to undo stuff just register functions in the undo stack

It's easier on the server to keep element references around without having to keep the NodeActor references and have the client have to deal with that.

Undoing edit tagname or update outerHTML from the client-side would be a little weird at the moment. You'd have to implement something really complex to retain the right nodes and let the client know what to do when undoing.
To do undo edit tagname remotely (in bug 917696), I've had to implement a new 'undoEditTagName' actor method, which really makes me thing we should just have an 'undo' method instead and let the server deal with the stack.