1877510 - Bookmark creation / editing popover dialog does not have a keyboard shortcut mnemonic for the Save button

Status: NEW

(Firefox :: Bookmarks & History, enhancement, P3)

Description

[Jeff Fortin]

Attachment: Screenshot from 2024-01-30 13-47-00.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:121.0) Gecko/20100101 Firefox/121.0

Steps to reproduce:

1. Ctrl+D to open the bookmark creation popover

Actual results:

The "Show editor when saving" checkbox and the "Cancel" widgets have accelerators (S and C, respectively), but not the Save button.

Expected results:

The Save button should probably have an accelerator for direct access (ex: S for Alt+S) much more than the "Show editor when saving" checkbox, which is not something expected to be used frequently (or even more than once in the user's life; I'm not sure why it is even present in that popover dialog).

Comment 1

[Jeff Fortin]

Attachment: Screenshot showing the "Cancel" button and its accelerator (vs the "Remove bookmark" button)

Comment 2

[Jeff Fortin]

Correction: above I said "accelerator", but I actually meant "mnemonic", to use accurate terminology.

The 1st screenshot also showed the "Remove bookmark" button, which has the "R" mnemonic. I have attached a second screenshot that also shows the Cancel button with its mnemonic.

Comment 3

[Ina Popescu, Desktop QA]

Thank you for submitting this enhancement. Setting the component and waiting for the developer's opinion about it.
If this is not the correct component, please feel free to change it to a more appropriate one.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

This is a UI issue, there is no access key definition.
https://searchfox.org/mozilla-central/rev/54c9b4896fdc1e858cd4942f306d877f1f3d195e/browser/locales/en-US/browser/browser.ftl#357-358

Comment 5

[Marco Bonardo [:mak]]

I suspect the reason is that being the default operation one can just use Enter, so the accelerator would be redundant.
Is this just a discoverability concern?

Comment 6

[Jeff Fortin]

I suspect the reason is that being the default operation one can just use Enter, so the accelerator would be redundant.
Is this just a discoverability concern?

Not just a discoverability concern, no, as the Enter key cannot be used to trigger the "Save" action when inside the expanded folders view to file the bookmark into at the same time you are creating/saving it.

More specific steps for this scenario:

1. Ctrl+D
2. Tab
3. Tab again
4. Spacebar to expand
5. Up/Down or typeahead to specify the folder to file into

In that situation, you can activate the "Cancel" action with Esc or Alt+C, but you cannot directly activate the "Save" action unless you press the Tab key 4 more times. It would be more valuable to provide the mnemonic for Save on equal footing to Cancel.

Comment 7

[Marco Bonardo [:mak]]

Anna, from an a11y point of view, what would be the best action here?

Comment 8

[Anna Yeddi [:ayeddi]]

