NativeKey::mCommittedCharsAndModifiers should be computed with following WM_CHAR messages as far as possible

RESOLVED FIXED in Firefox 52

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({intl})

Trunk
mozilla52
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 affected, firefox52 fixed)

Details

(Whiteboard: tpi:+)

Attachments

(9 attachments)

58 bytes, text/x-review-board-request
m_kato
: review+
Details
58 bytes, text/x-review-board-request
m_kato
: review+
Details
58 bytes, text/x-review-board-request
m_kato
: review+
Details
58 bytes, text/x-review-board-request
m_kato
: review+
Details
58 bytes, text/x-review-board-request
m_kato
: review+
Details
58 bytes, text/x-review-board-request
m_kato
: review+
Details
58 bytes, text/x-review-board-request
m_kato
: review+
Details
58 bytes, text/x-review-board-request
m_kato
: review+
Details
58 bytes, text/x-review-board-request
m_kato
: review+
Details
NativeKey::mCommittedCharsAndModifiers is used for setting KeyboardEvent.key value of eKeyDown event and each KeyboardEvent.charCode value of eKeyPress events.  So, this is what will be inputted into editor if eKeyPress events cause some text.

However, it's initialized with KeyboardLayout::InitNativeKey() with KeyboardLayout's information in some cases.  Unfortunately, KeyboardLayout class doesn't support unusual KeyboardLayout, e.g., much complicated dead key sequence, known as non-printable keys mapped as printable keys, etc.  Therefore, changing NativeKey is now very risky for such special keyboard layout users.

If NativeKey decides mCommittedCharsAndModifiers only from following WM_CHAR messages, it becomes safer at least for inputting text.

After fixing bug 1300003, we can do this because NativeKey will store whole following WM_CHAR messages for a WM_KEYDOWN.
Priority: -- → P2
Whiteboard: tpi:+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

3 years ago
mozreview-review
Comment on attachment 8798869 [details]
Bug 1303273 part.1 KeyboardLayout::IsSysKey() should check MODIFIER_ALT as a bit flag

https://reviewboard.mozilla.org/r/84242/#review83310

::: widget/windows/KeyboardLayout.cpp:3542
(Diff revision 1)
>    }
>  
>    // If the Alt key state isn't consumed, that means that the key with Alt
>    // doesn't cause text input.  So, the combination is a system key.
> -  return inputCharsAndModifiers.mModifiers[0] != MODIFIER_ALT;
> +  return (inputCharsAndModifiers.mModifiers[0] & MODIFIER_ALT) != 0;
>  }

nit: Use "!!(inputCharsAndModifiers.mModifiers[0] & MODIFIER_ALT)" insted.
Attachment #8798869 - Flags: review?(m_kato) → review+
Comment on attachment 8798869 [details]
Bug 1303273 part.1 KeyboardLayout::IsSysKey() should check MODIFIER_ALT as a bit flag

https://reviewboard.mozilla.org/r/84242/#review83310

> nit: Use "!!(inputCharsAndModifiers.mModifiers[0] & MODIFIER_ALT)" insted.

Okay, fixed locally. (I'd like it to be in coding style rules because I've changed it from |!!| to |!= 0| in other bugs, though...)

Comment 16

3 years ago
mozreview-review
Comment on attachment 8798870 [details]
Bug 1303273 part.2 KeyboardLayout::InitNativeKey() should initialize aNativeKey.mCommittedCharsAndModifiers with following WM_CHAR or WM_SYSCHAR messages which are not providing a control character

https://reviewboard.mozilla.org/r/84244/#review83346
Attachment #8798870 - Flags: review?(m_kato) → review+

Comment 17

3 years ago
mozreview-review
Comment on attachment 8798871 [details]
Bug 1303273 part.3 Dispatch eKeyPress events without NativeKey::HandleCharMessage() when it handles WM_(SYS)KEYDOWN message and there are following WM_(SYS)CHAR messages which includes non-control character

