Closed
Bug 900802
Opened 11 years ago
Closed 10 years ago
WM_SYSDEADKEY ignored in some contexts
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
Tracking | Status | |
---|---|---|
firefox29 | --- | fixed |
People
(Reporter: nospam-abuse, Assigned: masayuki)
References
(Blocks 1 open bug)
Details
(Whiteboard: [qa-])
Attachments
(1 file)
542 bytes,
patch
|
masayuki
:
review-
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 2•11 years ago
|
||
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?
Assignee | ||
Comment 3•11 years ago
|
||
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
Reporter | ||
Comment 4•11 years ago
|
||
(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.]
Reporter | ||
Comment 5•11 years ago
|
||
(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).
Assignee | ||
Comment 6•11 years ago
|
||
(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.
Reporter | ||
Comment 7•11 years ago
|
||
(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.)
Assignee | ||
Comment 8•11 years ago
|
||
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.
Attachment #784755 -
Flags: review?(masayuki) → review-
Assignee | ||
Comment 9•10 years ago
|
||
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.
Assignee: nobody → masayuki
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Depends on: 962140
Resolution: --- → FIXED
Updated•10 years ago
|
Target Milestone: --- → mozilla29
status-firefox29:
--- → fixed
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•