Currently only KeyDown/KeyUp/RawKeyDown are allowed. Char events should be allowed for processing space key events of type Char.
Created attachment 135938 [details] Patch
What is the rationale for this change? Is there any spec for this?
(In reply to comment #2) > What is the rationale for this change? Is there any spec for this? According to PlatformKeyboardEvent.h, PlatformKeyboardEvent::windowsVirtualKeyCode() returns zero for Char events. Therefore, space events of type Char are currently not allowed in fullscreen mode. They should be allowed, because there are tests that expect this and Firefox also does this. However, these tests that seem to check space key events currently pass on Chromium because Chromium doesn't implement windowsVirtualKeyCode as specified in PlatformKeyboardEvent.h. I have not investigated how it works in other ports. Perhaps the windowsVirtualKeyCode() specification in the header is incorrect?
Comment on attachment 135938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135938&action=review It is expected behavior that keypress events (corresponding to Char) have a character code, but no virtual key code. It's quite unfortunate that information about key bindings is hardcoded here. In fact, existing code below makes me quite suspicious (why VK_BACK? why VK_OEM_1?) > Source/WebCore/page/EventHandler.cpp:2728 > + char character = keyEvent.text()[0]; This character is Unicode, so converting to char loses high bits. Also, this should check that length of text is 1.
(In reply to comment #4) > (From update of attachment 135938 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135938&action=review > > It is expected behavior that keypress events (corresponding to Char) have a character code, but no virtual key code. In that case, is PlatformKeyboardEvent.h inaccurate? Or by "no virtual key code" do you mean a windowsVirtualKeyCode value of zero? > It's quite unfortunate that information about key bindings is hardcoded here. In fact, existing code below makes me quite suspicious (why VK_BACK? why VK_OEM_1?) Without VK_BACK, backspace on textboxes do not work and takes us back to the previous page. Without VK_OEM_1, I think the semicolon key wouldn't work. > > Source/WebCore/page/EventHandler.cpp:2728 > > + char character = keyEvent.text()[0]; > > This character is Unicode, so converting to char loses high bits. Also, this should check that length of text is 1. Can we not assume that the string always has a null character? If windowsVirtualKeyCode is supposed to give us the char code, should we use it instead of ".text()[0]"? I'll fix these and character type and resubmit a new patch, if these are the reasons for the r-. Please let me know if there's any particular refactoring that you think should be done.
> Or by "no virtual key code" do you mean a windowsVirtualKeyCode value of zero? I mean that text events such as keypress don't have a virtual key code conceptually - they may not even be initiated by a keyboard. At code level, that corresponds to a zero value. > Without VK_BACK, backspace on textboxes do not work and takes us back to the previous page. Without VK_OEM_1, I think the semicolon key wouldn't work. Why is the semicolon expected to work, in the first place? Backspace is an example of why this is a very poor place to hardcode key behaviors - it may or may not cause a navigation, but that's decided elsewhere. > Can we not assume that the string always has a null character? I'm not sure about that, but I know that it can be longer than 1 character.
Created attachment 136163 [details] Patch
(In reply to comment #6) > Why is the semicolon expected to work, in the first place? You're right, I can't see a reason for that either. > > Can we not assume that the string always has a null character? > > I'm not sure about that, but I know that it can be longer than 1 character. I changed the type to UChar and it checks the length now. Could you review this patch so we can fix the current implementation?
Jer Noble is our fullscreen expert, I'd like him to do review. Please mark the patch as r? to make sure it doesn't get overlooked.
(In reply to comment #9) > Jer Noble is our fullscreen expert, I'd like him to do review. > > Please mark the patch as r? to make sure it doesn't get overlooked. This looks fine, though the next revision of the W3C Full Screen spec does not include any keyboard limitations (all key press events will be supported in full screen). So it's likely this code will eventually be obsoleted. In the meantime, this patch should be fine.
Comment on attachment 136163 [details] Patch Clearing flags on attachment: 136163 Committed r114024: <http://trac.webkit.org/changeset/114024>
All reviewed patches have been landed. Closing bug.