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)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: pbro, Assigned: pbro)
References
Details
Attachments
(1 file, 7 obsolete files)
(deleted),
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•11 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•10 years ago
|
||
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]
Assignee | ||
Updated•10 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → pbrosset
Mentor: pbrosset
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [good next bug][lang=js]
Assignee | ||
Comment 2•10 years ago
|
||
I haven't worked on tests yet. Will get to those tomorrow.
Assignee | ||
Comment 3•10 years ago
|
||
v2 - Cleaning up this._editedTagNameNode after it's been used in the undoEditTagName's method.
Attachment #8497851 -
Attachment is obsolete: true
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
Looks like I broke all outerHTML update tests :(
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
v5 - no more undo
try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9312aa566a79
e10s try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bd757b5d286a
Attachment #8498727 -
Attachment is obsolete: true
Attachment #8499620 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
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)
Assignee | ||
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•