Ctrl++ doesn't work with JIS keyboard

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Widget: Win32
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({intl, regression})

Trunk
mozilla1.9alpha1
x86
Windows 2000
intl, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
Status: NEW → ASSIGNED
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.
Attachment #223825 - Flags: review?(emaijala)

Comment 2

11 years ago
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.
Attachment #223825 - Flags: review?(emaijala) → review+
Created attachment 224340 [details] [diff] [review]
Patch rv1.1 [checked in]
Attachment #223825 - Attachment is obsolete: true
Attachment #224340 - Flags: superreview?(roc)
Attachment #224340 - Flags: review+
Attachment #224340 - Flags: superreview?(roc) → superreview+
Whiteboard: Pending check-in until the tree is opened
checked-in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: Pending check-in until the tree is opened

Comment 5

11 years ago
VK_ADD is the + key on the numeric keyboard.
VK_OEM_PLUS is the =/+ key to the left of Backspace on a UK keybord.
Neil, are there regressions by my patch in UK keyboard layout?

Comment 7

11 years ago
(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
-	_	-	-
=	+	+	+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?
Status: REOPENED → ASSIGNED
Ah, did you want to say we should not dispatch keypress event if alt++ etc?

Comment 10

11 years ago
On the 1.8 branch the Alt key doesn't affect the char code:
Key     Shift   Alt     Alt+Shift
-       _       -       _
=       +       =       +
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.
Attachment #224340 - Attachment is obsolete: true
Attachment #224871 - Flags: review?(emaijala)

Updated

11 years ago
Depends on: 341308

Updated

11 years ago
Blocks: 341308
No longer depends on: 341308
Ere:

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

Comment 13

11 years ago
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.
Attachment #224871 - Flags: review?(emaijala) → review+
Created attachment 226198 [details] [diff] [review]
Additional Patch rv1.1

Thank you Ere for your review.

Roc, Would you sr this?
Attachment #224871 - Attachment is obsolete: true
Attachment #226198 - Flags: superreview?(roc)
Attachment #226198 - Flags: review+

Updated

11 years ago
Blocks: 342197
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.
(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...
Oops, Roc, see comment 16.
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?
(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.)
(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.
(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.
Ah, but I have an idea. It may removes the *textzoom* special case from the code. But you may hate it too. Please wait.
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.)
Attachment #226198 - Attachment is obsolete: true
Attachment #227254 - Flags: superreview?(roc)
Attachment #226198 - Flags: superreview?(roc)
Comment on attachment 227254 [details] [diff] [review]
Additional Patch rv1.2

This looks much better
Attachment #227254 - Flags: superreview?(roc) → superreview+
Comment on attachment 227254 [details] [diff] [review]
Additional Patch rv1.2

Ere: Would you re-review this?

Thank you, roc.
Attachment #227254 - Flags: review?(emaijala)

Updated

11 years ago
Attachment #227254 - Flags: review?(emaijala) → review+
checked-in to trunk, thanks.

now, if Alt is used without Ctrl, the key code is used as '-' or '+'.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Oops, the tree is closed now, I backed-out the patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
re-checked-in.

-> FIXED
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Depends on: 359638
Attachment #224340 - Attachment description: Patch rv1.1 → Patch rv1.1 [checked in]
Attachment #224340 - Attachment is obsolete: false
You need to log in before you can comment on or make changes to this bug.