776075 - Command keys using upper-case non-ASCII characters are broken

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[u60234]

+++ 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]

Looks like bug 492931

Comment 2

[Gordon P. Hemsley [:GPHemsley]]

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

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] (he/him)(JST, +0900)]

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 [:bzbarsky]]

Yeah, that change should probably be undone.

Comment 6

[Henri Sivonen (:hsivonen)]

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

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]

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-4, no longer working on Mozilla)]

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

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-4, no longer working on Mozilla)]

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] (he/him)(JST, +0900)]

Maybe, we can test it in widget/tests/test_keycodes.xul.

Comment 13

[Ed Morley [:emorley]]

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

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

[u279076]

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

[u60234]

Yes, uppercase letters such as Ä and Ö works fine in today's nightly. Tested on Windows and Linux.

Comment 17

[u279076]

Thanks Hasse, removing verifyme keyword. Lukas, back to you.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2fa008533f03

Comment 19

[u279076]

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-4, no longer working on Mozilla)]

(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

[u60234]

Tested today's Aurora build on Windows and Linux. Uppercase Ä and Ö worked fine in both.

Comment 22

[u279076]

Thanks Hasse!