1090874 - [markup view] Node deleted and recreated when the tagname field is blurred, even when the tagname didn't change

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

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.

Comment 1

[Willy Aguirre]

Could I take this bug ?

Comment 2

[Patrick Brosset <:pbro>]

(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

Comment 3

[Timothée Pillard [:ziir] [:tim]]

Attachment: 1090874-onTagEdit.patch

Suggested patch.

Comment 4

[Patrick Brosset <:pbro>]

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.

Comment 5

[Timothée Pillard [:ziir] [:tim]]

Attachment: 1090874-onTagEdit-reworked.patch

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 :)

Comment 6

[Patrick Brosset <:pbro>]

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.

Comment 7

[Timothée Pillard [:ziir] [:tim]]

Attachment: 1090874-onTagEdit-reworked.patch

Minor changes.

Comment 8

[Patrick Brosset <:pbro>]

(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.

Comment 9

[Patrick Brosset <:pbro>]

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.

Comment 10

[Patrick Brosset <:pbro>]

Pending try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9efed4f819f2

Comment 11

[Timothée Pillard [:ziir] [:tim]]

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.

Comment 12

[Patrick Brosset <:pbro>]

(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.

Comment 13

[Patrick Brosset <:pbro>]

Here's a new pending try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cd6c31bbca84

Comment 14

[Timothée Pillard [:ziir] [:tim]]

(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!

Comment 15

[Patrick Brosset <:pbro>]

Attachment: edit-tagname.diff

Ok, try is green.
I have only updated the patch to correct the commit message.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/419f0456a41c

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/419f0456a41c