Closed
Bug 1090874
Opened 10 years ago
Closed 10 years ago
[markup view] Node deleted and recreated when the tagname field is blurred, even when the tagname didn't change
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 37
People
(Reporter: pbro, Assigned: ziir, Mentored)
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
STR: - Open the inspector - Select any node - Double-click on the tagname field to switch it to edit mode - Leave the field (using tab) without actually changing the tagname ER: the field is blurred and nothing happens AR: a mutation occurs: the node is deleted and recreated again, just like if the tagname was changed. The bug is here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#2231 Specifically when evaluating this expression: |newTagName == this.node.tagName|, one is lowercase this other uppercase, so they are never equal.
Reporter | ||
Updated•10 years ago
|
Whiteboard: [good first bug][lang=js]
Reporter | ||
Comment 2•10 years ago
|
||
(In reply to Willy Aguirre from comment #1) > Could I take this bug ? Sure, make sure you have the dev environment setup first: https://wiki.mozilla.org/DevTools/GetInvolved#Hacking
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8527245 [details] [diff] [review] 1090874-onTagEdit.patch Review of attachment 8527245 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch. I'll assign the bug to you. As commented below, the fix isn't correct though. Also, we need a new test to make sure this problem doesn't come back in the future. Markup-view tests are in /browser/devtools/markupview/test/. To add a new test: - create a new file in this directory, I suggest the following name: browser_markupview_tag_edit_11.js - add an entry in browser.ini for this new file - follow the examples in other similar test files in this directory to write your new test. Your test should do the following: - add a new test tab - open the inspector - select a test node - switch the tagname to edit mode - just blur the field without doing any change - verify that no tag editing was done. Because editing tags is an asynchronous operation, it is difficult to ensure that nothing was done. My suggestion is to mock the call to the async tag edition function, and make it sync instead, this way after blurring the field, you can immediately check that it wasn't called. For instance, in the onTagEdit method, this.markup.walker.editTagName() is called, that's the async function that changes the tagname. In your test, you could just do something like this.markup.walker.editTagName = function() {isEditTagNameCalled = true;} And then make your test check for the value of isEditTagNameCalled. To run your test, enter the following command: ./mach mochitest-devtools browser/devtools/markupview/test/browser_markupview_tag_edit_11.js Let me know if you need more information, I don't know how much about the code base and the test system you already know. ::: browser/devtools/markupview/markup-view.js @@ +2229,5 @@ > /** > * Called when the tag name editor has is done editing. > */ > onTagEdit: function(newTagName, isCommit) { > + if (!isCommit || newTagName.toUpperCase() === this.node.tagName || This does not fix the issue (at least in my test) since this.node.tagName is lowercase, and you're making newTagName uppercase here. To make sure the comparison is safe, just lowercase both tagNames.
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → tim+bugzilla
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•10 years ago
|
||
Thanks for the detailed feedback and instructions. I assumed this.node.tagName would always return an uppercase string as it does with HTML. I have created a new patch which the modifications you suggested. Let me know if there's anything else I can do, or, if I did something wrong :)
Attachment #8527245 -
Attachment is obsolete: true
Attachment #8527673 -
Flags: review?(pbrosset)
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8527673 [details] [diff] [review] 1090874-onTagEdit-reworked.patch Review of attachment 8527673 [details] [diff] [review]: ----------------------------------------------------------------- R+ because my comments are really just minor changes. I think it still makes sense to revisit the patch to get this done because these are really quick changes. Other than this, the patch looks good, thanks for working on this. I'll push the updated patch to our TRY server to check nothing's broken because of this change. In the meantime, you can also run the tests locally to detect any obvious problems: I think just running the markup-view tests would be enough: > ./mach mochitest-devtools browser/markupview/test/ ::: browser/devtools/markupview/test/browser_markupview_tag_edit_11.js @@ +3,5 @@ > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +// Tests that a node is not recreated when it's tagname is blurred. "Tests that a node is not recreated when its tagname editor is blurred and no changes were done" @@ +4,5 @@ > + > +"use strict"; > + > +// Tests that a node is not recreated when it's tagname is blurred. > +// Bug 1090874 - https://bugzilla.mozilla.org/show_bug.cgi?id=1090874 nit: mentioning the bug number if enough, no need for the URL. @@ +7,5 @@ > +// Tests that a node is not recreated when it's tagname is blurred. > +// Bug 1090874 - https://bugzilla.mozilla.org/show_bug.cgi?id=1090874 > + > +const TEST_URL = "data:text/html;charset=utf-8,<div></div>"; > + nit: just one empty line is fine here. @@ +10,5 @@ > +const TEST_URL = "data:text/html;charset=utf-8,<div></div>"; > + > + > +let test = asyncTest(function*() { > + nit: no need for an empty line at the start of this function. @@ +15,5 @@ > + let isEditTagNameCalled = false; > + > + let {toolbox, inspector} = yield addTab(TEST_URL).then(openInspector); > + > + inspector.walker.editTagName = function() { isEditTagNameCalled = true; } This requires a comment I think, because it's not something we do very often, and it can lead to weirdness later when someone's trying to debug why editTagName doesn't work as it should. So something like: // Overriding the editTagName walkerActor method here to check that it isn't called when blurring the tagname field. @@ +17,5 @@ > + let {toolbox, inspector} = yield addTab(TEST_URL).then(openInspector); > + > + inspector.walker.editTagName = function() { isEditTagNameCalled = true; } > + > + yield inspector.markup.expandAll(); You don't need to expandAll here, the <body> node is expanded by default when opening the inspector, and so you can select the div node directly. @@ +22,5 @@ > + yield selectNode("div", inspector); > + let container = yield getContainerForSelector("div", inspector); > + let tagEditor = container.editor.tag; > + > + nit: just one empty line @@ +34,5 @@ > + > + info("Updating the tagname to a different value"); > + setEditableFieldValue(tagEditor, "SPAN", inspector); > + is(isEditTagNameCalled, true, "The editTagName method was called"); > + nit: no need for an empty line at the end of this function.
Attachment #8527673 -
Flags: review?(pbrosset) → review+
Reporter | ||
Comment 8•10 years ago
|
||
(In reply to Ziir from comment #7) > Created attachment 8528497 [details] [diff] [review] > 1090874-onTagEdit-reworked.patch > > Minor changes. Since I already R+ the previous patch and these are only minor comments, it's ok to R+ the new patch yourself. Let me do this now.
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8528497 [details] [diff] [review] 1090874-onTagEdit-reworked.patch Review of attachment 8528497 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, I'll push this patch to try to see if all tests still pass fine after this change.
Attachment #8528497 -
Flags: review+
Reporter | ||
Comment 10•10 years ago
|
||
Pending try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9efed4f819f2
Flags: needinfo?(tim+bugzilla)
Assignee | ||
Comment 11•10 years ago
|
||
Hi, I'm unsure what's happenning with these tests. Also, I have noticed that I don't cancel the inspector.walker.editTagName override at the end of the test. Should I submit a new patch that takes care of it? Thanks.
Flags: needinfo?(tim+bugzilla)
Reporter | ||
Comment 12•10 years ago
|
||
(In reply to Ziir from comment #11) > Hi, > I'm unsure what's happenning with these tests. Sorry about that, I should have explained. Also, sorry for the delay. So, try is our test infrastructure. Whenever a patch is ready and we want to make sure it doesn't unexpectedly breaks other things, we push that patch to our try server which ends up building and starting various tests on various machines and OSs. I just realized that for some reason, my push didn't work. I'll push again to try soon. > Also, I have noticed that I don't cancel the inspector.walker.editTagName > override at the end of the test. > Should I submit a new patch that takes care of it? No that should be ok, each test re-opens the toolbox, and so re-instantiate a walker front, so the override will be gone.
Reporter | ||
Comment 13•10 years ago
|
||
Here's a new pending try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cd6c31bbca84
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #12) > (In reply to Ziir from comment #11) > > Hi, > > I'm unsure what's happenning with these tests. > Sorry about that, I should have explained. Also, sorry for the delay. That's absolutely ok :) > So, try is our test infrastructure. Whenever a patch is ready and we want to > make sure it doesn't unexpectedly breaks other things, we push that patch to > our try server which ends up building and starting various tests on various > machines and OSs. I figured that, but I was unable to determine what wasn't working and why. > I just realized that for some reason, my push didn't work. I'll push again > to try soon. > > Also, I have noticed that I don't cancel the inspector.walker.editTagName > > override at the end of the test. > > Should I submit a new patch that takes care of it? > No that should be ok, each test re-opens the toolbox, and so re-instantiate > a walker front, so the override will be gone. Yes, of course, I didn't think about it. (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #13) > Here's a new pending try build: > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cd6c31bbca84 I'm glad to see that nothing seems to break from the changes we've made - failed tests seems unrelated -, yay!
Reporter | ||
Comment 15•10 years ago
|
||
Ok, try is green. I have only updated the patch to correct the commit message.
Attachment #8528497 -
Attachment is obsolete: true
Attachment #8532292 -
Flags: review+
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 16•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/419f0456a41c
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/419f0456a41c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 37
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•