Shortcuts in customizableWidgets.properties should not include modifiers

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: flod, Assigned: Gijs)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
Firefox 28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M9][Australis:P3][strings])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Bug 868433 introduced several shortcuts in customizableui/customizableWidgets.properties

save-page-button.shortcut = Ctrl+S
developer-button.shortcut = Shift+F11
preferences-button.shortcut = Ctrl+Shift+O
[...]

Problem is that these shortcuts change according to the OS, so it doesn't make sense to hardcode modifiers inside properties files (e.g on Mac they will use Cmd instead of Ctrl).

If, for example, "Save page button" has the same function of the Save menu in File, it should probably use its commandkey directly to avoid differences.
(Reporter)

Updated

5 years ago
Blocks: 872617
(Reporter)

Updated

5 years ago
Blocks: 887911
(Reporter)

Updated

5 years ago
No longer blocks: 887911
(Reporter)

Updated

5 years ago
OS: Windows 7 → All
Are these strings even being used anywhere? Are they for display, or does something parse them?
Whiteboard: [Australis:M?][Australis:P3][strings]
(Assignee)

Comment 2

5 years ago
(In reply to Justin Dolske [:Dolske] from comment #1)
> Are these strings even being used anywhere? Are they for display, or does
> something parse them?

We're setting them as acceltext attributes on toolbarbuttons, which AFAIK does nothing. (see also bug 868433 comment 6 )

I think we should remove them, or find some way to include them in the button tooltips. As long as we still have the actual menubar and keysets, they should continue to work, but we wouldn't be exposing them so for discoverability it's probably better if we have 'something'.

Blair, does that sound about right? What would you prefer happen here?
Flags: needinfo?(bmcbride)
I think we should just remove them.
I think we need some UX brainwaves on this. 

I'd love to see the shortcuts displayed in the tooltip, but that does add noise.That said, bug 492557 could solve the discovery issue nicely, but I don't know when that's going to be ready to land (almost certainly sometime after Australis ships).
Flags: needinfo?(bmcbride) → needinfo?(zfang)
(Assignee)

Comment 5

5 years ago
(In reply to Blair McBride [:Unfocused] from comment #4)
> I think we need some UX brainwaves on this. 
> 
> I'd love to see the shortcuts displayed in the tooltip, but that does add
> noise.That said, bug 492557 could solve the discovery issue nicely, but I
> don't know when that's going to be ready to land (almost certainly sometime
> after Australis ships).

Personally, I think, given:

1) There are issues with duplication of strings which are actually used for the actual shortcut, and the risk of saying one thing but having another thing that actually works;
2) There are platform-specificity issues where we have a string that's hardcoded to one platform which is wrong on another platform;
3) We don't show them and it's not clear exactly how we would;
4) Adding a 'shortcut' property to a widget spec in the API might have people believe that alone would be enough to make it work;
5) For it to really be equivalent to clicking the items, we'd need to implement extra code to have eg. the shortcut for the history panel actually open the menupanel and then the subview, and not some other, somewhat equivalent thing for which we do already have shorcuts.

We should just remove them. Which is sad because I agree that we need discoverable shortcuts; I just don't think this is how we'll get them.

If/when we have the HUD (seems that patch is coming along!) that can provide discoverability.
(Assignee)

Comment 6

5 years ago
I discussed this with Stephen on IRC. The idea would be to include the shortcuts in the tooltips. Implementation-wise, I would like to:

- remove the l10n strings
- add a property to the widget spec being "shortcutKeyId" that should point to a valid <key> in the main document, from which we can get the information to display the correct shortcut key
- add code to get that info and include it in the tooltip

This doesn't address the usecase of discoverability for keyboard users. We may want to file a followup bug about that, but I don't think it should block this bug.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(zfang)
(Assignee)

Comment 7

5 years ago
Created attachment 816535 [details] [diff] [review]
part 1: remove shortcuts from l10n file,

Alright, let's get cracking. First off, let's remove these.
Attachment #816535 - Flags: review?(mconley)
(Assignee)

Comment 8

5 years ago
Created attachment 816546 [details] [diff] [review]
part 2: add shortcut references and display them,

This was surprisingly easy. However, instances of code using platformkeys (each with its own quirks) live in at least 3-4 different places in the codebase (in both C++ and JS). Maybe we should file a followup bug to deal with this situation. Or maybe, considering the simplicity of the code, we should just live with it?
Attachment #816546 - Flags: review?(mconley)
(Assignee)

Comment 9

5 years ago
Created attachment 816547 [details] [diff] [review]
part 2: add shortcut references and display them,

This was suI added two keybindings and forgot to update the strings.
Attachment #816547 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #816546 - Attachment is obsolete: true
Attachment #816546 - Flags: review?(mconley)
Comment on attachment 816547 [details] [diff] [review]
part 2: add shortcut references and display them,

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

Yeah, I think this will work nicely. I'm a little wary of code duplication with that createShortcutString function... at the very least, we might want to file a bug for de-duping this somehow.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1048,5 @@
>      }
>      return def;
>    },
>  
> +  // From devtools/shared/helpers.js:

Part of me wonders if this should be some kind of globally available function, or importable via some module, rather than being duped in several places...

::: browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties
@@ +4,5 @@
>  
>  history-panelmenu.label = History
>  # LOCALIZATION NOTE (history-panelmenu.tooltiptext): Use the unicode ellipsis char,
>  # \u2026, or use "..." if \u2026 doesn't suit traditions in your locale.
> +history-panelmenu.tooltiptext = History… (%S)

We'll probably want to document what the %S stands for in all of these for our localizers.
Attachment #816547 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) from comment #10)
> Part of me wonders if this should be some kind of globally available
> function, or importable via some module, rather than being duped in several
> places...

+1 - want this for the likes of bug 492557 too. File a followup?


> We'll probably want to document what the %S stands for in all of these for
> our localizers.

Not "want". Need. Don't land without documenting that please.
(In reply to :Gijs Kruitbosch from comment #6)
> This doesn't address the usecase of discoverability for keyboard users. We
> may want to file a followup bug about that, but I don't think it should
> block this bug.

Did this get filed?
(Assignee)

Updated

5 years ago
Depends on: 927603
(Assignee)

Updated

5 years ago
Depends on: 927605
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/projects/ux/rev/254750fd2dd1
https://hg.mozilla.org/projects/ux/rev/4f9c73d2fae3
Whiteboard: [Australis:M?][Australis:P3][strings] → [Australis:M9][Australis:P3][strings][fixed-in-ux]
(Assignee)

Comment 14

5 years ago
(In reply to Blair McBride [:Unfocused] from comment #11)
> (In reply to Mike Conley (:mconley) from comment #10)
> > Part of me wonders if this should be some kind of globally available
> > function, or importable via some module, rather than being duped in several
> > places...
> 
> +1 - want this for the likes of bug 492557 too. File a followup?

Bug 927605

> > We'll probably want to document what the %S stands for in all of these for
> > our localizers.
> 
> Not "want". Need. Don't land without documenting that please.

Fixed before landing.

(In reply to Blair McBride [:Unfocused] from comment #12)
> (In reply to :Gijs Kruitbosch from comment #6)
> > This doesn't address the usecase of discoverability for keyboard users. We
> > may want to file a followup bug about that, but I don't think it should
> > block this bug.
> 
> Did this get filed?

It did now! Bug 927603
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/mozilla-central/rev/254750fd2dd1
https://hg.mozilla.org/mozilla-central/rev/4f9c73d2fae3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P3][strings][fixed-in-ux] → [Australis:M9][Australis:P3][strings]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.