Last Comment Bug 339723 - Ctrl++ doesn't work with JIS keyboard
: Ctrl++ doesn't work with JIS keyboard
Status: RESOLVED FIXED
: intl, regression
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows 2000
: -- normal (vote)
: mozilla1.9alpha1
Assigned To: Masayuki Nakano [:masayuki]
: Hixie (not reading bugmail)
: Jim Mathies [:jimm]
Mentors:
Depends on: 359638
Blocks: 287179 341308 342197
  Show dependency treegraph
 
Reported: 2006-05-30 12:46 PDT by Masayuki Nakano [:masayuki]
Modified: 2006-07-17 07:09 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch rv1.0 (2.76 KB, patch)
2006-05-30 12:49 PDT, Masayuki Nakano [:masayuki]
emaijala+moz: review+
Details | Diff | Splinter Review
Patch rv1.1 [checked in] (2.57 KB, patch)
2006-06-04 00:48 PDT, Masayuki Nakano [:masayuki]
masayuki: review+
roc: superreview+
Details | Diff | Splinter Review
Additional Patch rv1.0 (8.29 KB, patch)
2006-06-08 09:04 PDT, Masayuki Nakano [:masayuki]
emaijala+moz: review+
Details | Diff | Splinter Review
Additional Patch rv1.1 (8.26 KB, patch)
2006-06-19 13:03 PDT, Masayuki Nakano [:masayuki]
masayuki: review+
Details | Diff | Splinter Review
Additional Patch rv1.2 (8.34 KB, patch)
2006-06-27 08:14 PDT, Masayuki Nakano [:masayuki]
emaijala+moz: review+
roc: superreview+
Details | Diff | Splinter Review

Description User image Masayuki Nakano [:masayuki] 2006-05-30 12:46:46 PDT
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 User image Masayuki Nakano [:masayuki] 2006-05-30 12:49:22 PDT
Created attachment 223825 [details] [diff] [review]
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 User image Ere Maijala (slow) 2006-06-03 11:01:05 PDT
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 User image Masayuki Nakano [:masayuki] 2006-06-04 00:48:51 PDT
Created attachment 224340 [details] [diff] [review]
Patch rv1.1 [checked in]
Comment 4 User image Masayuki Nakano [:masayuki] 2006-06-05 18:53:23 PDT
checked-in.
Comment 5 User image neil@parkwaycc.co.uk 2006-06-06 16:41:42 PDT
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 User image Masayuki Nakano [:masayuki] 2006-06-07 09:45:04 PDT
Neil, are there regressions by my patch in UK keyboard layout?
Comment 7 User image neil@parkwaycc.co.uk 2006-06-07 14:32:50 PDT
(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 User image Masayuki Nakano [:masayuki] 2006-06-07 22:48:46 PDT
(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 User image Masayuki Nakano [:masayuki] 2006-06-07 23:11:57 PDT
Ah, did you want to say we should not dispatch keypress event if alt++ etc?
Comment 10 User image neil@parkwaycc.co.uk 2006-06-08 01:37:02 PDT
On the 1.8 branch the Alt key doesn't affect the char code:
Key     Shift   Alt     Alt+Shift
-       _       -       _
=       +       =       +
Comment 11 User image Masayuki Nakano [:masayuki] 2006-06-08 09:04:57 PDT
Created attachment 224871 [details] [diff] [review]
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 User image Masayuki Nakano [:masayuki] 2006-06-16 09:56:06 PDT
Ere:

Would you review the patch? I think bug 341308 is serious for German people.
# This patch fixes bug 341308 too.
Comment 13 User image Ere Maijala (slow) 2006-06-19 11:50:57 PDT
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 User image Masayuki Nakano [:masayuki] 2006-06-19 13:03:47 PDT
Created attachment 226198 [details] [diff] [review]
Additional Patch rv1.1

Thank you Ere for your review.

Roc, Would you sr this?
Comment 15 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2006-06-22 03:04:27 PDT
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 User image Masayuki Nakano [:masayuki] 2006-06-22 08:38:21 PDT
(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 User image Masayuki Nakano [:masayuki] 2006-06-22 08:39:02 PDT
Oops, Roc, see comment 16.
Comment 18 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2006-06-22 16:01:39 PDT
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 User image Masayuki Nakano [:masayuki] 2006-06-23 01:39:29 PDT
(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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2006-06-25 16:44:46 PDT
(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 User image Masayuki Nakano [:masayuki] 2006-06-26 13:56:32 PDT
(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 User image Masayuki Nakano [:masayuki] 2006-06-26 14:07:11 PDT
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 User image Masayuki Nakano [:masayuki] 2006-06-27 08:14:48 PDT
Created attachment 227254 [details] [diff] [review]
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2006-07-04 22:11:35 PDT
Comment on attachment 227254 [details] [diff] [review]
Additional Patch rv1.2

This looks much better
Comment 25 User image Masayuki Nakano [:masayuki] 2006-07-04 22:54:49 PDT
Comment on attachment 227254 [details] [diff] [review]
Additional Patch rv1.2

Ere: Would you re-review this?

Thank you, roc.
Comment 26 User image Masayuki Nakano [:masayuki] 2006-07-17 02:25:19 PDT
checked-in to trunk, thanks.

now, if Alt is used without Ctrl, the key code is used as '-' or '+'.
Comment 27 User image Masayuki Nakano [:masayuki] 2006-07-17 02:31:43 PDT
Oops, the tree is closed now, I backed-out the patch.
Comment 28 User image Masayuki Nakano [:masayuki] 2006-07-17 07:09:11 PDT
re-checked-in.

-> FIXED

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