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

Status: RESOLVED FIXED

(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)]

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] ❌ On leave until 2027-01-04]

(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] ❌ On leave until 2027-01-04]

(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.

Comment 14

[Dão Gottwald [:dao]]

Attachment: Bug 1877510 - Let Enter in the edit bookmark folder tree save the bookmark. r=mak,places-reviewers

Right and Left arrows still expand and collapse folders. The Bookmark Properties dialog shares the tree, so Enter there now accepts the dialog too.

Comment 15

[Dão Gottwald [:dao]]

Attachment: Bug 1877510 - Add an access key to the bookmark panel's Save button. r=mak,fluent-reviewers,places-reviewers,flod

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/02a0ffa2653f
https://hg.mozilla.org/integration/autoland/rev/1fd3b1a6fa33
Let Enter in the edit bookmark folder tree save the bookmark. r=mak,places-reviewers
https://github.com/mozilla-firefox/firefox/commit/ad5b15f10088
https://hg.mozilla.org/integration/autoland/rev/b04e8d448dcc
Add an access key to the bookmark panel's Save button. r=mak,fluent-reviewers,places-reviewers,flod

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6674cdd6715f
https://hg.mozilla.org/integration/autoland/rev/3348b35c8948
Revert "Bug 1877510 - Add an access key to the bookmark panel's Save button. r=mak,fluent-reviewers,places-reviewers,flod" for causing failures at browser_bookmark_popup.js.

Comment 18

[Atila Butkovits]

Backed out for causing failures at browser_bookmark_popup.js.

Backout link: https://hg.mozilla.org/integration/autoland/rev/3348b35c8948

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=b04e8d448dccc60cea90d2f38a4f0b3f9636ada0&selectedTaskRun=MxDb6e0XQh6NBBFOo-XsYw.0

Failure logs:
https://treeherder.mozilla.org/logviewer?job_id=598067158&repo=autoland&task=MxDb6e0XQh6NBBFOo-XsYw.0&lineNumber=56401
https://treeherder.mozilla.org/logviewer?job_id=598067060&repo=autoland&task=PcOIxNz2TlaHdDMUMjtavg.0&lineNumber=7524
https://treeherder.mozilla.org/logviewer?job_id=598066262&repo=autoland&task=bknb-9tTQX2NOSHZNsCeyA.0&lineNumber=15173

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/679598de6dfc
https://hg.mozilla.org/integration/autoland/rev/4476766bdf1f
Let Enter in the edit bookmark folder tree save the bookmark. r=mak,places-reviewers
https://github.com/mozilla-firefox/firefox/commit/d97c7730c880
https://hg.mozilla.org/integration/autoland/rev/c6eada363031
Add an access key to the bookmark panel's Save button. r=mak,fluent-reviewers,places-reviewers,flod

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/99dfbd65f567
https://hg.mozilla.org/integration/autoland/rev/e57a5eb02f67
Revert "Bug 1877510 - Add an access key to the bookmark panel's Save button. r=mak,fluent-reviewers,places-reviewers,flod" for causing bc failures @ browser_bookmark_popup.js

Comment 21

[amarc]

Backed out for causing bc failures

• Backout link
• Push with failures
• Failure Log

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/31b3fcd53887
https://hg.mozilla.org/integration/autoland/rev/d304c8040802
Let Enter in the edit bookmark folder tree save the bookmark. r=mak,places-reviewers
https://github.com/mozilla-firefox/firefox/commit/6f9e2ef0e91a
https://hg.mozilla.org/integration/autoland/rev/4a9c5e783008
Add an access key to the bookmark panel's Save button. r=mak,fluent-reviewers,places-reviewers,flod

Comment 23

[Silaghi Andreea]

https://hg.mozilla.org/mozilla-central/rev/d304c8040802
https://hg.mozilla.org/mozilla-central/rev/4a9c5e783008

Comment 24

[Donal Meehan [:dmeehan]]

Did you want to nominate this for the Fx160 relnotes? If so, set the relnote-firefox flag to "?"
https://wiki.mozilla.org/Release_Management/Release_Notes_Nomination

Possible wording:

The Save button in the bookmark panel now has a keyboard access key, and pressing Enter in the folder list saves the bookmark.

Comment 25

[Dão Gottwald [:dao]]

This seems minor, I wouldn't add a relnote.