Closed Bug 1311941 Opened 3 years ago Closed 3 years ago

Adding a new node using the + button in the inspector toolbar should keep the parent selected

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: pbro, Assigned: djmdeveloper060796, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 3 obsolete files)

STR:
- open any page
- open inspector
- select any node
- click on the + icon in the inspector toolbar
- click again a second time

Expected: I would like these 2 new elements to be siblings, direct children of the element I selected in step 3

Actual: the new created element gets selected, so the second element becomes its child.

UX discussion: this feature was added in bug 1261781 and it was decided that the newly created nodes would get selected because this way users can modify them further. 
Because the node gets selected immediately, you can delete it by pressing the DELETE key, you can press enter to edit the tagname, or tab to add an attribute.

If we decide here that it's better to *not* select the newly created node, then those things won't work, you'll first need to navigate to the node to do this.

Having said this, I've used this feature quite a bit already, and every time I wanted the new nodes to be siblings, not children of each other.
And because that didn't work, I was either reselecting the parent and then pressing + again, or just creating one element only and then using the "duplicate element" context menu item.

Gabriel has also said that it would make more sense to keep the parent selected.
I think we should go with keeping the parent selected. After having actually used this feature a number of times, it just makes more sense to me.
Of course we could also have a setting and have the 2 behaviors, but a setting just for this is overkill.

So, if someone is interested in fixing this, I can help.

- Make sure you go through our contribution guide first: https://developer.mozilla.org/en-US/docs/Tools/Contributing
- The function that is responsible for adding the new node is in the inspector.js file, here is a link to it:
http://searchfox.org/mozilla-central/rev/445097654c1cc1098ee0171a29c439afe363a588/devtools/client/inspector/inspector.js#1483
- Once the fix is done, the following test should also be changed devtools\client\inspector\test\browser_inspector_addNode_03.js
Mentor: pbrosset
Keywords: good-first-bug
Priority: -- → P2
Attached patch bug1311941.patch (obsolete) — Splinter Review
This is a test patch(dif). I have changed the addNode function so that the newly added node is not selected and the focus is still on the parent node.I am not quite sure about the test file. Should I just remove the tests which checks whether the new node is selected ?
Flags: needinfo?(pbrosset)
Attachment #8805783 - Flags: review?(pbrosset)
(In reply to Deepjyoti Mondal from comment #2)
> This is a test patch(dif). I have changed the addNode function so that the
> newly added node is not selected and the focus is still on the parent node.
Thank you! I'll assign the bug to you and review the code changes.
> I am not quite sure about the test file. Should I just remove the tests which
> checks whether the new node is selected ?
The testAddNode function in browser_inspector_addNode_03.js needs to be updated. There is some code that checks whether the new node is selected:
is(newNode, inspector.selection.nodeFront,
     "The new node is selected");
So at least this part needs to be changed. It should check that the parent is still selected instead.
Also, minor change but still important: the comment at the very top of this test file also needs to be changed. It says that the new node should be selected, that's not true anymore.
Assignee: nobody → djmdeveloper060796
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)
Comment on attachment 8805783 [details] [diff] [review]
bug1311941.patch

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

Looks good.
Please do submit a new patch with the test changes as described in the last comment.
Attachment #8805783 - Flags: review?(pbrosset) → feedback+
I will update the test and submit a patch soon.
Attached patch bug1311941.patch (obsolete) — Splinter Review
Updated the test file.
Attachment #8805783 - Attachment is obsolete: true
Attachment #8806458 - Flags: review?(pbrosset)
Comment on attachment 8806458 [details] [diff] [review]
bug1311941.patch

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

Nice. Almost there. Still needs a little bit of work on the test.

::: devtools/client/inspector/test/browser_inspector_addNode_03.js
@@ +49,4 @@
>    btn.click();
>    let mutations = yield onMutation;
>  
> +  info("Expecting an inspector-updated event right after the mutation event");

The test doesn't work for me locally, it times out, blocked while waiting for the inspector-updated event.
Since we are no longer selecting a different node, I believe this needs to be removed.

I also get a couple of other test failures in the same test:

Later in this test, we check that selectedContainer has the right "aria-level" attribute, but selectedContainer isn't the same as before. It used to be the child, now it's the parent. So you should change selectedContainer with newChildContainer (or something like this). mutations[0].added should give you enough information (I believe) to access the new element's container.

And finally, the test also checks focusedContainer, but the way this is done seems to fail.

Try and run this test locally with ./mach mochitest devtools/client/inspector/test/browser_inspector_addNode_03.js , you should get the same test failures that I'm getting.

Let me know if you need more help, I'm happy to provide bits and pieces of code for this test.
Attachment #8806458 - Flags: review?(pbrosset) → feedback+
Attached patch bug1311941.patch (obsolete) — Splinter Review
I have removed the part of the test which waited for inspector-updated event.
I have replaced selectedContainer with newChildContainer which is the container of the newly added node. I am not quite sure about the focusedContainer. Also when I am running this test I am getting passed:0 and failed:0. Somewhere I am making a mistake but I am not quite sure where.Some pointers regarding this will be of great help.
Attachment #8806458 - Attachment is obsolete: true
Attachment #8807341 - Flags: review?(pbrosset)
Comment on attachment 8807341 [details] [diff] [review]
bug1311941.patch

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

I investigated the test failures locally, and that made me realize I had forgotten about something: when a new node is added inside an element that appears collapsed in the inspector, then that element should become expanded. This is so that you can see the newly added node.
So, I added this locally, and proceeded to fix the test. I think I'm going to attach a patch here, because it took me a while to fix the test, and although I could guide you to fix it too, it would only be about internal inspector test framework things that are sort not very interesting and useful. So if that's not too much of a problem, I'll attach the corrected patch I did locally.
Please take a look at it, ask questions about it, etc. And let me know what you think.
I'll push it to TRY too, to see if the change broke other things or not.
Attachment #8807341 - Flags: review?(pbrosset)
Attached patch bug1311941.patchSplinter Review
Please take a look and let me know.

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=363e5ba7f61f3690ac03856c61b7fa78de027a5f
Could you keep an eye on it and see if there are any related failures when it completes?
Flags: needinfo?(djmdeveloper060796)
Attachment #8808692 - Flags: review+
Thanks a lot for modifying my patch, I will be keeping an eye on the progress.
Flags: needinfo?(djmdeveloper060796)
The test is complete, one red and one exception.
Flags: needinfo?(pbrosset)
Attachment #8807341 - Attachment is obsolete: true
Flags: needinfo?(pbrosset)
Thank you Deepjyoti for your work on this. I'll mark this as "checkin-needed" so someone lands the patch.
Let me know if you're looking for other bugs to work on.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc61f761ef70
Updated addNode function so that on adding a new now the parent node remains selected. r=pbrosset
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cc61f761ef70
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I have reproduced this bug with Nightly 52.0a1 (2016-10-21) (64-bit) on Windows 7, 64 Bit !

This bug's fix is verified with latest Nightly 

Build   ID  20161126030207
User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
 
[bugday-20161123]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.