Closed
Bug 339723
Opened 19 years ago
Closed 19 years ago
Ctrl++ doesn't work with JIS keyboard
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
(Keywords: intl, regression)
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
masayuki
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
emaijala+moz
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•19 years ago
|
||
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•19 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+
Assignee | ||
Comment 3•19 years ago
|
||
Attachment #223825 -
Attachment is obsolete: true
Attachment #224340 -
Flags: superreview?(roc)
Attachment #224340 -
Flags: review+
Attachment #224340 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Updated•19 years ago
|
Whiteboard: Pending check-in until the tree is opened
Assignee | ||
Comment 4•19 years ago
|
||
checked-in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: Pending check-in until the tree is opened
Comment 5•19 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.
Assignee | ||
Comment 6•19 years ago
|
||
Neil, are there regressions by my patch in UK keyboard layout?
Comment 7•19 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 → ---
Assignee | ||
Comment 8•19 years ago
|
||
(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
Assignee | ||
Comment 9•19 years ago
|
||
Ah, did you want to say we should not dispatch keypress event if alt++ etc?
Comment 10•19 years ago
|
||
On the 1.8 branch the Alt key doesn't affect the char code:
Key Shift Alt Alt+Shift
- _ - _
= + = +
Assignee | ||
Comment 11•19 years ago
|
||
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•19 years ago
|
Assignee | ||
Comment 12•19 years ago
|
||
Ere:
Would you review the patch? I think bug 341308 is serious for German people.
# This patch fixes bug 341308 too.
Comment 13•19 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+
Assignee | ||
Comment 14•19 years ago
|
||
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+
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.
Assignee | ||
Comment 16•19 years ago
|
||
(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...
Assignee | ||
Comment 17•19 years ago
|
||
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?
Assignee | ||
Comment 19•19 years ago
|
||
(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.
Assignee | ||
Comment 21•19 years ago
|
||
(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.
Assignee | ||
Comment 22•19 years ago
|
||
Ah, but I have an idea. It may removes the *textzoom* special case from the code. But you may hate it too. Please wait.
Assignee | ||
Comment 23•19 years ago
|
||
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+
Assignee | ||
Comment 25•19 years ago
|
||
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•19 years ago
|
Attachment #227254 -
Flags: review?(emaijala) → review+
Assignee | ||
Comment 26•19 years ago
|
||
checked-in to trunk, thanks.
now, if Alt is used without Ctrl, the key code is used as '-' or '+'.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 27•19 years ago
|
||
Oops, the tree is closed now, I backed-out the patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 28•19 years ago
|
||
re-checked-in.
-> FIXED
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
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.
Description
•