Closed Bug 285161 Opened 19 years ago Closed 19 years ago

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

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: linxspider, Assigned: emaijala+moz)

References

Details

(Keywords: intl, regression, useless-UI)

Attachments

(1 file, 1 obsolete file)

Patch v2
2.40 KB, patch
neil
: review+
roc
: superreview+
asa
: approval1.8b2+
asa
: approval1.8b3+
Details | Diff | Splinter Review 
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
Flags: blocking1.8b2?
Flags: blocking-aviary1.1?
Keywords: intl, useless-UI
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.
affects the hebrew l10n as well.
Any idea when this regressed?  At least which 1.8a release?
To ere based on that guess.
Assignee: win32 → emaijala
(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
See 2nd patch in bug 287179. It should fix this bug.
ere? I really think this needs to be dealt with, or backed out.
Flags: blocking-aviary1.1? → blocking-aviary1.1+
Keywords: regression
I was waiting for what Dainis comes up with the work in progress in bug 287179.
Dainis, what's the situation?
Depends on: 287179
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...
I'll take a look at this specific problem now. We can try to come up with
something better in bug 287179 later.
Just for the record: the faulty fix that was accidentally checked in while
fixing bug 217560 should've been bug 178110.
Depends on: 178110
Attached patch Patch v1 (obsolete) — — Splinter Review 
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.
Attachment #180165 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #180165 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attached patch Patch v2 — — Splinter Review 
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.
Attachment #180165 - Attachment is obsolete: true
Attachment #180184 - Flags: superreview?(bryner)
Attachment #180184 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #180184 - Flags: review?(neil.parkwaycc.co.uk) → review+
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.
MingW testing would be a good idea. Please go ahead and test it.
Comment on attachment 180184 [details] [diff] [review]
Patch v2

FYI, 
This patch compiles fine under cygwin/MingW
Comment on attachment 180184 [details] [diff] [review]
Patch v2

FYI, 
This patch compiles fine under cygwin/MingW
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.
==========================================================================
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.
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.
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.
Attachment #180184 - Flags: superreview?(bryner) → superreview?(roc)
Flags: blocking1.8b3+
Flags: blocking1.8b2?
Flags: blocking1.8b2-
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?
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.
Agreed. But don't forget for 1.9 the changes need to be done for other platforms
too. Anyway, sr, please?
Attachment #180184 - Flags: superreview?(roc) → superreview+
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?
Attachment #180184 - Flags: approval1.8b3?
Attachment #180184 - Flags: approval1.8b2?
Comment on attachment 180184 [details] [diff] [review]
Patch v2

a=asa
Attachment #180184 - Flags: approval1.8b3?
Attachment #180184 - Flags: approval1.8b3+
Attachment #180184 - Flags: approval1.8b2?
Attachment #180184 - Flags: approval1.8b2+
Fix checked in to trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
This may have caused bug 319308.
No longer blocks: 319308
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: