947344 - Australis - Show shortcuts for all widgets

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Alice0775 White]

Attachment: screenshot

Add shortcuts label "Ctrl+B" for "View Bookmarks Sidebar" in Bookmarks Widget panel

Comment 1

[Justin Dolske [:Dolske]]

I vaguely remember discussion on not wanting to show shortcuts in the panel views, but it does at least seem inconsistent with showing the shortcut for Show All Bookmarks at the bottom...

Comment 2

[Stephen Horlander [:shorlander]]

(In reply to Justin Dolske [:Dolske] from comment #1)
> I vaguely remember discussion on not wanting to show shortcuts in the panel
> views, but it does at least seem inconsistent with showing the shortcut for
> Show All Bookmarks at the bottom...

I don't remember a discussion about not showing them. We should show them, when we have them, in list views whenever possible; for parity with the previous menu and just generally to expose the information.

Comment 3

[Justin Dolske [:Dolske]]

Perhaps even more generally (for this bug) we should make sure all subview items that we want to show keyboard shortcut do in fact show them.

Comment 4

[u428464]

(In reply to Justin Dolske [:Dolske] from comment #3)
> Perhaps even more generally (for this bug) we should make sure all subview
> items that we want to show keyboard shortcut do in fact show them.

That includes for example all history view subitems (clear recent history, View history sidebar, show all history).

Comment 5

[Tim Nguyen :ntim]

Giving a shot at this.

Comment 6

[Tim Nguyen :ntim]

I'm not familiar with the Customize UI code unfortunatly :(

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: add more shortcut tooltip and label helpers where possible

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: add more shortcut tooltip and label helpers where possible

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8411772 [details] [diff] [review]
Patch v1: add more shortcut tooltip and label helpers where possible

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

I'm fine with this, with my documentation / warn caveats below. Thanks mikedeboer!

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1253,5 @@
> +      let commandId = aShortcutNode.getAttribute("command");
> +      if (commandId)
> +        shortcut = ShortcutUtils.findShortcut(document.getElementById(commandId));
> +    }
> +    if (!shortcut)

It might be good to warn in the console for the reason why this didn't go through instead of just silently failing.

@@ +3272,5 @@
> +   *
> +   * @param aShortcutNode the XUL node where the shortcut will be derived from;
> +   * @param aTargetNode   the XUL node on which the `shortcut` attribute will be
> +   *                      set;
> +   */

We should also mention the fact that if aTargetNode is null, we'll attempt to apply the shortcut attribute to the original aShortcutNode.

We'll also need to add documentation for this in https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/CustomizableUI.jsm (so please add dev-doc-needed).

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: add more shortcut tooltip and label helpers where possible

Patch with comments addressed. Carrying over r=mconley.

Comment 11

[Mike de Boer [:mikedeboer]]

Thanks mconley!

Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/992fc815a807

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/992fc815a807