1203303 - When an attribute is focused, Delete should only delete the attribute, not the entire element

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Jesse Ruderman]

1. Open Inspector (DOM tree view).
1. Click an attribute on an HTML element.
2. Press the Backspace (Delete) button.

Result: element is deleted (!!)
Expected: attribute is deleted

In contrast, pressing Enter does what I expect (edits only the attribute). CF bug 994555.

Comment 1

[Patrick Brosset <:pbro>]

Good point. I think this was reported before but can't find the bug.

That's because the markup-view has a listener for keydown events and uses it to delete selected nodes when delete is pressed, no matter what is currently focused:

https://dxr.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f/browser/devtools/markupview/markup-view.js#580

Comment 2

[Jesse Ruderman]

Although maybe it's too easy to accidentally select an attribute when you mean to select an element? Hmm.

Comment 3

[Patrick Brosset <:pbro>]

Bug 994555 adds a handy method to MarkupContainer objects that we could use to delete the attribute if it is focused.
One way to implement this would be:
- update the _onKeyDown handler that's already defined in MarkupView.prototype,
- instead of unconditionally deleting the node, first check if one of the attributes is focused,
- if one is focused, then call container.removeAttribute(name) instead

Comment 4

[Patrick Brosset <:pbro>]

Attachment: Bug_1203303_-_If_an_attribute_is_focused_then_DEL_.diff

Work in progress, needs a test.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: Bug_1203303_-_When_attribute_is_focused_DEL_remove.diff

This seems to work, the code is fairly simple and there's a new test. Brian what do you think?
(for info, I originally wanted to add the new test just as a test case into an existing test file, so started to refactor it a little (also to avoid using CPOWs), but ended up adding a new file, and keeping the refactor).

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8677061 [details] [diff] [review]
Bug_1203303_-_When_attribute_is_focused_DEL_remove.diff

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

Looks generally good to me, see comments

::: devtools/client/markupview/markup-view.js
@@ +576,5 @@
>            }
>          }
>          break;
>        case Ci.nsIDOMKeyEvent.DOM_VK_DELETE:
> +        this.deleteNodeOrAttribute(this._selectedContainer.node);

I think you shouldn't be passing this._selectedContainer.node into these calls - the one and only param to deleteNodeOrAttribute is a bool.  I'd be surprised if all tests pass in this case since it's always doing 'moveBackward'.  If they do we should expand the test coverage to make sure it catches that.

@@ +689,5 @@
> +                           : null;
> +    if (focusedAttribute) {
> +      // The focused attribute might not be in the current selected container.
> +      let container = focusedAttribute.closest("li.child").container;
> +      container.removeAttribute(focusedAttribute.dataset.attr);

Ideally we would move the focus (according to moveBackward), such that if I delete the last attribute on a new node then the new attr element will become focused.  So then I could immediately press <ENTER> and re-add an attribute.  That's fine to be a follow up if that's going to be a lot of work

Comment 7

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> Comment on attachment 8677061 [details] [diff] [review]
> Bug_1203303_-_When_attribute_is_focused_DEL_remove.diff
> 
> Review of attachment 8677061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks generally good to me, see comments
> 
> ::: devtools/client/markupview/markup-view.js
> @@ +576,5 @@
> >            }
> >          }
> >          break;
> >        case Ci.nsIDOMKeyEvent.DOM_VK_DELETE:
> > +        this.deleteNodeOrAttribute(this._selectedContainer.node);
> 
> I think you shouldn't be passing this._selectedContainer.node into these
> calls - the one and only param to deleteNodeOrAttribute is a bool.  I'd be
> surprised if all tests pass in this case since it's always doing
> 'moveBackward'.  If they do we should expand the test coverage to make sure
> it catches that.
Thanks, that was a dumb mistake on my side. This argument isn't needed at all indeed, and tests do fail: https://treeherder.mozilla.org/#/jobs?repo=try&revision=32cbfe68bc2e
I'll fix this.

> @@ +689,5 @@
> > +                           : null;
> > +    if (focusedAttribute) {
> > +      // The focused attribute might not be in the current selected container.
> > +      let container = focusedAttribute.closest("li.child").container;
> > +      container.removeAttribute(focusedAttribute.dataset.attr);
> 
> Ideally we would move the focus (according to moveBackward), such that if I
> delete the last attribute on a new node then the new attr element will
> become focused.  So then I could immediately press <ENTER> and re-add an
> attribute.  That's fine to be a follow up if that's going to be a lot of work
Yeah I thought about that but thought that this extended a bit outside the scope of this polish bug, so yeah, I'll file a bug for it.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: Bug_1203303_-_When_attribute_is_focused_DEL_remove.diff

Comment 9

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd3bd8a6d83f

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/180eb2f441d6

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/180eb2f441d6