Andrei Eftimie Reporter
	Description • 11 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 1 • 11 years ago

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?

	Axel Hecht [:Pike]
	Comment 2 • 11 years ago

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.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 3 • 11 years ago

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

	Matt Brubeck (:mbrubeck) Assignee
	Comment 4 • 11 years ago

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

	Rodrigo Silveira [:rsilveira] [:rodms]
	Comment 5 • 11 years ago

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?

	Matt Brubeck (:mbrubeck) Assignee
	Comment 6 • 11 years ago

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

	Carsten Book [:Tomcat]
	Comment 7 • 11 years ago

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

	Manuela Muntean [Away]
	Comment 8 • 11 years ago

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

	Rodrigo Silveira [:rsilveira] [:rodms]
	Comment 9 • 11 years ago

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

	Manuela Muntean [Away]
	Comment 10 • 11 years ago

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!

	Matt Brubeck (:mbrubeck) Assignee
	Comment 11 • 11 years ago

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

	Manuela Muntean [Away]
	Comment 12 • 11 years ago

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