Bug 49289

Summary: [chromium] menu key doesn't work when capslock or numslock is on
Product: WebKit Reporter: Evan Stade <estade>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
try1
tony: review-
try2
none
try3 none

Description Evan Stade 2010-11-09 17:21:29 PST
the menu key is ignored in certain situations, depending on modifier state. patch forthcoming.
Comment 1 Evan Stade 2010-11-09 17:26:55 PST
Created attachment 73443 [details]
try1
Comment 2 WebKit Review Bot 2010-11-09 17:30:23 PST
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.
Comment 3 Tony Chang 2010-11-09 17:39:46 PST
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?
Comment 4 Evan Stade 2010-11-09 18:16:30 PST
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.
Comment 5 Tony Chang 2010-11-10 09:56:53 PST
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.
Comment 6 Evan Stade 2010-11-10 11:59:18 PST
Created attachment 73521 [details]
try2

style fixed; readability suggestion implemented.
Comment 7 WebKit Review Bot 2010-11-10 12:01:11 PST
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.
Comment 8 Evan Stade 2010-11-10 12:56:02 PST
Created attachment 73529 [details]
try3

style again
Comment 9 WebKit Commit Bot 2010-11-10 14:47:55 PST
Comment on attachment 73529 [details]
try3

Clearing flags on attachment: 73529

Committed r71774: <http://trac.webkit.org/changeset/71774>
Comment 10 WebKit Commit Bot 2010-11-10 14:48:00 PST
All reviewed patches have been landed.  Closing bug.