Bug 49289 – [chromium] menu key doesn't work when capslock or numslock is on

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Evan Stade]

the menu key is ignored in certain situations, depending on modifier state. patch forthcoming.

[Evan Stade]

Created attachment 73443 [details]
try1

[WebKit Review Bot]

Attachment 73443 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit/chromium/ChangeLog', u'WebKit/chromium/public/WebInputEvent.h', u'WebKit/chromium/src/WebViewImpl.cpp']" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:580:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tony Chang]

Comment on attachment 73443 [details]
try1

View in context: https://bugs.webkit.org/attachment.cgi?id=73443&action=review

> WebKit/chromium/public/WebInputEvent.h:133
> +    static const int InputModifiers = ShiftKey | ControlKey | AltKey | MetaKey;

Nit: I don't like this name much.  Aren't capslock and numlock also input modifiers?  Should there just be a static method that returns event.modifiers & (ShiftKey | ControlKey | AltKey | MetaKey) like the other methods?

[Evan Stade]

Comment on attachment 73443 [details]
try1

View in context: https://bugs.webkit.org/attachment.cgi?id=73443&action=review

>> WebKit/chromium/public/WebInputEvent.h:133
>> +    static const int InputModifiers = ShiftKey | ControlKey | AltKey | MetaKey;
> 
> Nit: I don't like this name much.  Aren't capslock and numlock also input modifiers?  Should there just be a static method that returns event.modifiers & (ShiftKey | ControlKey | AltKey | MetaKey) like the other methods?

I can't think of a better name (really the best name is "modifier", but the enum took that already). I got this name from the getWebInputModifiers() function in WebInputEventConversion.cpp

I think it's more versatile as a bitmask but if you would rather it be an applied bitmask I can do that.

[Tony Chang]

Comment on attachment 73443 [details]
try1

View in context: https://bugs.webkit.org/attachment.cgi?id=73443&action=review

>>> WebKit/chromium/public/WebInputEvent.h:133
>>> +    static const int InputModifiers = ShiftKey | ControlKey | AltKey | MetaKey;
>> 
>> Nit: I don't like this name much.  Aren't capslock and numlock also input modifiers?  Should there just be a static method that returns event.modifiers & (ShiftKey | ControlKey | AltKey | MetaKey) like the other methods?
> 
> I can't think of a better name (really the best name is "modifier", but the enum took that already). I got this name from the getWebInputModifiers() function in WebInputEventConversion.cpp
> 
> I think it's more versatile as a bitmask but if you would rather it be an applied bitmask I can do that.

Ok, if you want to keep it as a bitmask, can you change the if below to have named bools to make it easier to read?  E.g.:
bool isUnmodifiedMenuKey = !(event.modifiers & WebInputEvent::InputModifiers) && (event.windowsKeyCode == VKEY_APPS);
bool isShiftF10 = (event.modifiers == WebInputEvent::ShiftKey) && (event.windowsKeyCode == VKEY_F10)
if ((isUnmodifiedMenuKey || isShiftF10) && event.type == contextMenuTriggeringEventType) {

Please fix the style complaint too.

[Evan Stade]

Created attachment 73521 [details]
try2

style fixed; readability suggestion implemented.

[WebKit Review Bot]

Attachment 73521 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit/chromium/ChangeLog', u'WebKit/chromium/public/WebInputEvent.h', u'WebKit/chromium/src/WebViewImpl.cpp']" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:574:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Evan Stade]

Created attachment 73529 [details]
try3

style again

[WebKit Commit Bot]

Comment on attachment 73529 [details]
try3

Clearing flags on attachment: 73529

Committed r71774: <http://trac.webkit.org/changeset/71774>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.