339723 - Ctrl++ doesn't work with JIS keyboard

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ctrl++ doesn't work on Firefox with JIS Keyboard.
This regressed between 20060316 build and 20060317 build.
I think that the cause is bug 287179.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

I think that we don't need to care the VK_OEM_PLUS as VK_EQUAL. Because on Win2k and WinXP, plus key always send VK_OEM_PLUS on every keyboard layout that information is written in MSDN library.

Comment 2

[Ere Maijala (slow)]

Comment on attachment 223825 [details] [diff] [review]
Patch rv1.0

Please remove the comment part in parentheses ("Equals is not needed..."). It's only confusing to comment that something works as it should. Other than that, good.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1 [checked in]

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

Comment 5

[[e-mail]]

VK_ADD is the + key on the numeric keyboard.
VK_OEM_PLUS is the =/+ key to the left of Backspace on a UK keybord.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Neil, are there regressions by my patch in UK keyboard layout?

Comment 7

[[e-mail]]

(In reply to comment #6)
>Neil, are there regressions by my patch in UK keyboard layout?
Sorry for the delay, I pulled the tree at a bad time and had to repull.
Please see attachment 55849 [details] for the keyboard test page.
The UK keyboard layout has two keys between 0 and backspace.
On the 1.8 branch the Alt key doesn't affect the char code.
On the trunk the some Alt keys have the wrong char code:
Key	Shift	Alt	Alt+Shift
-	_	-	-
=	+	+	+

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #7)
> On the 1.8 branch the Alt key doesn't affect the char code.
> On the trunk the some Alt keys have the wrong char code:
> Key     Shift   Alt     Alt+Shift
> -       _       -       -
> =       +       +       +

Did you mean the correct behavior is:

Key     Shift   Alt     Alt+Shift
-       _       -       -
=       +       =       =

this table?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, did you want to say we should not dispatch keypress event if alt++ etc?

Comment 10

[[e-mail]]

On the 1.8 branch the Alt key doesn't affect the char code:
Key     Shift   Alt     Alt+Shift
-       _       -       _
=       +       =       +

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Additional Patch rv1.0

The problem that is reported by Neil is not my regression. Because current logic in |nsWindow::OnKeyDown| is wrong.

We have two key codes:
1. aVirtualKeyCode: This comes from WM_KEYDOWN message. So, this should use only for native processing. We should not use this for XP processing.
2. virtualKeyCode: This is converted from aVirtualKeyCode. So, this should use only XP processing. We should not use this for native processing.

So, the names are very confusing. I rename the virtualKeyCode to DOMKeyCode. By this changing, we can find a bug for the variables using.

Now, we are using DOMKeyCode for the switch statement that is bottom of the function. But the switch statement is processing native to XP. So, the switch should not use DOMKeyCode. We should use aVirtualKeyCode.

But if only changing it, this bug comes back. Because VK_OEM_PLUS and VK_OEM_MINUS cases go to default. But Ctrl++ and Ctrl+- are not character inputtable key combination. Therefore, |gKbdLayout.GetUniChars| returns 0.

We should not handle textzoom case on here. Instead of, we should early return after WM_CHAR messages removing.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ere:

Would you review the patch? I think bug 341308 is serious for German people.
# This patch fixes bug 341308 too.

Comment 13

[Ere Maijala (slow)]

Comment on attachment 224871 [details] [diff] [review]
Additional Patch rv1.0

The thing worrying me a bit is that the patch switches by aVirtualKeyCode but then uses DOMKeyCode in the case statements. I hope we won't suffer from it.

Change the first comment to something like this:

// Use only DOMKeyCode for XP processing.
// Use aVirtualKeyCode for gKbdLayout and native processing.

And the text zoom comment:

// We need to handle text zoom as a special case
// because it needs to have unichar from DOMKeyCode.

With these changes, let's try it in trunk.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Additional Patch rv1.1

Thank you Ere for your review.

Roc, Would you sr this?

Comment 15

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

Can you explain carefully why ctrl-+ and ctrl-- need special handling here? it looks very nasty having textzoom key bindings, which are a very high level application-specific thing, mentioned down here in nsWindow.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #15)
> Can you explain carefully why ctrl-+ and ctrl-- need special handling here? it
> looks very nasty having textzoom key bindings, which are a very high level
> application-specific thing, mentioned down here in nsWindow.

Yeah, I agree it. But the key press event is not dispatched without the code for special cases on Ctrl++ or Ctrl+- in some keyboard layout. Because if '+' or '-' needs shift key for inputting, Ctrl++ or Ctrl+- is not dispatched as '+' or '-' key events. But Ctrl+Shift++ or Ctrl+Shift+- are ignored by XUL on these keyboard layout. So we need these special case code for some keyboard layouts.
# So, these shortcuts are not suitable for intl...

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, Roc, see comment 16.

Comment 18

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

Is there a way to fix this at a higher level? Suppose we add the DOMKeyCode to the KEY_PRESS event and then modify command dispatching to optionally look at the DOMKeyCode?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #18)
> Is there a way to fix this at a higher level? Suppose we add the DOMKeyCode to
> the KEY_PRESS event and then modify command dispatching to optionally look at
> the DOMKeyCode?

Roc, I want to look the command dispatching code, where is it?
And I think that current trunk build is not usable for some keyboard layout users. First, should we check-in the patch? (before this discussion.)

Comment 20

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

(In reply to comment #19)
> (In reply to comment #18)
> > Is there a way to fix this at a higher level? Suppose we add the DOMKeyCode
> > to the KEY_PRESS event and then modify command dispatching to optionally
> > look at the DOMKeyCode?
> 
> Roc, I want to look the command dispatching code, where is it?

XUL key bindings are set up by <key> elements, see http://developer.mozilla.org/en/docs/XUL:key
http://lxr.mozilla.org/mozilla/search?string=key_textZoomEnlarge
I don't know where the code for dispatching these is, off the top of my head, but I guess you can find it.

> And I think that current trunk build is not usable for some keyboard layout
> users. First, should we check-in the patch? (before this discussion.)

It's not usable because of this bug? Not being able to do text zoom with the keyboard doesn't seem so serious to me. I'd rather not check in a temporary fix if we can figure out a better fix.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #20)
> > And I think that current trunk build is not usable for some keyboard layout
> > users. First, should we check-in the patch? (before this discussion.)
> 
> It's not usable because of this bug? Not being able to do text zoom with the
> keyboard doesn't seem so serious to me. I'd rather not check in a temporary fix
> if we can figure out a better fix.

No, see bug 341308 and 342197. They are regression of previous patch. If the textzoom special case is used since my current patch, I should back out the previous patch. But the special case code is existing on current Trunk. I think that your discontent should be separated to new bug. Because it has current code.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, but I have an idea. It may removes the *textzoom* special case from the code. But you may hate it too. Please wait.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Additional Patch rv1.2

Roc:

How about this patch?
This patch makes general the shortcut key processing.

If Ctrl or Alt is pressed and any characters are not inputted, this patch prefers DOMKeyCode. Therefore, we can keep normal key inputting. And we can change only shortcut key dispatching. So, the behavior of comment 7 is designed by this patch. (I think that the problem is WONTFIX.)

Comment 24

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

Comment on attachment 227254 [details] [diff] [review]
Additional Patch rv1.2

This looks much better

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 227254 [details] [diff] [review]
Additional Patch rv1.2

Ere: Would you re-review this?

Thank you, roc.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in to trunk, thanks.

now, if Alt is used without Ctrl, the key code is used as '-' or '+'.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, the tree is closed now, I backed-out the patch.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

re-checked-in.

-> FIXED