Closed Bug 917696 Opened 11 years ago Closed 10 years ago

devtools markup view doesn't allow tag changes remotely

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: pbro, Assigned: pbro)

References

Details

Attachments

(1 file, 7 obsolete files)

Although the markup-view inspector works with remote targets, it doesn't support the same level of functionality.

Namely, changing a tag name is not supported.

In markup-view.js, class ElementEditor:

  this.rawNode = aNode.rawNode();

  // Make the tag name editable (unless this is a remote node or
  // a document element)
  if (this.rawNode && !aNode.isDocumentElement) {
    this.tag.setAttribute("tabindex", "0");
    editableField({
      element: this.tag,
      trigger: "dblclick",
      stopOnReturn: true,
      done: this.onTagEdit.bind(this),
    });
  }

As documented in inspector.js, accessing rawNode is only for local targets:

  /**
   * Get an nsIDOMNode for the given node front.  This only works locally,
   * and is only intended as a stopgap during the transition to the remote
   * protocol.  If you depend on this you're likely to break soon.
   */
  rawNode: function(rawNode) {...}

The goal of this bug is therefore to make this work remotely.
Priority: -- → P2
This bug isn't straightforward and requires a little bit of knowledge of our codebase, but is doable by someone who's contributed already and I can mentor it. Filing as good next bug.
Mentor: pbrosset
Whiteboard: [good next bug][lang=js]
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → pbrosset
Mentor: pbrosset
Status: NEW → ASSIGNED
Whiteboard: [good next bug][lang=js]
I haven't worked on tests yet. Will get to those tomorrow.
v2 - Cleaning up this._editedTagNameNode after it's been used in the undoEditTagName's method.
Attachment #8497851 - Attachment is obsolete: true
I think this is ready for a first review.
I've enabled the existing tagedit test on e10s.
Pending try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=02d8eda80660
Attachment #8497853 - Attachment is obsolete: true
Attachment #8498021 - Flags: review?(bgrinstead)
Looks like I broke all outerHTML update tests :(
I think this fixes the test failures.
New try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6224525dad73
Attachment #8498021 - Attachment is obsolete: true
Attachment #8498021 - Flags: review?(bgrinstead)
Attachment #8498727 - Flags: review?(bgrinstead)
Comment on attachment 8498727 [details] [diff] [review]
bug917696-remote-edit-tagname v4.patch

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

I like the approach overall, but the patch needs rebasing and also it looks like there would be an issue currently with undoing multiple changes

::: toolkit/devtools/server/actors/inspector.js
@@ +2096,5 @@
> +      return;
> +    }
> +
> +    let attrs = oldNode.attributes;
> +    for (let i = 0 ; i < attrs.length; i ++) {

Nit extra space before ;

@@ +2105,5 @@
> +    oldNode.parentNode.insertBefore(this._editedTagNameNode, oldNode);
> +    while (oldNode.firstChild) {
> +      this._editedTagNameNode.appendChild(oldNode.firstChild);
> +    }
> +    // Remove the old node.

No need for this comment

@@ +2128,5 @@
> +    if (!this._editedTagNameNode) {
> +      return;
> +    }
> +
> +    this.editTagName(this._ref(this._editedTagNameNode), this._editedTagName);

I believe this will fail in the case of editing two tag names in a row then undoing twice:

Edit span#node1 tagName to foo
Edit p#node2 tagName to bar
undoEditTagName() // resets bar#node2 to p#node2 (and resets this._editedTagName / this._editedTagName
undoEditTagName() // resets p#node2 back to bar#node2 instead of changing #node1

Probably the easiest way to fix this would be to stuff these two variables into some indexed storage here and pass back an id to the client that it can then pass that ID back up to undoEditTagName which would allow us to find the correct {editedTagNameNode, editedTagName});
Attachment #8498727 - Flags: review?(bgrinstead)
As discussed with Brian, let's remove the undo editTagName feature altogether from this patch and work on a second patch that handles the undo/redo on the actor-side.
Comment on attachment 8499620 [details] [diff] [review]
bug917696-remote-edit-tagname v5.patch

Sorry, I forgot to do some clean-up.
Attachment #8499620 - Flags: review?(bgrinstead)
v6 is ready for review. Just like v5, but removing the now useless this._editedTagName property set on the walker actor for undo.
Attachment #8499620 - Attachment is obsolete: true
Attachment #8499648 - Flags: review?(bgrinstead)
Comment on attachment 8499648 [details] [diff] [review]
bug917696-remote-edit-tagname v6.patch

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

::: browser/devtools/markupview/markup-view.js
@@ +860,5 @@
> +    };
> +
> +    // Make sure we're not still listening to markupmutation from an earlier
> +    // html/tagname change which wasn't carried out eventually.
> +    this._inspector.off("markupmutation", onMutations);

I don't think this is actually removing the correct callback in the case where a previous change was not carried out.  This function is declared within this scope so it will be a new instance each for call.  I guess we could store this as let onMutations = this.onMutations = () => {} and remove the listener from this.onMutations before that happens?

Either way, I'd like to see a tag name editing test cover this case where an invalid tag name is called in followed by a valid tag name so that we can make sure it isn't firing onMutations twice.
Comment on attachment 8499648 [details] [diff] [review]
bug917696-remote-edit-tagname v6.patch

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

Everything looks good except for the note in Comment 12
Attachment #8499648 - Flags: review?(bgrinstead)
Good catch on the event listener Brian. Tbh I hadn't given much thoughts to the case where the update would fail.

Here's a new patch that should handle this case:

The outerhtml and tagname editing markup-view functions can now call a new cancelReselect function, which they do if the update on the server-side fails.
I've also added a new test case.

Pending try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ea58fcc9f2ce
Attachment #8499648 - Attachment is obsolete: true
Attachment #8500314 - Flags: review?(bgrinstead)
Comment on attachment 8500314 [details] [diff] [review]
bug917696-remote-edit-tagname v7.patch

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

OK, just a couple of thoughts wrt to reselectOnRemoved / cancelReselectOnRemoved.  I don't think we are going to be able to make it completely perfect in all scenarios so I have a couple of ideas to make sure that we don't add any observers that never get removed on _inspector.

I wonder if there is a way to 'replace' the node on the server side so that the frontend could just act like it was somehow the same reference even though the raw node has been swapped out.  That would make this code a lot simpler

::: browser/devtools/markupview/markup-view.js
@@ +830,2 @@
>  
> +    let onMutations = this._removedNodeObserver = (e, mutations) => {

This is quite tricky - I *think* we should call cancelReselectOnRemoved before resetting this._removedNodeObserver just to be make sure that we don't hold onto a listener that never gets removed.

1) Bad Edit HTML Request -> this._removedNodeObserver = func 1
2) Good Tag Name Request -> this._removedNodeObserver = func 2
3) Bad Edit HTML Server Response -> cancelReselectOnRemoved() but this._removedNodeObserver is func 2
4) Good Tag Name Server Response -> No this._removedNodeObserver (and func 2 has already been unbound), but func 1 is still bound to the markupmutation event

This scenario won't even be great with this suggested change, but at least we won't have an observer that never gets removed:

1) Bad Edit HTML Request -> this._removedNodeObserver = func 1
2) Good Tag Name Request -> Unbind func 1 from markupmutation, this._removedNodeObserver = func 2
3) Bad Edit HTML Server Response -> cancelReselectOnRemoved() but this._removedNodeObserver is func 2
4) Good Tag Name Server Response -> No this._removedNodeObserver so reselection doesn't happen, but there is no leaky event

@@ +868,5 @@
> +   * Make sure to stop listening for node removal markupmutations and not
> +   * reselect the corresponding node when that happens.
> +   * Useful when the outerHTML/tagname edition failed.
> +   */
> +  cancelReselectOnRemoved: function() {

Should this (or a variation of it) also be called on destroy to make sure we don't hold onto the inspector object?
Attachment #8500314 - Flags: review?(bgrinstead)
As discussed on IRC, we can probably just avoid these parallel requests use cases by canceling the previous reselection if there is one in 'reselectOnRemoved'.
Here's an updated patch, let me know what you think.
Attachment #8500314 - Attachment is obsolete: true
Attachment #8501011 - Flags: review?(bgrinstead)
Comment on attachment 8501011 [details] [diff] [review]
bug917696-remote-edit-tagname v8.patch

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

This looks good to me.  It makes the markup view fully remoted and I think simplifies the frontend by getting rid of functionality in the mutation observer.  We can handle any other edge cases with reselection and undo/redo in separate bugs
Attachment #8501011 - Flags: review?(bgrinstead) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a633be151fa7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.