900802 - WM_SYSDEADKEY ignored in some contexts

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Ilya Zakharevich]

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

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

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

416         if (WinUtils::PeekMessage(&msg, mMsg.hwnd, WM_KEYFIRST, WM_KEYLAST,
417                                   PM_NOREMOVE | PM_NOYIELD) &&
418             (msg.message == WM_CHAR || msg.message == WM_SYSCHAR ||
419              msg.message == WM_DEADCHAR)) {

This misses the case of WM_SYSDEADCHAR.

Recall that WM_SYSDEADCHAR is in the same relation to WM_DEADCHAR as WM_SYSCHAR is to WM_CHAR.

Comment 1

[Ilya Zakharevich]

Attachment: diff_sysdeadchar should fix the issue

Untested.

Comment 2

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

Comment on attachment 784755 [details] [diff] [review]
diff_sysdeadchar should fix the issue

I think that you need to change here:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#566

Additionally, we need to change KeyboardLayout::SynthesizeNativeKeyEvent() too because it not uses WM_SYS* message even if Alt key is down. But this is not in the scope of this bug.

And please test at least with debugger if you cannot test it visually. Why can we trust untested patch?

Comment 3

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

I think that following lines need to check WM_SYSDEADCHAR too.

http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#1135
http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#1324
http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#1353
http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#1363

Comment 4

[Ilya Zakharevich]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 8/13-8/18 JST) from comment #2)

> I think that you need to change here:
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.
> cpp#566

This code looks like this now:


580   mIsDeadKey =
581     (mCharMsg.message == WM_DEADCHAR ||
582      keyboardLayout->IsDeadKey(mOriginalVirtualKeyCode, mModKeyState));

This exposes one of the MAJOR problems with the current Mozilla code: it is mixing (correct) processing of character-bringing (or “eventually”-character-bringing, as in deadKeys) key-events with (wrong, and unneeded) heuristics.  I think the code in question should look just

  (mCharMsg.message == WM_DEADCHAR || mCharMsg.message == WM_SYSDEADCHAR);

The (heuristic) information in keyboardLayout should NEVER be used for processing characters carried by keypresses.

[But to be absolutely sure, one should look at all the places this code is used, since removal of dependency on keyboardLayout should be done consistently in the win32-specific code.  I did do a global analysis in April, but the code has changed from this time.]

> And please test at least with debugger if you cannot test it visually. Why
> can we trust untested patch?

As far as I can recall, I explicitly write that my patches are untested.  (And I will not be able to test them.)  So it is not clear why the question of trust enters the discussion — of course one should not trust my patches blindly.

The patches are just “another way to illustrate the problems I see”, and are provided as a courtesy to Olli (who said

  And patches are always very welcome ;)
).

[I did not yet look at code locations mentioned in Comment 3.]

Comment 5

[Ilya Zakharevich]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> I think that following lines need to check WM_SYSDEADCHAR too.
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#1135
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#1324
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#1353
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#1363

The first 3: yes.  The fourth one should not (the case of WM_SYSDEADCHAR would be covered in the 3rd chunk already).

Comment 6

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

(In reply to Ilya Zakharevich from comment #4)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 8/13-8/18
> JST) from comment #2)
> > And please test at least with debugger if you cannot test it visually. Why
> > can we trust untested patch?
> 
> As far as I can recall, I explicitly write that my patches are untested. 
> (And I will not be able to test them.)  So it is not clear why the question
> of trust enters the discussion — of course one should not trust my patches
> blindly.
> 
> The patches are just “another way to illustrate the problems I see”, and are
> provided as a courtesy to Olli (who said
> 
>   And patches are always very welcome ;)
> ).

I believe that "writing patch" includes to test the behavior. If we accept untested patch, it may just cause regression without any improvements actually.

I agree with your claim about at least this bug, but I still don't check your claim in other bugs you reported. Your reports look like you found them from source code rather than actual wrong behavior. So, you should confirm if the bug actually causes wrong behavior and report the STR ("Step To Reproduce") clearly.

Comment 7

[Ilya Zakharevich]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> I believe that "writing patch" includes to test the behavior. If we accept
> untested patch, it may just cause regression without any improvements
> actually.

Did I say anything about “accepting untested patch”?!  All I said is that my patch is untested!

I believe that I should not tell you that

◌ Recognizing that a certain behaviour is a bug;
◌ Designing a reproducible scenario to demonstrate this behaviour;
◌ Finding which part of the code causes the bug;
◌ Finding how to fix this part of the code;
◌ Actually fixing this part of the code;
◌ Designing a test suite to detect this bug;
◌ Making the fix pass the test suite

are distinct steps in the lifetime of a bug, that these steps
require distinct skills and distinct development environment,
and may (or should?) be done by different people.

I was asked to make a patch (which means the third step above).  This is what I did.
As I said, I will not be able to do the last 3 steps.

> I agree with your claim about at least this bug, but I still don't check
> your claim in other bugs you reported. Your reports look like you found them
> from source code rather than actual wrong behavior.

As I said many times: both.  The actual wrong behaviour (see
  http://search.cpan.org/~ilyaz/UI-KeyboardLayout/lib/UI/KeyboardLayout.pm#Firefox_misinterprets_keypresses
) led to source code analysis, which led to bug reports (some
of them are related to reported above bugs, some are yet other
regressions).

> So, you should confirm
> if the bug actually causes wrong behavior and report the STR ("Step To
> Reproduce") clearly.

Since this is not needed for this particular bug, there should be no “should”
in your sentence.  STR are not a must for a bug report.

(Or do you say you do not know how to create WM_SYSDEADCHAR?
Combine Alt with a dead character which does not involve other modifiers than
Shift/Kana; e.g. Alt-" on US International keyboard should do.  One should be
able to use Alt-" Alt-a to access a menu entry which has ä underlined.)

Comment 8

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

Comment on attachment 784755 [details] [diff] [review]
diff_sysdeadchar should fix the issue

We don't accept any patches which nobody tests if it actually fixes the bug. It's wrong to request the test to reviewer. Any developers should test any patches which are written by themselves!

If you test it with the latest mozilla-central, please request review again. Then, I'll give r+ for it.

Comment 9

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

This was fixed by the first patch of bug 962140. When I tested it with debug build, pressing twice Alt+dead key causes crash by MOZ_ASSERT(), though. I'll file a follow up bug for it.