Last Comment Bug 776075 - Command keys using upper-case non-ASCII characters are broken
: Command keys using upper-case non-ASCII characters are broken
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: 13 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Emanuel Hoogeveen [:ehoogeveen]
:
Mentors:
Depends on:
Blocks: 492931
  Show dependency treegraph
 
Reported: 2012-07-20 12:58 PDT by Hasse
Modified: 2012-08-23 11:07 PDT (History)
19 users (show)
dbaron: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
verified
+
verified


Attachments
backout as described (1.68 KB, patch)
2012-08-20 16:36 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta-
Details | Diff | Review

Description Hasse 2012-07-20 12:58:06 PDT
+++ This bug was initially created as a clone of Bug #770447 +++

Starting with the trunk nightly from 2012-03-12, command keys with non-ASCII chars, like Ctrl+Ö and Ctrl+Ä, no longer works if they are defined with capital letters in the locale dtd file. Lowercase letters are fine.

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6737b6762eb8&tochange=5ec9524de1af
Comment 1 Simon Montagu :smontagu 2012-07-20 15:12:22 PDT
Looks like bug 492931
Comment 2 Gordon P. Hemsley [:GPHemsley] 2012-07-20 15:32:17 PDT
(In reply to Simon Montagu from comment #1)
> Looks like bug 492931

Indeed. That was established in bug 770447 comment 24.

I imagine this is by design, so would the best course of action here be to check the existing command keys for non-ASCII characters?
Comment 3 Axel Hecht [:Pike] 2012-07-20 16:06:44 PDT
Well, non-ASCII characters are totally fine command keys, IMHO.

Also, reading bug 492931, in particular bug 492931 comment 6, I'm not sure that changing the behaviour of control keys was intended. Henri?
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-20 16:17:04 PDT
Hmm, I think that nsXBLPrototypeHandler.cpp change should be backed out because it doesn't make sense ignoring letter case only for ASCII alphabets. And it might break some addon compatibility too.
https://hg.mozilla.org/integration/mozilla-inbound/diff/d09b4e60bb09/content/xbl/src/nsXBLPrototypeHandler.cpp
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-20 18:04:29 PDT
Yeah, that change should probably be undone.
Comment 6 Henri Sivonen (:hsivonen) 2012-07-22 23:35:45 PDT
(In reply to Axel Hecht [:Pike] from comment #3)
> Also, reading bug 492931, in particular bug 492931 comment 6, I'm not sure
> that changing the behaviour of control keys was intended. Henri?

Not intended.
Comment 7 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-23 14:31:43 PDT
Assigning this to Emanuel to get a fix on this, tracking for release so we don't ship this regression. Emanuel, thank you for contributing (and congrats on your first patch in bug 492931, as well), if you can't take on this fix please let us know so we can re-assign in time.
Comment 8 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-20 16:26:19 PDT
Wontfixing for 15 since we're a day away from our final beta of the cycle.  If Emanuel isn't the right assignee for this can someone select another?
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-20 16:36:47 PDT
Created attachment 653577 [details] [diff] [review]
backout as described

This is the backout described in comment 4 and comment 5.  I haven't tested it.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 492931
User impact if declined: broken shortcut keys when non-ASCII and specified in uppercase in the code
Testing completed (on m-c, etc.): none
Risk to taking this patch (and alternatives if risky): Low, I think, but I'm not the expert.
String or UUID changes made by this patch: None.
Comment 10 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-20 18:07:29 PDT
Can we get a try build to test?  I'm not comfortable taking a completely untested backout on our final beta.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-20 19:31:01 PDT
Landed backout on:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1c9884c07e

Once that cycles, mozilla-inbound builds should be testable, though I'm not sure if anyone has localized builds for anything other than beta.  (And I suspect my spinning a build off beta won't help, since it won't be localized.)
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-08-20 19:41:53 PDT
Maybe, we can test it in widget/tests/test_keycodes.xul.
Comment 13 Ed Morley [:emorley] 2012-08-21 06:26:12 PDT
(In reply to David Baron [:dbaron] from comment #11)
> Landed backout on:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1c9884c07e
> 
> Once that cycles, mozilla-inbound builds should be testable, though I'm not
> sure if anyone has localized builds for anything other than beta.  (And I
> suspect my spinning a build off beta won't help, since it won't be
> localized.)

Merged to m-c:
https://hg.mozilla.org/mozilla-central/rev/cb1c9884c07e
Comment 14 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-21 12:15:44 PDT
Comment on attachment 653577 [details] [diff] [review]
backout as described

So it's too late to get the info we need on beta for this, if we can get a verification on the Nightly builds I'll approve for Aurora.
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-21 12:56:33 PDT
RE: verifyme
I don't have access to a keyboard with non-ASCII characters.

Hosse, can you please verify if this is fixed for you with Firefox 17.0a1 Nightly 2012-08-22 or newer? Thanks.
Comment 16 Hasse 2012-08-22 10:40:50 PDT
Yes, uppercase letters such as Ä and Ö works fine in today's nightly. Tested on Windows and Linux.
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-22 10:56:15 PDT
Thanks Hasse, removing verifyme keyword. Lukas, back to you.
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-22 15:45:19 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/2fa008533f03
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-22 15:49:13 PDT
I'm actually going to mark this verified for Firefox 17 based on comment 16.

Hasse, would you mind testing tomorrow's Aurora build to confirm it's fixed for Firefox 16?
Comment 20 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-22 15:50:03 PDT
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 8/22 - 8/27) from comment #12)
> Maybe, we can test it in widget/tests/test_keycodes.xul.

Any chance I could interest you in doing that?
Comment 21 Hasse 2012-08-23 10:59:21 PDT
Tested today's Aurora build on Windows and Linux. Uppercase Ä and Ö worked fine in both.
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-23 11:07:25 PDT
Thanks Hasse!

Note You need to log in before you can comment on or make changes to this bug.