1217311 - Deleting attributes with the keyboard should move the focus to the right attribute

Status: NEW

(DevTools :: Inspector, defect, P3)

Description

[Patrick Brosset <:pbro>]

With bug 1203303, users can now delete attributes on nodes in the markup-view when they are focused by pressing either delete or backspace.

This bug is about moving the focus to the right position when this happens.

- When backspace is used: the focus should move to the previous attribute, and if there is none, to the tagname.
- When delete is used: the focus should move to the next attribute, and if there is none, to the newattr field.

Comment 1

[Grisha Pushkov]

Attachment: 1217311-focus-after-attribute-removal.patch

I have used moveFocus function which was implemented in bug 1090871. It fits handy here.
In tests there is new property called deleteKey (to trigger specific key) and checks for focused element.

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8683846 [details] [diff] [review]
1217311-focus-after-attribute-removal.patch

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

::: devtools/client/markupview/markup-view.js
@@ +685,5 @@
> +
> +      if (moveBackward) {
> +        moveFocus(this.doc, Ci.nsIFocusManager.MOVEFOCUS_BACKWARD);
> +      } else {
> +        moveFocus(this.doc, Ci.nsIFocusManager.MOVEFOCUS_FORWARD);

I'm not sure calling moveFocus from the inplace-editor module is the best choice here.
The inplace-editor module is expected to only contain things related to editing text. I find it a bit weird that it would also expose a focus management helper that you'd use here.
Especially that because you're not passing true as the last parameter, this ends up just calling 1 line:

focusManager.moveFocus(doc.defaultView, null, direction, 0);

So I'm going to suggest that you don't export moveFocus in inplace-editor and don't use it here, but instead just call the focusManager.moveFocus.

Comment 3

[Grisha Pushkov]

moveFocus was introduced in bug 1090871.
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1090871&attachment=8683217
In markup-view.js on 2997 line, it is used with editMode.

Where we can place this helper if not in inplace-editor.js?

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Grisha Pushkov from comment #3)
> moveFocus was introduced in bug 1090871.
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=1090871&attachment=8683217
> In markup-view.js on 2997 line, it is used with editMode.
> 
> Where we can place this helper if not in inplace-editor.js?
Ah, I have not yet reviewed bug 1090871. Maybe it would have provided me with more context then.
What I'm saying is that in attachment 8683846 [details] [diff] [review], I do not think it is worth it to even use this helper.
You could just call focusManager.moveFocus() instead, because it's just one line, and to me, using something that belongs to inplace editing to perform something that has nothing to do with editing text feels odd.
But again, I haven't reviewed this other bug yet. I'll probably do this tomorrow.

Comment 5

[Grisha Pushkov]

Yeah, I understand that introducing helper only for this bug is worthless. And with bug 1090871 in mind I'm fully convinced that we need it :)
Going to wait your review for bug 1090871 and after that we will decide how to do it better.

Using focusManager.moveFocus() in one place and moveFocus in other is confusing. And export moveFocus in inplace-editor.js is confusing too. That's why I think we will move this helper to other place.

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Filter on CLIMBING SHOES