https://reviewboard.mozilla.org/r/84246/#review83350
Attachment #8798871 - Flags: review?(m_kato) → review+

Comment 18

3 years ago
mozreview-review
Comment on attachment 8798872 [details]
Bug 1303273 part.4 Add automated tests for bug 1293505, bug 1307703 and bug 1297985

https://reviewboard.mozilla.org/r/84248/#review83368
Attachment #8798872 - Flags: review?(m_kato) → review+

Comment 19

3 years ago
mozreview-review
Comment on attachment 8798873 [details]
Bug 1303273 part.5 UniCharsAndModifiers should hide mChars

https://reviewboard.mozilla.org/r/84250/#review83380
Attachment #8798873 - Flags: review?(m_kato) → review+

Comment 20

3 years ago
mozreview-review
Comment on attachment 8798874 [details]
Bug 1303273 part.6 Hide UniCharsAndModifiers::mModifiers

https://reviewboard.mozilla.org/r/84252/#review83388
Attachment #8798874 - Flags: review?(m_kato) → review+

Comment 21

3 years ago
mozreview-review
Comment on attachment 8798875 [details]
Bug 1303273 part.7 Hide UniCharsAndModifiers::mLength and make its type size_t

https://reviewboard.mozilla.org/r/84254/#review83394
Attachment #8798875 - Flags: review?(m_kato) → review+

Comment 22

3 years ago
mozreview-review
Comment on attachment 8798876 [details]
Bug 1303273 part.8 UniCharsAndModifiers should use nsAutoString and AutoTArray to store characters and modifiers

https://reviewboard.mozilla.org/r/84256/#review83398
Attachment #8798876 - Flags: review?(m_kato) → review+

Comment 23

3 years ago
mozreview-review
Comment on attachment 8798877 [details]
Bug 1303273 part.9 NativeKey::mFollowingCharMsgs should be AutoTArray

https://reviewboard.mozilla.org/r/84258/#review83400
Attachment #8798877 - Flags: review?(m_kato) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

3 years ago
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/a3446be6bb98
part.1 KeyboardLayout::IsSysKey() should check MODIFIER_ALT as a bit flag r=m_kato
https://hg.mozilla.org/integration/autoland/rev/259115245f85
part.2 KeyboardLayout::InitNativeKey() should initialize aNativeKey.mCommittedCharsAndModifiers with following WM_CHAR or WM_SYSCHAR messages which are not providing a control character r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4e3211aa00ab
part.3 Dispatch eKeyPress events without NativeKey::HandleCharMessage() when it handles WM_(SYS)KEYDOWN message and there are following WM_(SYS)CHAR messages which includes non-control character r=m_kato
https://hg.mozilla.org/integration/autoland/rev/55817ab6a405
part.4 Add automated tests for bug 1293505, bug 1307703 and bug 1297985 r=m_kato
https://hg.mozilla.org/integration/autoland/rev/24569a596578
part.5 UniCharsAndModifiers should hide mChars r=m_kato
https://hg.mozilla.org/integration/autoland/rev/816a22bf25dd
part.6 Hide UniCharsAndModifiers::mModifiers r=m_kato
https://hg.mozilla.org/integration/autoland/rev/65d72f0e62aa
part.7 Hide UniCharsAndModifiers::mLength and make its type size_t r=m_kato
https://hg.mozilla.org/integration/autoland/rev/f34b67c757f7
part.8 UniCharsAndModifiers should use nsAutoString and AutoTArray to store characters and modifiers r=m_kato
https://hg.mozilla.org/integration/autoland/rev/927bfc865b55
part.9 NativeKey::mFollowingCharMsgs should be AutoTArray r=m_kato
Duplicate of this bug: 1320499

Updated

3 years ago
Duplicate of this bug: 1323028

Updated

2 years ago
Depends on: 1346499
You need to log in before you can comment on or make changes to this bug.