Steve Melia Assignee
	Description • 8 years ago

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0 Iceweasel/38.8.0
Build ID: 20160426225641

Steps to reproduce:

1. Enter the URI "data:text/html,<div>1<div>2</div>3</div>"
2. Open the inspector. select the text node "3" and press the "delete" key
3. Reopen the URI
4. Right click the text node "3" in the inspector to open the context menu 



Actual results:

The Delete menu item is disabled.


Expected results:

Behaviour should be consistent - It should be possible to delete a text node with both the keyboard and mouse, or with neither.

	Nicolas Chevobbe [:nchevobbe]
	Comment 1 • 8 years ago

Here is where the disabled state of the context menu is handled : https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector-panel.js#751 .

I don't know if we should allow to delete the node from the context menu, or if we should prevent to delete it from the keyboard shortcut though.

	Steve Melia Assignee
	Comment 2 • 7 years ago

What should be the correct behaviour here? I wouldn't mind picking this one up!

	Patrick Brosset <:pbro>
	Comment 3 • 7 years ago

Very cool, thanks for your interest. The correct behavior should be that the "delete" item to be enabled even for text nodes.

I believe this needs to be fixed in this part of the code: https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/devtools/client/inspector/inspector.js#1275-1276

And, once fixed, we should also add a test case to make sure this always works: https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/devtools/client/inspector/test/browser_inspector_menu-06-other.js#60-73

We have docs here, to get started: http://docs.firefox-dev.tools/

Feel free to use the needinfo? flag at the bottom to ask for help.

	Steve Melia Assignee
	Comment 4 • 7 years ago

See attached for patch - hope this is all formatted correctly etc. & thanks in advance for a review.

I wanted to use MozReview & do a try push etc. but I think some of my credentials have expired; I have opened a bug for this.

	Patrick Brosset <:pbro>
	Comment 5 • 7 years ago

Comment on attachment 8931900 [details] [diff] [review]
1272969.patch

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

::: devtools/client/inspector/inspector.js
@@ +1278,4 @@
>        id: "node-menu-delete",
>        label: INSPECTOR_L10N.getStr("inspectorHTMLDelete.label"),
>        accesskey: INSPECTOR_L10N.getStr("inspectorHTMLDelete.accesskey"),
> +      disabled: !isEditableElement && !this.selection.isTextNode(),

This works well for me. But I'm wondering if other node types should be included too. And therefore if instead of a whitelist approach like here, we shouldn't do a black list.

In fact if you look at the top of the deleteNode function in devtools/client/inspector/markup/markup.js file (this is what handles the delete key shortcut), we only stop deleting if the node isDocumentElement or DOCUMENT_TYPE_NODE or isAnonymous.

So that means text nodes can be deleted, as well as comment nodes for instance. Maybe there are other types of nodes too.

Here's an idea: create a isDeletable(node) method in the Inspector class and call it here, and also call it from the MarkupView class (using this.inspector.isDeletable) in the deleteNode method.

This way both places share the same definition of what is and what isn't deletable.

	Patrick Brosset <:pbro>
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8933636 [details]
Bug 1272969: Delete item is enabled on context menu for text nodes in inspector.

https://reviewboard.mozilla.org/r/204586/#review210214

The code changes look nice. The test too. I noticed a couple bugs though, that will need to be addressed before we land this. But after that's done, we should be good to go.

::: devtools/client/inspector/inspector.js:1281
(Diff revision 1)
>      }));
>      menu.append(new MenuItem({
>        id: "node-menu-delete",
>        label: INSPECTOR_L10N.getStr("inspectorHTMLDelete.label"),
>        accesskey: INSPECTOR_L10N.getStr("inspectorHTMLDelete.accesskey"),
> -      disabled: !isEditableElement,
> +      disabled: !this.isDeletable(this.selection),

You are passing a `Selection` object here, not really the node itself.
You need to do this instead:

`disabled: !this.isDeletable(this.selection.nodeFront),`

::: devtools/client/inspector/markup/markup.js:870
(Diff revision 1)
>     *         The node to remove.
>     * @param  {Boolean} moveBackward
>     *         If set to true, focus the previous sibling, otherwise the next one.
>     */
>    deleteNode: function (node, moveBackward) {
> -    if (node.isDocumentElement ||
> +    if (this.inspector.isDeletable(node)) {

This should be the opposite. We want to return early only if the node is *not* deletable:

```
if (!this.inspector.isDeletable(node)) {
  return;
}
```

	Steve Melia Assignee
	Comment 8 • 7 years ago

Ah sorry I was so focussed on reviewboard I forgot to actually check it worked... :(

I was going to do a try push with `-b do -p linux,linux64,macosx64,win32 -u mochitests -t none` but it hung with a "waiting for lock" message and then I got cold feet thinking it was a bit wasteful to run the whole suite for just one change - I think you guys batch up small commits and run them in one try?

I will fix, squash, and repost to reviewboard; I did also think maybe the `isDeletable` method could be a member of `NodeFront` but I am not sure if there is some separation of responsibility issue there?

	Patrick Brosset <:pbro>
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8933636 [details]
Bug 1272969: Delete item is enabled on context menu for text nodes in inspector.

https://reviewboard.mozilla.org/r/204586/#review210604

Looks good now thanks!
Yeah let's avoid moving the isDeletable function to NodeFront. This class is just a thin wrapper around a protocol object. I'd like to keep it as small as possible.
I pushed this commit to TRY, let's see how it does there: https://treeherder.mozilla.org/#/jobs?repo=try&revision=535da93f7e20297b7413da38dfc92afe6776fc57

	Patrick Brosset <:pbro>
	Comment 11 • 7 years ago

Looks like we missed a spot. This test now fails: browser_inspector_menu-01-sensitivity.js
Do you mind looking at the test failure log, running this test locally and finding a fix for it?

	Steve Melia Assignee
	Comment 13 • 7 years ago

This was a discrepancy between MarkupView and Inspector - I think the correct behaviour is that the document node should not be deletable; so i've adjusted the test; also changed the logging slightly to indicate which test case.

I have pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4098c4cab771e3b5c2d3fcb05557e0fe92accbdc&selectedJob=151073259 but it does not seem to have completed; i'm not sure why. There is a failure but it seems unrelated.

	Patrick Brosset <:pbro>
	Comment 14 • 7 years ago

Awesome, thank you Steve. I'll land this!

	Pulsebot
	Comment 15 • 7 years ago

Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f5281c6b186
Delete item is enabled on context menu for text nodes in inspector. r=pbro

	Natalia Csoregi [:nataliaCs]
	Comment 16 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1f5281c6b186

	Mohammad Maruf Rahman
	Comment 17 • 7 years ago

I have reproduced this bug with Nightly 49.0a1 (2016-05-17) on Windows 10, 64 Bit!

This bug's fix is verified with latest Nightly!

Build ID   : 20180120100135
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0