1100001 - paste outer HTML should be undoable

Status: NEW

(DevTools :: Inspector, enhancement, P3)

Description

[Florent Fayolle]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141110195804

Steps to reproduce:

1. Copy this code (without the quotes): "<p>foo</p>"
2. Open the Devtools inspector panel on this page
3. Select some elements (let's say #title > a)
4. Right-Click -> "Paste Outer HTML"
    |=> Bugzilla@Mozilla should be replaced with "Foo"
5. Press Ctrl-Z


Actual results:

The previous element should be restored.

Comment 1

[Florent Fayolle]

Attachment: 1100001.patch (WIP)

WIP. Almost done, still have some trouble with |node.nextSibling()| in the patch (I have to understand why it doesn't return the node we expect).

Florent

Comment 2

[Florent Fayolle]

Comment on attachment 8530681 [details] [diff] [review]
1100001.patch (WIP)

Review of attachment 8530681 [details] [diff] [review]:
-----------------------------------------------------------------

I took a little look at NodeFront#_next, it looks like _next and _prev are inverted in some (all?) situations. For example when I edit the <h2> tag of this page: http://perdu.com

It appears that the _next and _prev members are set through reparent() ( https://hg.mozilla.org/mozilla-central/file/2273193cc525/toolkit/devtools/server/actors/inspector.js#l885 ), but I don't understand the algorithm.

Patrick, do you think using |NodeFront#nextSibling()| in markup-view.js is the right way for this patch? Or should I request it through |this.walker.nextSibling(node)|?

Florent

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8530681 [details] [diff] [review]
1100001.patch (WIP)

Review of attachment 8530681 [details] [diff] [review]:
-----------------------------------------------------------------

I hope my comment below answer your question about using _next and _prev. In short: you shouldn't.

::: toolkit/devtools/server/actors/inspector.js
@@ +752,5 @@
> +  /**
> +   * Returns the next sibling NodeFront for this NodeFront.
> +   */
> +  nextSibling: function() {
> +    return this._next;

this._next isn't guarantied to be the next sibling. In fact, for any NodeFront, the _prev and _next properties are only used internally to be able to reparent nodes, when processing mutations, or creating a new front.
In fact, they are just used to build a linked list of known nodes.

You can't assume that all children nodes of a node are known, and are in the right order, so you shouldn't expose these 2 properties publicly here.

Instead, use the walker actor's previousSibling and nextSibling methods if needed.

Comment 4

[Florent Fayolle]

Thanks, I will update the code soon.

Florent

Comment 5

[Brian Grinstead [:bgrins]]

There is also Bug 998933 opened for undoing 'edit as html' which is pretty much the same thing, and has some related discussion

Comment 6

[mgkac111]

Mozilla/5.0 (Windows NT 10.0; rv:44.0) Gecko/20100101 Firefox/44.0

Steps to reproduce:

1. Copy this code (without the quotes): "<p>foo</p>"
2. Open the Devtools inspector panel on this page
3. Select some elements (let's say #title > a)
4. Right-Click -> "Paste Outer HTML"
    |=> Bugzilla@Mozilla should be replaced with "Foo"
5. Press Ctrl-Z


Actual results:

The previous element should be restored.

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

Filter on CLIMBING SHOES

Comment 8

[Matteo Ferretti [:zer0] [:matteo]]

Filter on CLIMBING SHOES

Comment 9

[Sebastian Zartner [:sebo]]

My fault! This is unrelated to undo/redo within the Rules view.

Sebastian