Closed Bug 1722781 Opened 4 months ago Closed 2 months ago

On Bookmarks Toolbar, "Remove" context-menu-item uses different accesskey for the two items we put there ("Import Bookmarks" & "Get Involved")

Categories

(Firefox :: Bookmarks & History, defect)

defect

Tracking

()

VERIFIED WONTFIX

People

(Reporter: dholbert, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [proton-context-menus])

Attachments

(2 files)

STR (in a fresh profile):

  1. Open a new tab, to make the bookmarks toolbar show up.

...now suppose you want to remove both of the default entries from the toolbar.

  1. Right-click the first entry ("Import Bookmarks") and note the underlined hotkey for its "Remove from Toolbar" option.

  2. Right-click the second entry ("Get Involved") and note the underlined hotkey for its "Remove Bookmark" option.

ACTUAL RESULTS:
The first one ("import bookmarks" button) uses "R" as its hotkey for Remove, whereas the second ("Get Involved", an actual bookmark) one uses "e" as its hotkey for Remove.

EXPECTED RESULTS:
Should be consistent (probably both "R"?) for this Remove action. Also, there is no other item in the list that uses "R" as its hotkey, so there's no obvious reason why we'd be avoiding using it here.

It looks like the "e" accesskey usage here came from this commit for bug 1692668:
https://hg.mozilla.org/integration/autoland/rev/9a043bb287a5#l16.98

Gijs, do you recall why we used "e" instead of "R" for Remove here, and could/should we consider changing it for consistency (at least for this case of the button + bookmarks on the bookmarks toolbar which use different removal accesskeys)?

Flags: needinfo?(gijskruitbosch+bugs)
Summary: On Bookmarks Toolbar, "Remove" context-menu-item uses different hotkey for the two items we put there ("Import Bookmarks" & "Get Involved") → On Bookmarks Toolbar, "Remove" context-menu-item uses different accesskey for the two items we put there ("Import Bookmarks" & "Get Involved")
Component: General → Bookmarks & History
Depends on: 1692668

(In reply to Daniel Holbert [:dholbert] from comment #1)

It looks like the "e" accesskey usage here came from this commit for bug 1692668:
https://hg.mozilla.org/integration/autoland/rev/9a043bb287a5#l16.98

Gijs, do you recall why we used "e" instead of "R" for Remove here, and could/should we consider changing it for consistency (at least for this case of the button + bookmarks on the bookmarks toolbar which use different removal accesskeys)?

r is used for Sort already, in the same menu, so you can't just use "R" without more work.. If you want to then change the access key for Sort by Name, you have to check all the other menus that that entry appears in (not just the bookmarks toolbar, also check the bookmarks menu, the sidebar, and the places organizer), and it all goes downhill from there. It's like a bizarre game of musical chairs except there is no music to make it fun, instead everyone yells at you no matter what you do (cf. bug 1716223, bug 1701324 and dupes).

The follow-ups that Marco fixed might have made this easier - perhaps we can "just" change the Remove item and the Sort item and that's enough? I don't know off-hand.

Flags: needinfo?(gijskruitbosch+bugs)

All the entries in the menu should still be grouped in placesContextMenu.inc.xhtml, I don't think my patch made it harder or simpler, it would still require to check for conflicts. And of course we'd still have no control over other locales to impose them to use the same letter.
This is an issue causes by willing to change the generic Delete to more specific forms of removals. See also Bug 1714478 where now we have some menus using Delete and some using Remove, and that is inconsistent. And Bug 1721500
If there's no intent to revert the change or move to Delete, then we'll have to do that conflict check and change Sort to be able to use the R.

Whiteboard: [proton-context-menus]

(In reply to :Gijs (he/him) from comment #2)

(In reply to Daniel Holbert [:dholbert] from comment #1)

It looks like the "e" accesskey usage here came from this commit for bug 1692668:
https://hg.mozilla.org/integration/autoland/rev/9a043bb287a5#l16.98

Gijs, do you recall why we used "e" instead of "R" for Remove here, and could/should we consider changing it for consistency (at least for this case of the button + bookmarks on the bookmarks toolbar which use different removal accesskeys)?

r is used for Sort already, in the same menu, so you can't just use "R" without more work..

Are you sure? I only see "Sort" in "Sort by name", which is in context-menu for bookmarks folders (not individual bookmarks which is the thing involved in this bug's STR).

e.g. if I right-click a folder in the bookmarks/places organizer, I do see So(r)t by name; and in that context-menu, "Remove" has yet-another-accesskey, "m". So there's no strong consistency or collision argument for using "e" as the accesskey for removal action in the individual-bookmark contextmenu, AFAICT...

Attaching two screenshots to demonstrate my previous comment. It seems you're right that bookmarks folders can't use "r" as their accesskey for the "remove" action (for removing the entire folder); but individual bookmarks conceivably could, since "r" is unused there; and we don't have any consistency that we'd be losing in making that change (i.e. we don't consistently use "e" for remove, as is shown in this screenshot where we use "m")

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Daniel Holbert [:dholbert] from comment #6)

Created attachment 9234490 [details]
screenshot of context-menu for a folder, with Re(m)ove and with So(r)t using "r"

Attaching two screenshots to demonstrate my previous comment. It seems you're right that bookmarks folders can't use "r" as their accesskey for the "remove" action (for removing the entire folder); but individual bookmarks conceivably could, since "r" is unused there; and we don't have any consistency that we'd be losing in making that change (i.e. we don't consistently use "e" for remove, as is shown in this screenshot where we use "m")

I think the distinction you make between folders and bookmarks is correct. I don't know if that means that the individual bookmarks case is truly "safe" to change to R, off-hand. As Marco said in comment #3, we'd need to check https://searchfox.org/mozilla-central/rev/4f05a46731c1f7f111ec7a41ce38a34594aa0d37/browser/components/places/content/placesContextMenu.inc.xhtml for conflicts. If we want to change it, someone would need to audit that the same access key is not used for any other entries that can appear at the same time. I don't have time to do that right now, but it's worth noting that the same items can appear/disappear in different places (different bookmarks views, different non-bookmarks views (though that likely doesn't matter here), and different selection contexts), so it's not entirely straightforward. Note for instance how selecting both a folder and a bookmark in the bookmarks window gets you "delete" as a context menu, not either of the other (Remove bookmark/Remove folder) items...

Flags: needinfo?(gijskruitbosch+bugs)

Bug 1714478 has now reverted the Remove bookmark/folder to use Delete. "Remove from toolbar" still remains, but I think that is appropriate because it isn't a bookmark.

I think keeping the access key for "Remove from toolbar" as "e" makes sense for now, given the sort option.

Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → WONTFIX

Thanks - that makes sense.

The core issue I was reporting here was the inconsistent accesskey for the same word on the context menus for two side-by-side things -- and Bug 1714478 has addressed this by making it no longer the same word.

Status: RESOLVED → VERIFIED

(Given comment 8 and comment 9, I'd be happy to consider this "fixed" or "worksforme" via bug 1714478, rather than wontfix; but I won't bikeshed the resolution field too much. :) )

You need to log in before you can comment on or make changes to this bug.