887853 - Shortcuts in customizableWidgets.properties should not include modifiers

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Francesco Lodolo [:flod]]

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.

Comment 1

[Justin Dolske [:Dolske]]

Are these strings even being used anywhere? Are they for display, or does something parse them?

Comment 2

[:Gijs (he/him)]

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

Comment 3

[:Gavin Sharp [email: [e-mail]]]

I think we should just remove them.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 5

[:Gijs (he/him)]

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

Comment 6

[:Gijs (he/him)]

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.

Comment 7

[:Gijs (he/him)]

Attachment: part 1: remove shortcuts from l10n file,

Alright, let's get cracking. First off, let's remove these.

Comment 8

[:Gijs (he/him)]

Attachment: 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?

Comment 9

[:Gijs (he/him)]

Attachment: part 2: add shortcut references and display them,

This was suI added two keybindings and forgot to update the strings.

Comment 10

[Mike Conley (: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.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 13

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/254750fd2dd1
https://hg.mozilla.org/projects/ux/rev/4f9c73d2fae3

Comment 14

[:Gijs (he/him)]

(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

Comment 15

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/254750fd2dd1
https://hg.mozilla.org/mozilla-central/rev/4f9c73d2fae3