1861674 - Issue around selector/property name/property value inputs "commit" behavior + keyboard navigation

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[Nicolas Chevobbe [:nchevobbe]]

We have a few bugs filed about element not being focusable in the rule view:

• checkbox toggle (Bug 1844055)
• selector highlighter button (Bug 1844053)
• stylesheet location (Bug 1844054)
• "filter rules container this property" button (Bug 1844058)

for some of those, the fix is easy: use the right HTML element and remove any tabindex=-1 attributes that we may have.

The issue is that doing so is making tests to fail because of the expected behaviour of the rule view.

At the moment, when you click on a selector, a property name or a property value, we display a text input to modify the underlying text. Then when the user hits Enter or (Shift+)Tab, we "move" to the next (previous for Shift+Tab) "editable" property which we also directly turn into a text input. This allows to quickly edit multiple properties in a rule without leaving the keyboard (which is good).
Hitting the Escape key makes the input goes away and cancel the editing that was happening.

But now, if we make, let's say the property-toggle checkbox focusable, hitting Enter or Tab after modifying a selector or a property value will focus the next checkbox (if there's one), instead of the expected next property name .

We can probably fix this in the code, but we should also think of a way to be able to opt-out of the "editing" mode, which is only possible with Escape key at the moment, but at the expense of losing your modification.
We also need to think of how this affect the autocomplete popup that we show (at the moment, hitting Enter/Tab when it's displayed will autocomplete the value and proceed into "commiting" it, moving the focus to the next text input)

Maybe we should make it so that only (Shift+)Tab should move you to the (previous) next text input, but Enter will simply commit the change and focus the underlying element, so you can then Tab to other type of focusable elements (e.g. property-toggle checkbox, selector highlighter, …).
I'm not 100% positive about this approach, maybe there should be some sort of visual feedback so the user would know what would happen when they press a specific key.

We have to be careful in what we're going to do as this behavior was implemented a long time ago and people might rely on it. To make it worse, Chrome and Safari are doing the exact same thing, so this is a consistent pattern in the industry (even if this means they're also not accessible, and this would give us an advantage here)

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1861674 - [devtools] Add InplaceEditor options to move focus to next editor when commiting. r=#devtools-reviewers.

By default, when hitting Tab (and Enter if stopOnReturn is false), the focus
is moved to the next (previous if Shift+Tab) focusable element.
If such element is also an inplace editor, then we trigger the editing.
Here, we're adding options so the focus will try to move to the next inplace
editor, even if there are other focusable items before it.
When the option is set to true, we require a mandatory selector that matches
an element onto which we are searching for inplace editors to focus, so we
don't move the focus indefinitely.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1861674 - [devtools] Change focus behavior for inplace editor in the Rules view. r=#devtools-reviewers.

For the selector, property name and property value editors, we don't want the
Enter key to trigger editing the next inplace editor.
We only want to close the current editor and focus the associated element so the
user can use regular keyboard navigation.
We still want the Tab key to trigger "editor navigation", so we pass the appropriate
options to editableField.

Existing test that were not specifically asserting the moving focus behaviour
are updated. If they were using Enter only to commit and move to the next input,
we update them to use Tab instead.

Depends on D193194

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a7356ea4281
[devtools] Add InplaceEditor options to move focus to next editor when commiting. r=devtools-reviewers,ochameau.
https://hg.mozilla.org/integration/autoland/rev/9da366771c58
[devtools] Change focus behavior for inplace editor in the Rules view. r=devtools-reviewers,ochameau.

Comment 4

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/4a7356ea4281
https://hg.mozilla.org/mozilla-central/rev/9da366771c58