Open
Bug 1136539
Opened 10 years ago
Updated 2 years ago
All event handlers in C++ code should handle non-printable key events with WidgetKeyboardEvent::mKeyNameIndex instead of keyCode
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Core
DOM: UI Events & Focus Handling
Tracking
()
NEW
People
(Reporter: masayuki, Unassigned)
References
Details
.keyCode cannot represent new keys including multimedia keys because its compatibility with other browsers doesn't allow that and its spec won't standardized forever.
Instead, .key value will support new keys. In fact, some keys which are handled by Gecko or its product can be handled only with .key value.
In the future, we may need to handle it with .key value. In such cases, we would need two switch statements, one is for .keyCode, the other is for .key if we wouldn't rewrite all key event handlers with .key value as soon as possible.
First of all, all automated tests should generate key events with proper key values. (bug 1134539) And also we need to drop legacy key event dispatcher of nsIDOMWindowUtils (bug 1134542).
Comment 1•6 years ago
|
||
Hello. Has this bug been fixed? If not, I would like to try and work on it.
Reporter | ||
Comment 2•6 years ago
|
||
(In reply to P Kausthubh S from comment #1)
> Hello. Has this bug been fixed? If not, I would like to try and work on it.
Of course, welcome. But perhaps, there are a lot of event handlers and if they listen to untrusted events, we cannot change them. I recommend that you should file separated bug which blocks this bug and work on it.
Comment 3•6 years ago
|
||
So first, I have to file a separate bug blocking this bug. Then, I have to look for switch statements on .keycode and include another switch statement on .key (with the same statements inside the switch?). Correct me if I'm wrong, please - this is just my second bug :)
Reporter | ||
Comment 4•6 years ago
|
||
Right. You don't need to file a lot of bugs once. You should look for a keydown, keypress or keyup event handler which handles only trusted events and then, you file a bug for one of them.
> this is just my second bug :)
Do you have a permission to push tryserver? (It's difficult to run all tests locally.) If you don't have it, I'll do it for you when you request a review to me.
Oh, and I realized that...
> all automated tests should generate key events with proper key values. (bug 1134539)
Blocking issue of this bug was almost fixed by bug 1119609. However, when you fix a blocker bug, you *might* need to update some mochitests.
Comment 5•6 years ago
|
||
The C++ code we are concerned with is in ~/mozilla-central/widgets right?
Flags: needinfo?(masayuki)
Reporter | ||
Comment 6•6 years ago
|
||
No, mainly in dom/, editor/ or layout/.
https://searchfox.org/mozilla-central/search?q=AddEventListener%5BA-Za-z%5D*%5C(&case=false®exp=true&path=.cpp
Flags: needinfo?(masayuki)
Comment 7•6 years ago
|
||
I've looked at some of the files from the searchfox link and there appear to be no switch statements on "keycode". When I search for "keycode", however, I see some switch cases on some form of keycode (mKeyCode or AKEYCODE). Also, the definition of the "AddEventListener" functions involve neither "keycode" nor a switch on any variable. Shouldn't I be looking at these pieces of code instead of the ones given in the above searchfox link? I just want to be sure I'm doing the right thing before I begin :)
Reporter | ||
Comment 8•6 years ago
|
||
How about https://searchfox.org/mozilla-central/search?q=symbol%3AF_%3CT_mozilla%3A%3AWidgetKeyboardEvent%3E_mKeyCode&case=false®exp=false&path= ? (Ignore under widget/ since they are setter.)
Comment 9•6 years ago
|
||
Just looked at all the results for the above search,and there seem to be 9 switch cases on mKeyCode, as part of different objects and/or pointers. Also, in dom/events/EventStateManager.cpp, line 2878 says :
// XXX Currently, our automated tests don't support mKeyNameIndex.
// Therefore, we still need to handle this with keyCode.
switch (aKeyboardEvent->mKeyCode) {
Does the mKeyNameIndex variable referred to over here supposed to serve the same purpose as .key value (as referred to by you in the bug description)? If so, will we have to manually test the additional switch case on mKeyNameIndex?
Flags: needinfo?(masayuki)
Reporter | ||
Comment 10•6 years ago
|
||
> Does the mKeyNameIndex variable referred to over here supposed to serve the same purpose as .key value (as referred to by you in the bug description)?
Yes, mKeyNameIndex indicates that if the key is one of declared names by https://w3c.github.io/uievents-key/ or KEY_NAME_INDEX_USE_STRING otherwise (e.g., when inputting a character). In the later case, mKeyValue is the value exposed to the web.
> If so, will we have to manually test the additional switch case on mKeyNameIndex?
No, see comment 4. Currently, our test API computes proper .key value from .keyCode value even if the API caller does not specify the .key value. Therefore, now, the comment should've been outdated.
Flags: needinfo?(masayuki)
Assignee | ||
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•