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

RESOLVED FIXED in Firefox 44

Status

P2
normal
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: jruderman, Assigned: pbro)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 44
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox44 fixed)

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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
Whiteboard: [polish-backlog]
(Reporter)

Comment 2

3 years ago
Although maybe it's too easy to accidentally select an attribute when you mean to select an element? Hmm.
Priority: -- → P2
(Assignee)

Updated

3 years ago
Whiteboard: [polish-backlog] → [polish-backlog][difficulty=easy]
(Assignee)

Comment 3

3 years ago
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
Depends on: 994555
(Assignee)

Updated

3 years ago
Blocks: 1210159
(Assignee)

Comment 4

3 years ago
Created attachment 8676304 [details] [diff] [review]
Bug_1203303_-_If_an_attribute_is_focused_then_DEL_.diff

Work in progress, needs a test.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
(Assignee)

Comment 5

3 years ago
Created attachment 8677061 [details] [diff] [review]
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).
Attachment #8676304 - Attachment is obsolete: true
Attachment #8677061 - Flags: review?(bgrinstead)
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
Attachment #8677061 - Flags: review?(bgrinstead)
(Assignee)

Comment 7

3 years ago
(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.
(Assignee)

Updated

3 years ago
Blocks: 1217311
(Assignee)

Comment 8

3 years ago
Created attachment 8677461 [details] [diff] [review]
Bug_1203303_-_When_attribute_is_focused_DEL_remove.diff
Attachment #8677061 - Attachment is obsolete: true
Attachment #8677461 - Flags: review?(bgrinstead)
Attachment #8677461 - Flags: review?(bgrinstead) → review+

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/180eb2f441d6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44

Updated

5 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.