882074 - [MP] Defect - Shortcuts are hard-coded instead of used via accesskey from DTD files

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[Andrei Eftimie]

Firefox Metro is missing all keyboard shortcuts from its browser.dtd

Desktop: http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd

Metro: http://mxr.mozilla.org/mozilla-central/source/browser/metro/locales/en-US/chrome/browser.dtd

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is pretty bad, given that all access keys are stored directly inside the XUL file, e.g. the shortcut to open the find bar:

 <key id="key_find" key="f" modifiers="accel" command="cmd_find"/>

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.xul#144

That should really be moved to the DTD file to make the Metro UI localizable. Hasn't this really be noticed yet or is a bug already filed on it? Axel?

Comment 2

[Axel Hecht [:Pike]]

We're not tracking metro until we're tracking metro, basically. It's good to find these bugs, though, cause they're the reason why metro isn't switched on for l10n.

Comment 3

[Matt Brubeck (:mbrubeck)]

Adding this to our v1 backlog since it looks like we need to fix this for proper localization.  Work estimate: p=2.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This patch moves the keys for our existing keyboard shortcuts into browser.dtd.  Notes:

* Metro apps don't have "accesskeys" like Windows or Linux desktop apps.  Instead they have only keyboard shortcuts (like Mac or Android apps) with "Control" as the accelerator key.

* Shortcuts that use non-alphabetic keys like F5 or Backspace are still hard-coded.  This matches the behavior of desktop Firefox:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#313

Comment 5

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 790768 [details] [diff] [review]
patch

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

LGTM.

::: browser/metro/base/content/browser.xul
@@ +135,1 @@
>      <key id="key_focusURL2" key="&urlbar.accesskey;" modifiers="alt" command="cmd_openLocation"/>

nit: can you move the urlbar.accesskey definition on the dtd file together with the other shortcuts?

@@ +139,4 @@
>  
>      <!-- misc -->
> +    <key id="key_find" key="&find.key;" modifiers="accel" command="cmd_find"/>
> +    <key id="key_find" key="&quickFind.key;" command="cmd_find"/>

Is this one localizable?

Comment 6

[Matt Brubeck (:mbrubeck)]

Landed with review comments addressed:
https://hg.mozilla.org/integration/fx-team/rev/77b5c14373b1

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77b5c14373b1

Comment 8

[Manuela Muntean [Away]]

Could anyone please give any guidelines that can help the QA in verifying this issue? Thanks!

Comment 9

[Rodrigo Silveira [:rsilveira] [:rodms]]

This was replacing where the values for shortcut keys come from, but the values are all the same. You can test if shortcuts are working. Later, once they're localized we can test if they change according to locale. The shortcuts altered by this change were:

reload2 - accel + R
forceReload2 - accel + shift + R
focusURL - accel + L
open - accel + O
save - accel + S
find - accel + F
findNext2 - accel + G
findPrevious2 - accel + shift + G
quit - accel + Q
addBoomkark - accel + D
newTab - accel + T
newTab2 - accel + N
closeTab - accel + W
undoCloseTab - accel + shift + T

developer shortcuts:
console - accel + shift + J
options flyout - accel + shift + O
sync flyout - accel + shift + S
about - accel + shift + A

Comment 10

[Manuela Muntean [Away]]

While testing this for iteration #13, on Win 8 64bit, with the latest Nightly (build ID: 20130908030205), I confirm that all shortcuts from comment 9 work, except for:

1) save - CTRL + S
2) quit - CTRL + Q

that don't seem to do anything in Metro mode.

Any thoughts/suggestions? Thanks!

Comment 11

[Matt Brubeck (:mbrubeck)]

(In reply to Manuela Muntean [:Manuela] [QA] from comment #10)
> 1) save - CTRL + S
> 2) quit - CTRL + Q
> 
> that don't seem to do anything in Metro mode.

Those are both bugs, though they are unrelated to this change.  (It looks like the key commands are received but the code that they run is broken.)  Could you please file two follow-up bugs, one for each of these?  Thanks!

Comment 12

[Manuela Muntean [Away]]

> (In reply to Manuela Muntean [:Manuela] [QA] from comment #10)
> > 1) save - CTRL + S
> > 2) quit - CTRL + Q
> > 
> > that don't seem to do anything in Metro mode.
> 
> Those are both bugs, though they are unrelated to this change.  (It looks
> like the key commands are received but the code that they run is broken.) 
> Could you please file two follow-up bugs, one for each of these?  Thanks!

I've logged bug 923460 and bug 923462, and CC'ed you.