Australis - Show shortcuts for all widgets

VERIFIED FIXED in Firefox 32

Status

()

Firefox
Toolbars and Customization
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Alice0775 White, Assigned: mikedeboer)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {dev-doc-needed})

28 Branch
Firefox 32
x86_64
Windows 7
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox30 affected, firefox31 affected, firefox32 affected)

Details

(Whiteboard: [Australis:P4+] p=2 s=it-32c-31a-30b.2 [qa-])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8343862 [details]
screenshot

Add shortcuts label "Ctrl+B" for "View Bookmarks Sidebar" in Bookmarks Widget panel
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...
Flags: needinfo?(shorlander)
Whiteboard: [Australis:P4]
(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.
Flags: needinfo?(shorlander)
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.
Whiteboard: [Australis:P4] → [Australis:P4+]
(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).

Updated

4 years ago
Summary: Add shortcuts label for "View Bookmarks Sidebar" in Bookmarks Widget panel like "Show All Bookmarks" → Australis - Show shortcuts for all widgets

Comment 5

4 years ago
Giving a shot at this.
Assignee: nobody → ntim007
Status: NEW → ASSIGNED

Comment 6

4 years ago
I'm not familiar with the Customize UI code unfortunatly :(
Assignee: ntim007 → nobody
Status: ASSIGNED → NEW

Updated

4 years ago
Flags: firefox-backlog+

Updated

4 years ago
Whiteboard: [Australis:P4+] → [Australis:P4+] p=2
(Assignee)

Comment 7

4 years ago
Created attachment 8411771 [details] [diff] [review]
Patch v1: add more shortcut tooltip and label helpers where possible
Attachment #8411771 - Flags: review?(jaws)
(Assignee)

Updated

4 years ago
Blocks: 979479
(Assignee)

Updated

4 years ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
(Assignee)

Comment 8

4 years ago
Created attachment 8411772 [details] [diff] [review]
Patch v1: add more shortcut tooltip and label helpers where possible
Attachment #8411771 - Attachment is obsolete: true
Attachment #8411771 - Flags: review?(jaws)
Attachment #8411772 - Flags: review?(jaws)
Attachment #8411772 - Flags: review?(jaws) → review?(mconley)

Updated

4 years ago
No longer blocks: 979479
Depends on: 979479
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).
Attachment #8411772 - Flags: review?(mconley) → review+
(Assignee)

Comment 10

4 years ago
Created attachment 8419261 [details] [diff] [review]
Patch v1.1: add more shortcut tooltip and label helpers where possible

Patch with comments addressed. Carrying over r=mconley.
Attachment #8411772 - Attachment is obsolete: true
Attachment #8419261 - Flags: review+
(Assignee)

Comment 11

4 years ago
Thanks mconley!

Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/992fc815a807
status-firefox30: --- → affected
status-firefox31: --- → affected
status-firefox32: --- → affected
Whiteboard: [Australis:P4+] p=2 → [Australis:P4+][fixed-in-fx-team] p=2
(Assignee)

Updated

4 years ago
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/992fc815a807
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4+][fixed-in-fx-team] p=2 → [Australis:P4+] p=2
Target Milestone: --- → Firefox 32

Updated

4 years ago
Whiteboard: [Australis:P4+] p=2 → [Australis:P4+] p=2 s=it-32c-31a-30b.2 [qa?]

Updated

4 years ago
Whiteboard: [Australis:P4+] p=2 s=it-32c-31a-30b.2 [qa?] → [Australis:P4+] p=2 s=it-32c-31a-30b.1 [qa?]

Updated

4 years ago
Whiteboard: [Australis:P4+] p=2 s=it-32c-31a-30b.1 [qa?] → [Australis:P4+] p=2 s=it-32c-31a-30b.2 [qa?]

Updated

4 years ago
Whiteboard: [Australis:P4+] p=2 s=it-32c-31a-30b.2 [qa?] → [Australis:P4+] p=2 s=it-32c-31a-30b.2 [qa-]

Updated

4 years ago
Status: RESOLVED → VERIFIED
Depends on: 1039747
Depends on: 1062128
You need to log in before you can comment on or make changes to this bug.