900814 - Characters produced with Ctrl xor Alt should be dropped only on as-needed basis

Status: UNCONFIRMED

(Core :: Widget: Win32, defect)

Description

[Ilya Zakharevich]

This is a split of https://bugzilla.mozilla.org/show_bug.cgi?id=880971#c4.

I continue discussion of the source code in KeyboardLayout.cpp.

http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#920

920   // Alt+Space key is handled by OS, we shouldn't touch it.
921   if (mModKeyState.IsAlt() && !mModKeyState.IsControl() &&
922       mVirtualKeyCode == VK_SPACE) {
923     return false;
924   }
925 
926   // Bug 818235: Ignore Ctrl+Enter.
927   if (!mModKeyState.IsAlt() && mModKeyState.IsControl() &&
928       mVirtualKeyCode == VK_RETURN) {
929     return false;
930   }

Let me recall again that doing decisions based on IsAlt() and IsControl() alone is not possible.  There may be other modifiers not visible via mModKeyState.  One should drop messages only if the Unicode character they are delivering is the expected one (above, that would be SPACE or '\n' or '\r').

Comment 1

[Ilya Zakharevich]

Attachment: diff_drop_special should address the special cases mentioned in this bug report

Untested.

This patch addresses only two special cases in the code.  ALL the special cases should be removed, and should be covered by general logic.

For example, Control-(Shift-)keys produce characters 0x00..0x20 and 0x7F which should be dropped.  All the rest of Control-(Shift-)keys should be preserved.

Comment 2

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

Comment on attachment 784775 [details] [diff] [review]
diff_drop_special should address the special cases mentioned in this bug report

Do you think that this is possible case? I don't think so...

However, if your concern is correct, it looks good to me, but please wrap the long lines. One line must be less than 80 characters.

Comment 3

[Ilya Zakharevich]

> Do you think that this is possible case? I don't think so...

It may be that this code is never touched, and these combinations are already processed earlier.  But then the code should be better removed.

[Currently, my keyboards do not produce any characters on these locations (I mean: Control-!Alt and Alt-!Control with extra modifiers not visible via CapsLock/Shift/Alt/Ctrl model), so I cannot check whether this code is reachable.]

Comment 4

[Ilya Zakharevich]

Attachment: should address the special cases mentioned in this bug report

Comment 5

[Ilya Zakharevich]

All that the attachment #791039 [details] [diff] [review] is changing is line wrap in the comments

Comment 6

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

Comment on attachment 791039 [details] [diff] [review]
should address the special cases mentioned in this bug report

Okay, but please remove bug number, which should be found from hg annotation tool. And don't indent the second line and I don't think the () are not necessary.

-  // Alt+Space key is handled by OS, we shouldn't touch it.
+  // Alt+Space key is handled by OS, we shouldn't touch it but only if it is
+  // producing the expected char.

and

-  // Bug 818235: Ignore Ctrl+Enter.
+  // Ignore Ctrl+Enter but only if it is producing the expected char.

are enough.

And could you attach the patch after do:

hg qrefresh -u "your name<your email>" -m "summary of the change" && hg qcommit -m "backup"

The -u option is your name and your email address. The -m is the summary of the patch. I think "Bug 900814 WM_CHAR message with Ctrl or Alt modifier shouldn't be ignored when it causes text input actually r=masayuki" is good enough.

Then, .hg/patches has a patch which include your name and the summary. Please attach it to this bug. I'll land it (I guess you don't have a permission to land it).