83330 2012-04-05 16:55:41 -0700 Allow certain Char events in fullscreen 2012-04-12 13:23:50 -0700 1 1 1 Unclassified WebKit UI Events 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 83014 0 cem.kocagil+webkit webkit-unassigned ap cem.kocagil+webkit darin jer.noble rniwa sam webkit.review.bot oldest_to_newest 597035 0 cem.kocagil+webkit 2012-04-05 16:55:41 -0700 Currently only KeyDown/KeyUp/RawKeyDown are allowed. Char events should be allowed for processing space key events of type Char. 597038 1 135938 cem.kocagil+webkit 2012-04-05 16:59:23 -0700 Created attachment 135938 Patch 597446 2 rniwa 2012-04-06 10:17:46 -0700 What is the rationale for this change? Is there any spec for this? 597463 3 cem.kocagil+webkit 2012-04-06 10:32:36 -0700 (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? 597500 4 135938 ap 2012-04-06 11:16:49 -0700 Comment on attachment 135938 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. 597606 5 cem.kocagil+webkit 2012-04-06 13:25:18 -0700 (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. 597819 6 ap 2012-04-06 16:58:26 -0700 > 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. 598075 7 136163 cem.kocagil+webkit 2012-04-08 17:06:09 -0700 Created attachment 136163 Patch 598078 8 cem.kocagil+webkit 2012-04-08 17:28:49 -0700 (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? 598097 9 ap 2012-04-08 18:54:46 -0700 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. 598313 10 jer.noble 2012-04-09 09:11:00 -0700 (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. 601474 11 136163 webkit.review.bot 2012-04-12 13:23:45 -0700 Comment on attachment 136163 Patch Clearing flags on attachment: 136163 Committed r114024: <http://trac.webkit.org/changeset/114024> 601475 12 webkit.review.bot 2012-04-12 13:23:50 -0700 All reviewed patches have been landed. Closing bug. 135938 2012-04-05 16:59:23 -0700 2012-04-08 17:06:09 -0700 Patch fullscreen.patch text/plain 1305 cem.kocagil+webkit SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZw0KPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIFNvdXJjZS9X ZWJDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTEzMzk1KQorKysgU291cmNlL1dlYkNvcmUvQ2hh bmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTItMDQtMDUgIENlbSBL b2NhZ2lsICA8Y2VtLmtvY2FnaWxAZ21haWwuY29tPgorCisgICAgICAgIEFsbG93IGNlcnRhaW4g Q2hhciBldmVudHMgaW4gZnVsbHNjcmVlbgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9ODMzMzAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICAqIHBhZ2UvRXZlbnRIYW5kbGVyLmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OkV2ZW50SGFuZGxlcjo6aXNLZXlFdmVudEFsbG93ZWRJbkZ1bGxTY3JlZW4pOgorCiAyMDEy LTA0LTA1ICBKb3NodWEgQmVsbCAgPGpzYmVsbEBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgSW5k ZXhlZERCOiBTdXBwb3J0IHN0cmluZy5sZW5ndGggaW4ga2V5UGF0aHMKSW5kZXg6IFNvdXJjZS9X ZWJDb3JlL3BhZ2UvRXZlbnRIYW5kbGVyLmNwcA0KPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIFNvdXJjZS9XZWJD b3JlL3BhZ2UvRXZlbnRIYW5kbGVyLmNwcAkocmV2aXNpb24gMTEzMzkyKQorKysgU291cmNlL1dl YkNvcmUvcGFnZS9FdmVudEhhbmRsZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yNzI0LDYgKzI3 MjQsMTEgQEAgYm9vbCBFdmVudEhhbmRsZXI6OmlzS2V5RXZlbnRBbGxvd2VkSW5GdQogICAgIGlm IChkb2N1bWVudC0+d2Via2l0RnVsbFNjcmVlbktleWJvYXJkSW5wdXRBbGxvd2VkKCkpCiAgICAg ICAgIHJldHVybiB0cnVlOwogCisgICAgaWYgKGtleUV2ZW50LnR5cGUoKSA9PSBQbGF0Zm9ybUtl eWJvYXJkRXZlbnQ6OkNoYXIpIHsKKyAgICAgICAgY2hhciBjaGFyYWN0ZXIgPSBrZXlFdmVudC50 ZXh0KClbMF07CisgICAgICAgIHJldHVybiBjaGFyYWN0ZXIgPT0gJyAnOworICAgIH0KKwogICAg IGludCBrZXlDb2RlID0ga2V5RXZlbnQud2luZG93c1ZpcnR1YWxLZXlDb2RlKCk7CiAgICAgcmV0 dXJuIChrZXlDb2RlID49IFZLX0JBQ0sgJiYga2V5Q29kZSA8PSBWS19DQVBJVEFMKQogICAgICAg ICB8fCAoa2V5Q29kZSA+PSBWS19TUEFDRSAmJiBrZXlDb2RlIDw9IFZLX0RFTEVURSkK 136163 2012-04-08 17:06:09 -0700 2012-04-12 13:23:45 -0700 Patch windowsvk.patch text/plain 1376 cem.kocagil+webkit SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZw0KPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIFNvdXJjZS9X ZWJDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTEzNTUwKQorKysgU291cmNlL1dlYkNvcmUvQ2hh bmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTItMDQtMDcgIENlbSBL b2NhZ2lsICA8Y2VtLmtvY2FnaWxAZ21haWwuY29tPgorCisgICAgICAgIEFsbG93IGNlcnRhaW4g Q2hhciBldmVudHMgaW4gZnVsbHNjcmVlbgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9ODMzMzAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICAqIHBhZ2UvRXZlbnRIYW5kbGVyLmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OkV2ZW50SGFuZGxlcjo6aXNLZXlFdmVudEFsbG93ZWRJbkZ1bGxTY3JlZW4pOgorCiAyMDEy LTA0LTA3ICBQYXRyaWNrIEdhbnN0ZXJlciAgPHBhcm9nYUB3ZWJraXQub3JnPgogCiAgICAgICAg IFtDTWFrZV0gQ2xlYW51cCBXVEYgaW5jbHVkZSBkaXJlY3RvcmllcwpJbmRleDogU291cmNlL1dl YkNvcmUvcGFnZS9FdmVudEhhbmRsZXIuY3BwDQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09DQotLS0gU291cmNlL1dlYkNv cmUvcGFnZS9FdmVudEhhbmRsZXIuY3BwCShyZXZpc2lvbiAxMTM1NTApCisrKyBTb3VyY2UvV2Vi Q29yZS9wYWdlL0V2ZW50SGFuZGxlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTI3MjQsNiArMjcy NCwxMyBAQCBib29sIEV2ZW50SGFuZGxlcjo6aXNLZXlFdmVudEFsbG93ZWRJbkZ1CiAgICAgaWYg KGRvY3VtZW50LT53ZWJraXRGdWxsU2NyZWVuS2V5Ym9hcmRJbnB1dEFsbG93ZWQoKSkKICAgICAg ICAgcmV0dXJuIHRydWU7CiAKKyAgICBpZiAoa2V5RXZlbnQudHlwZSgpID09IFBsYXRmb3JtS2V5 Ym9hcmRFdmVudDo6Q2hhcikgeworICAgICAgICBpZiAoa2V5RXZlbnQudGV4dCgpLmxlbmd0aCgp ICE9IDEpCisgICAgICAgICAgICByZXR1cm4gZmFsc2U7CisgICAgICAgIFVDaGFyIGNoYXJhY3Rl ciA9IGtleUV2ZW50LnRleHQoKVswXTsKKyAgICAgICAgcmV0dXJuIGNoYXJhY3RlciA9PSAnICc7 CisgICAgfQorCiAgICAgaW50IGtleUNvZGUgPSBrZXlFdmVudC53aW5kb3dzVmlydHVhbEtleUNv ZGUoKTsKICAgICByZXR1cm4gKGtleUNvZGUgPj0gVktfQkFDSyAmJiBrZXlDb2RlIDw9IFZLX0NB UElUQUwpCiAgICAgICAgIHx8IChrZXlDb2RlID49IFZLX1NQQUNFICYmIGtleUNvZGUgPD0gVktf REVMRVRFKQo=