285161 - regression: access keys don't work on non latin locales when Alt+letter is pressed together

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Reuven Gonen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; he-IL; rv:1.8b2) Gecko/20050302 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; he-IL; rv:1.8b2) Gecko/20050302 Firefox/1.0+

in a non latin locale pressing the Alt+access key, dosen't work. I've tested
this with mozilla 1.8a6, and the firefox trunk. on aviary branch the access keys
seem to be fine.

Reproducible: Always

Steps to Reproduce:
1. download mozilla 1.8a6 and install a non-Latin locale (you can test with the
russian langpack from:
http://ftp.mozilla.org/pub/mozilla.org/mozilla/l10n/lang/moz1.8alpha6/

2.press the Alt-shortcut key combination to open one of the menus(use a non
latin access key)

Actual Results:  
nothing happens

Expected Results:  
menu opens

Comment 1

[Benjamin Smedberg]

Does this happen in locales other than Russian? I want to make sure it's a
"core" problem, not a Russian l10n issue, before spending a lot of time on it. I
don't know a lot about our accesskey code and certainly don't know how it has
changed recently.

Comment 2

[Asaf Romano (gone)]

affects the hebrew l10n as well.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Any idea when this regressed?  At least which 1.8a release?

Comment 4

[Simon Montagu :smontagu]

I think the fix for bug 178110 via bug 217560 might have regressed this (see bug
178110 comment 26 and
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/widget/src/windows&command=DIFF_FRAMESET&file=nsWindow.cpp&rev2=3.506&rev1=3.505
the hunk starting at line 2986)

Comment 5

[Boris Zbarsky [:bzbarsky]]

To ere based on that guess.

Comment 6

[Reuven Gonen]

(In reply to comment #4)
> I think the fix for bug 178110 via bug 217560 might have regressed this (see bug
> 178110 comment 26 and
>
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/widget/src/windows&command=DIFF_FRAMESET&file=nsWindow.cpp&rev2=3.506&rev1=3.505
> the hunk starting at line 2986)

indeed. by viewing the trunk nightlies, the bug was created between the 9 and 10
of july 2004. these are the changes committed to the trunk between those days
according to bonsai:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-07-09&maxdate=2004-07-10&cvsroot=%2Fcvsroot

Comment 7

[Dainis Jonitis]

See 2nd patch in bug 287179. It should fix this bug.

Comment 8

[Benjamin Smedberg]

ere? I really think this needs to be dealt with, or backed out.

Comment 9

[Ere Maijala (slow)]

I was waiting for what Dainis comes up with the work in progress in bug 287179.
Dainis, what's the situation?

Comment 10

[Dainis Jonitis]

I had no enough spare time to experiment with the code for bug 287179. I will
try to look at the dead-key problem interfering with the ToUnicode function this
week.
I also spent some time reading various things about international keyboard
layouts and related problems. Many things should be taken into account to
correctly emulate WM_CHAR from WM_KEYDOWN (like fact that in some layouts
CapsLock is not the same thing as Shift, left Ctrl+Alt combination (AltGr) is
yet another switch combination, there can be more than one deadkey etc.). Many
other open source projects gave up on this and use plain WM_CHAR instead...

Comment 11

[Ere Maijala (slow)]

I'll take a look at this specific problem now. We can try to come up with
something better in bug 287179 later.

Comment 12

[Ere Maijala (slow)]

Just for the record: the faulty fix that was accidentally checked in while
fixing bug 217560 should've been bug 178110.

Comment 13

[Ere Maijala (slow)]

Attachment: Patch v1

First stab at fixing this problem. This patch moves alt+char handling back to
WM_CHAR handling in OnChar(). As Dainis noted, it's pretty difficult to
synthesize the correct char in WM_KEYDOWN but the effort probably continues in
the other bug. What this doesn't fix are the cases when a WM_CHAR message isn't
received and a non-latin keyboard is used (typically ctrl+alt+something). This
patch now keeps also ctrl+char lowercase as this is the behavior I observed in
Linux (although there it doesn't work right with a Russian keyboard).

I couldn't get the Russian language packs working in a nightly build (any
tricks?) but I used Dean's keyboard test page
(https://bugzilla.mozilla.org/attachment.cgi?id=55849) to check the behavior.

Comment 14

[[e-mail]]

Comment on attachment 180165 [details] [diff] [review]
Patch v1

By my reading of the code you'll need to put the towlower in just before the
call to DispatchKeyEvent. Otherwise it looks good to me.

Comment 15

[Ere Maijala (slow)]

Attachment: Patch v2

This is practically rewritten due to Neil's comment. I also added a fix for
ctrl+alt+something with Caps Lock on for latin keyboards.

Comment 16

[David G King]

Ere,

If you want any cygwin/MingW build testing like I was doing for Dainis over at
Bug #287179, just let me know.

Also, technically, this bug now blocks Bug #287179, but I figure I'd just make
this comment rather than changing the dependencies.

Comment 17

[Ere Maijala (slow)]

MingW testing would be a good idea. Please go ahead and test it.

Comment 18

[David G King]

Comment on attachment 180184 [details] [diff] [review]
Patch v2

FYI, 
This patch compiles fine under cygwin/MingW

Comment 19

[David G King]

Comment on attachment 180184 [details] [diff] [review]
Patch v2

FYI, 
This patch compiles fine under cygwin/MingW

Comment 20

[Dainis Jonitis]

Neil, Ere, Simon, Jungshik

Please reconsider that it is responsibility of platform specific code to always
report lower case characters if either Alt or Ctrl keys are pressed or CapsLock
is active. The need to determine the real shifted/unshifted state of character
in KeyPress event was the reason why I opened bug 287179. This fix will purge
all my effort to fix it.

1. It is bad idea to throw away the CapsLock state (as in bug 178110). Not in
all languages CapsLock means ShiftLock. For some keybord layouts the CapsLock
turns on yet another shift state that does not always affect the case of
letters, but instead can give access to some special symbols, dead-keys etc.
Generally CapsLock+<letter> does not equal upper case <letter>.

2. The same can be true for Shift key, too. In some languages that do not have
concept of letter case the Shift key in combination with some letter my produce
some unrelated character or special symbol.
Generally Shift+<letter> does not equal upper case <letter>. Just by making call
to towlower() we will receive the same letter and will loose possibility to get
character that is mapped to Shift+<letter> state. 

3. To get correct shifted/unshifted state is even more important for OEM keys
(like '`~', ',<', '.>', '/?', '\|', '[{', ']}', ';:', ''"' in US layout).
Depending on keyboard layout they may be mapped to different keys and in
different combinations. For example, I want to handle Ctrl+'~' combination in
KeyPress event, no matter on which key the '~' symbol is situated. An
alternative is to handle it in KeyDown when
Ctrl+Shift+KeyEvent.DOM_VK_BACK_QUOTE is pressed. It will be true for
traditional US keyboard layout, but will not be true for Czech which have '~'
symbol on different key.

Except for bug 178110 I do not see any reason why to change letter case when Alt
or Ctrl key is pressed. If letter case is not important for access keys the
calling cross platform code can always call towlower () itself. As it is cross
platform code the changes are in one well localized place and they won't be
affected by some behaviour changes in platform specific code. 

==========================================================================
My suggestion is not to throw away uppercase/lowercase information in platform
specific widget code and pass it to the caller. If the caller does not care
about letter case it can always convert to the lowercase itself! On the other
hand it is impossible for caller to reliably determine the character for
Shift+<letter> combination if it only knows the lower case letter.
==========================================================================

Comment 21

[[e-mail]]

OEM keys are a nightmare. Using Mozilla 1.6 with a UK keyboard layout, if I
press Ctrl+# I actually get a) a keydown event for 222 (which I believe is \) b)
a keypress event for Ctrl+' c) a keypress for Ctrl+\ d) a keyup event for 222.
(I think one of those keypress events was suppressed in later versions.)
In this case Windows never mentions the # at all.

Comment 22

[Dainis Jonitis]

Neil,

With trunk Mozilla + my latest patch from bug 287179 for United Kingdom keyboard
layout I get this:

1) keydown charCode=undefined keyCode=222 character=|| modifiers=Ctrl
2) keypress charCode=35 keyCode=35 character=|#| modifiers=Ctrl
3) keyup charCode=undefined keyCode=222 character=|| modifiers=Ctrl

This is correct character '#' for UK layout.

Comment 23

[Ere Maijala (slow)]

Well, at least I'm not the one to say what platform-specific code should return.
I'm afraid it would be a pretty big task to change it as it would require all
the platform specific stuff for other platforms changed too. I have no idea how
much work it would be for different platforms.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Apparently Dainis' patch in bug 287179 will not fix this bug (and would create
nwe bugs) because upper-level code would also have to be changed ... right?

Suppose we check this in for 1.8, then for 1.9 take this out and put in Dainis'
patch it 287179, then fix the upper level code --- is that a good plan?

Comment 25

[Dainis Jonitis]

I found most of the callers that should be fixed to correctly handle
upper/lowercase characters. I will fix them in a separate bug.
I agree that for now it is better and safer to check in Ere's patch.

Comment 26

[Ere Maijala (slow)]

Agreed. But don't forget for 1.9 the changes need to be done for other platforms
too. Anyway, sr, please?

Comment 27

[Ere Maijala (slow)]

Comment on attachment 180184 [details] [diff] [review]
Patch v2

Requesting approval. Should I wait for b3 or would it be ok to have this in b2?

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 180184 [details] [diff] [review]
Patch v2

a=asa

Comment 29

[Ere Maijala (slow)]

Fix checked in to trunk.

Comment 30

[Marek Stępień [:marcoos, inactive]]

This may have caused bug 319308.