Closed Bug 882074 Opened 6 years ago Closed 6 years ago

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

Categories

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

All
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: andrei, Assigned: mbrubeck)

References

Details

(Keywords: intl, Whiteboard: [shovel-ready] [preview] feature=defect c=tbd u=tbd p=2)

Attachments

(1 file)

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?
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.
Summary: Missing shortcuts DTD entities in metro browser.dtd → Shortcuts are hard-coded instead of used via accesskey from DTD files
Adding this to our v1 backlog since it looks like we need to fix this for proper localization.  Work estimate: p=2.
Keywords: intl
Hardware: x86 → All
Summary: Shortcuts are hard-coded instead of used via accesskey from DTD files → [MP] Defect - Shortcuts are hard-coded instead of used via accesskey from DTD files
Whiteboard: feature=defect [shovel-ready]
Whiteboard: feature=defect [shovel-ready] → [shovel-ready] [preview] feature=defect c=tbd u=tbd p=0
Assignee: nobody → mbrubeck
Blocks: metrov1it13
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [shovel-ready] [preview] feature=defect c=tbd u=tbd p=0 → [shovel-ready] [preview] feature=defect c=tbd u=tbd p=2
Whiteboard: [shovel-ready] [preview] feature=defect c=tbd u=tbd p=2 → [shovel-ready] [preview] feature=defect c=tbd u=tbd p=2
Attached patch patchSplinter Review
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
Attachment #790768 - Flags: review?(rsilveira)
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?
Attachment #790768 - Flags: review?(rsilveira) → review+
Landed with review comments addressed:
https://hg.mozilla.org/integration/fx-team/rev/77b5c14373b1
https://hg.mozilla.org/mozilla-central/rev/77b5c14373b1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Could anyone please give any guidelines that can help the QA in verifying this issue? Thanks!
Flags: needinfo?
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
Flags: needinfo?
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!
Flags: needinfo?
Flags: needinfo? → needinfo?(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!
Flags: needinfo?(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!

I've logged bug 923460 and bug 923462, and CC'ed you.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.