(In reply to Marco Bonardo [:mak] from comment #7)

Anna, from an a11y point of view, what would be the best action here?

Thank you for the ping, Marco!

I agree that having a shortcut assigned to the main action Save (a mnemonic) would be beneficial for accessibility, since, as Jeff described in #c6, Enter does not work from within the tree. While I agree that the s accel key is not as logical to be used for Show checkbox as it would be for Save, if we were to change it, we need to be mindful of users' muscle memory - this would affect any keyboard user, but especially ones with cognitive difficulties and neurodiverse users who are likely to need more time to relearn the pattern for a shortcut.

Maybe, this could be resolved with allowing Enter on the tree view to perform a default/primary action, in this case, to Save the bookmark?

Comment 9

[Jeff Fortin]

Maybe, this could be resolved with allowing Enter on the tree view to perform a default/primary action, in this case, to Save the bookmark?

It seems to me that this key is already used inside the treeview to allow toggling folding/unfolding (i.e. expanding) a folder branch containing subfolders, so it would conflict with that, unless you reassign it to Spacebar, but then that's another muscle memory change and you'd also have to be careful to not hijack spacebar while outside the treeview (ex: typing whitespaces in a bookmark's name field)...

On the other hand, I do not see why the "Show editor when saving" checkbox has a mnemonic, for the reasons I mentioned earlier; I don't think it is something people toggle more than once or twice in their lives (I don't even see why it should be in that popover dialog, as it is essentially a preference, and presumably a very niche one).

Comment 10

[Anna Yeddi [:ayeddi]]

(In reply to Jeff Fortin from comment #9)

It seems to me that this key is already used inside the treeview to allow toggling folding/unfolding (i.e. expanding) a folder branch containing subfolders, so it would conflict with that, unless you reassign it to Spacebar, but then that's another muscle memory change and you'd also have to be careful to not hijack spacebar while outside the treeview (ex: typing whitespaces in a bookmark's name field)...

This is actually not an expected keyboard behavior for a Tree View, per the WAI ARIA Design Pattern keyboard interaction:

Enter: activates a node, i.e., performs its default action. For parent nodes, one possible default action is to open or close the node. In single-select trees where selection does not follow focus (see note below), the default action is typically to select the focused node.

:Jamie, do you think this pattern is something that we’d want to pursue (maybe, on the component level) or is the current behavior where Enter opens/closes the tree node is more delightful for keyboard-only users?

Comment 11

[James Teh [:Jamie]]

(In reply to Anna Yeddi [:ayeddi] from comment #10)

This is actually not an expected keyboard behavior for a Tree View, per the WAI ARIA Design Pattern keyboard interaction:

Enter: activates a node, i.e., performs its default action. For parent nodes, one possible default action is to open or close the node. In single-select trees where selection does not follow focus (see note below), the default action is typically to select the focused node.

:Jamie, do you think this pattern is something that we’d want to pursue (maybe, on the component level)

Strictly speaking, the pattern does allow for our current behaviour:

For parent nodes, one possible default action is to open or close the node.

If you're on a parent node, enter does indeed open/close the node. If you're on a leaf node, it doesn't, but I don't know if we'd want enter to behave inconsistently either.

or is the current behavior where Enter opens/closes the tree node is more delightful for keyboard-only users?

1. It's standard in all tree views I know of for right arrow to expand a node and left arrow to collapse it. So, there's already a keyboard accessible way to do that and I don't see why we need two ways.
2. Despite the ARIA Design Pattern's suggestion that enter might be acceptable to open or close a node, I've never seen an implementation which does this other than our bookmark tree here.
3. Even if we did want to keep the enter key behaviour on this tree when it's not inside a dialog (e.g. i the side bar or Library window), I'd argue that the intent of pressing enter is far more likely to be "affirmatively dismiss the dialog" than it is to be "expand this tree node" in this case. Certainly, I myself have banged enter multiple times when in this dialog while focused in the tree and been perplexed/frustrated that it didn't work.

With all of that context out of the way, I think we should:

1. Preferred: Make enter affirmatively dismiss the dialog (save the bookmark), even if the tree is focused. Alternatively:
2. If that's too hard or not possible for some reason, give the Save button an access key. As has been discussed, "Show editor when saving" already has "s". Regardless of how often this is used, it's been there for a long time and I agree with Anna that we should avoid breaking any existing user that might use this for whatever reason. We could go with "a" for save. Not ideal, but better than nothing.

Comment 12

[Jeff Fortin]

Have you considered option 3: removing that "Show editor when saving" toggle from that UI and relegating it to only its existing about:config boolean setting (browser.bookmarks.editDialog.showForNewBookmarks), or is that completely out of question? That this toggle has been present in the UI for a long time does not really convince me that it is needed from a design/UX perspective. It seems to me like a niche footgun, because if you toggle it off, you are then never prompted which folder to file a newly created bookmark into, which I believe is against the best interest of a majority of users. Maybe worth asking the UX team (if they are not present here) to review whether this UI element remains relevant today.

That said, even if the "Show editor when saving" toggle remains there with its "S" mnemonic, and even if Enter somehow makes the dialog's "Save" action activate while the treeview is focused, I believe it could be beneficial to add some mnemonic to the Save button (I don't care much if it's S, A, V, or E…) nonetheless, for discoverability, consistency, and so that it can work from anywhere (even if you have some other widget focused, like the "V" expander button above the treeview). It seems to me that doing that would not be incompatible with the rest, no?

Comment 13

[James Teh [:Jamie]]

Sure, it's an option, but it's an option which requires a lot more UX consideration beyond the scope of this bug. My concern is that we let the perfect be the enemy of the good here and this bug just never gets fixed, remaining an accessibility papercut forever more. That's why I suggested two options that address the accessibility concern without changing the core UX of the dialog.

Anyway, this isn't my decision to make - I'm just providing accessibility guidance - so I'll let the owning team decide how to handle this.