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

Evan Stade
Reported 2010-11-09 17:21:29 PST
the menu key is ignored in certain situations, depending on modifier state. patch forthcoming.
Attachments
try1 (2.40 KB, patch)
2010-11-09 17:26 PST, Evan Stade
tony: review-
try2 (2.62 KB, patch)
2010-11-10 11:59 PST, Evan Stade
no flags
try3 (2.61 KB, patch)
2010-11-10 12:56 PST, Evan Stade
no flags
Evan Stade
Comment 1 2010-11-09 17:26:55 PST
WebKit Review Bot
Comment 2 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.
Tony Chang
Comment 3 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?
Evan Stade
Comment 4 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.
Tony Chang
Comment 5 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.
Evan Stade
Comment 6 2010-11-10 11:59:18 PST
Created attachment 73521 [details] try2 style fixed; readability suggestion implemented.
WebKit Review Bot
Comment 7 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.
Evan Stade
Comment 8 2010-11-10 12:56:02 PST
Created attachment 73529 [details] try3 style again
WebKit Commit Bot
Comment 9 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>
WebKit Commit Bot
Comment 10 2010-11-10 14:48:00 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.