49056 2010-11-04 21:28:28 -0700 [GTK] Spatial Navigation: snav-input.html and snav-textarea.html fail 2010-11-11 20:16:39 -0800 1 1 1 Unclassified WebKit Accessibility 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 46905 1 tonikitoo cshu commit-queue cshu webkit.review.bot oldest_to_newest 305131 0 tonikitoo 2010-11-04 21:28:28 -0700 Tests added recently and run fine on Qt and Mac ports. 307498 1 cshu 2010-11-10 12:49:37 -0800 I will take a look. 307572 2 73543 cshu 2010-11-10 14:40:22 -0800 Created attachment 73543 fix patch 307577 3 webkit.review.bot 2010-11-10 14:45:42 -0800 Attachment 73543 did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/Skipped', u'WebKit/gtk/ChangeLog', u'WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp']" exit_code: 1 WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:219: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style. 307582 4 cshu 2010-11-10 14:52:44 -0800 I will fix the style problem along with other possible changes after the comments. 307704 5 73543 tonikitoo 2010-11-10 20:06:44 -0800 Comment on attachment 73543 fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=73543&action=review > WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:217 > + if (step == 1 && isSpatialNavigationEnabled(core(client->webView())->focusController()->focusedOrMainFrame())) { > + if (direction == 1) Looks good to me. Please reverse the order for the "if", so we bail out earlier in the common case. if (isSpatialNavigationEnabled(frame) && xxx) { all the test here. } 307863 6 73603 cshu 2010-11-11 06:06:14 -0800 Created attachment 73603 fix patch 2 307873 7 73603 tonikitoo 2010-11-11 06:30:40 -0800 Comment on attachment 73603 fix patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=73603&action=review Looks nice! Lets just fix the style issues before landing. > WebKit/gtk/ChangeLog:6 > + [GTK] Replace "MoveForward"/"MoveBackward" with "MoveRight"/"MoveLeft" > + to make spatial navigation work on input/textarea. You need the bug title here. Implementation details goes right below that. > WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:220 > + if (direction == 1) > + rawCommand = "MoveRight"; > + else if (!direction) > + rawCommand = "MoveLeft"; rawCommand = direction ? "MoveRight" ? "MoveLeft"; > LayoutTests/ChangeLog:5 > + [GTK] Unskip tests that are passing now. same here: first bug title , bugzilla entry url, then the description. 307881 8 cshu 2010-11-11 06:38:26 -0800 Thanks for the review, Antonio. The style problem was fixed. It was the "direction == 0" it was complaining. Since direction can be 2 or 3, I cannot do "direction?x:y". > Looks nice! Lets just fix the style issues before landing. > > > WebKit/gtk/ChangeLog:6 > > + [GTK] Replace "MoveForward"/"MoveBackward" with "MoveRight"/"MoveLeft" > > + to make spatial navigation work on input/textarea. > > You need the bug title here. Implementation details goes right below that. > > > WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:220 > > + if (direction == 1) > > + rawCommand = "MoveRight"; > > + else if (!direction) > > + rawCommand = "MoveLeft"; > > rawCommand = direction ? "MoveRight" ? "MoveLeft"; > 308343 9 73603 commit-queue 2010-11-11 20:16:34 -0800 Comment on attachment 73603 fix patch 2 Clearing flags on attachment: 73603 Committed r71881: <http://trac.webkit.org/changeset/71881> 308344 10 commit-queue 2010-11-11 20:16:39 -0800 All reviewed patches have been landed. Closing bug. 73543 2010-11-10 14:40:22 -0800 2010-11-11 06:06:14 -0800 fix patch 49056_1.patch text/plain 2724 cshu SW5kZXg6IFdlYktpdC9ndGsvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYktpdC9ndGsvQ2hh bmdlTG9nCShyZXZpc2lvbiA3MTc3MSkKKysrIFdlYktpdC9ndGsvQ2hhbmdlTG9nCSh3b3JraW5n IGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTAtMTEtMTAgIENoYW5nIFNodSAgPGNoYW5nLnNo dUBub2tpYS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgW0dUS10gUmVwbGFjZSAiTW92ZUZvcndhcmQiLyJNb3ZlQmFja3dhcmQiIHdpdGggIk1v dmVSaWdodCIvIk1vdmVMZWZ0IgorICAgICAgICB0byBtYWtlIHNwYXRpYWwgbmF2aWdhdGlvbiB3 b3JrIG9uIGlucHV0L3RleHRhcmVhLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9NDkwNTYKKworICAgICAgICAqIFdlYkNvcmVTdXBwb3J0L0VkaXRvckNs aWVudEd0ay5jcHA6CisgICAgICAgIChXZWJLaXQ6Om1vdmVDdXJzb3JDYWxsYmFjayk6CisKIDIw MTAtMTEtMDggIE5pY29sYXMgRHVmcmVzbmUgIDxuaWNvbGFzLmR1ZnJlc25lQGNvbGxhYm9yYS5j by51az4KIAogICAgICAgICBSZXZpZXdlZCBieSBNYXJ0aW4gUm9iaW5zb24uCkluZGV4OiBXZWJL aXQvZ3RrL1dlYkNvcmVTdXBwb3J0L0VkaXRvckNsaWVudEd0ay5jcHAKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g V2ViS2l0L2d0ay9XZWJDb3JlU3VwcG9ydC9FZGl0b3JDbGllbnRHdGsuY3BwCShyZXZpc2lvbiA3 MTc2MSkKKysrIFdlYktpdC9ndGsvV2ViQ29yZVN1cHBvcnQvRWRpdG9yQ2xpZW50R3RrLmNwcAko d29ya2luZyBjb3B5KQpAQCAtMjEzLDYgKzIxMywxMyBAQCBzdGF0aWMgdm9pZCBtb3ZlQ3Vyc29y Q2FsbGJhY2soR3RrV2lkZ2V0CiAgICAgaWYgKCFyYXdDb21tYW5kKQogICAgICAgICByZXR1cm47 CiAKKyAgICBpZiAoc3RlcCA9PSAxICYmIGlzU3BhdGlhbE5hdmlnYXRpb25FbmFibGVkKGNvcmUo Y2xpZW50LT53ZWJWaWV3KCkpLT5mb2N1c0NvbnRyb2xsZXIoKS0+Zm9jdXNlZE9yTWFpbkZyYW1l KCkpKSB7CisgICAgICAgIGlmIChkaXJlY3Rpb24gPT0gMSkKKyAgICAgICAgICAgIHJhd0NvbW1h bmQgPSAiTW92ZVJpZ2h0IjsKKyAgICAgICAgZWxzZSBpZiAoZGlyZWN0aW9uID09IDApCisgICAg ICAgICAgICByYXdDb21tYW5kID0gIk1vdmVMZWZ0IjsKKyAgICB9CisKICAgICBmb3IgKGludCBp ID0gMDsgaSA8IGFicyhjb3VudCk7IGkrKykKICAgICAgICAgY2xpZW50LT5hZGRQZW5kaW5nRWRp dG9yQ29tbWFuZChyYXdDb21tYW5kKTsKIH0KSW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZwo9 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHJldmlzaW9uIDcxNzcxKQorKysg TGF5b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAKKzIw MTAtMTEtMTAgIENoYW5nIFNodSAgPGNoYW5nLnNodUBub2tpYS5jb20+CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW0dUS10gVW5za2lwIHRlc3RzIHRo YXQgYXJlIHBhc3Npbmcgbm93LgorICAgICAgICBmYXN0L2V2ZW50cy9zcGF0aWFsLW5hdmlnYXRp b24vc25hdi1pbnB1dC5odG1sCisgICAgICAgIGZhc3QvZXZlbnRzL3NwYXRpYWwtbmF2aWdhdGlv bi9zbmF2LXRleHRhcmVhLmh0bWwKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTQ5MDU2CisKKyAgICAgICAgKiBwbGF0Zm9ybS9ndGsvU2tpcHBlZDoKKwog MjAxMC0xMS0xMCAgQWxleGV5IFByb3NrdXJ5YWtvdiAgPGFwQGFwcGxlLmNvbT4KIAogICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDUyMDUKSW5kZXg6IExh eW91dFRlc3RzL3BsYXRmb3JtL2d0ay9Ta2lwcGVkCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3Rz L3BsYXRmb3JtL2d0ay9Ta2lwcGVkCShyZXZpc2lvbiA3MTc2MSkKKysrIExheW91dFRlc3RzL3Bs YXRmb3JtL2d0ay9Ta2lwcGVkCSh3b3JraW5nIGNvcHkpCkBAIC00OTY0LDExICs0OTY0LDYgQEAg c2Nyb2xsYmFycy9zY3JvbGxiYXItbWlkZGxlY2xpY2stbm9wYXN0ZQogIyBSZWxpZXMgb24gV2Vi S2l0IEFQSSBbV2ViVmlldyBfbG9hZEJhY2tGb3J3YXJkTGlzdEZyb21PdGhlclZpZXc6XQogZmFz dC9sb2FkZXIvY3Jhc2gtY29weWluZy1iYWNrZm9yd2FyZGxpc3QuaHRtbAogCi0jIFNwYXRpYWwg TmF2aWdhdGlvbgotIyBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDkw NTYKLWZhc3QvZXZlbnRzL3NwYXRpYWwtbmF2aWdhdGlvbi9zbmF2LWlucHV0Lmh0bWwKLWZhc3Qv ZXZlbnRzL3NwYXRpYWwtbmF2aWdhdGlvbi9zbmF2LXRleHRhcmVhLmh0bWwKLQogIyBJbXBsZW1l bnQgTGF5b3V0VGVzdENvbnRyb2xsZXI6OmNhbGxTaG91bGRDbG9zZU9uV2ViVmlldygpCiBmYXN0 L2V2ZW50cy9vbmJlZm9yZXVubG9hZC1mb2N1c2VkLWlmcmFtZS5odG1sCiAK 73603 2010-11-11 06:06:14 -0800 2010-11-11 20:16:34 -0800 fix patch 2 49056_2.patch text/plain 2707 cshu SW5kZXg6IFdlYktpdC9ndGsvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYktpdC9ndGsvQ2hh bmdlTG9nCShyZXZpc2lvbiA3MTgxMykKKysrIFdlYktpdC9ndGsvQ2hhbmdlTG9nCSh3b3JraW5n IGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTAtMTEtMTEgIENoYW5nIFNodSAgPGNoYW5nLnNo dUBub2tpYS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgW0dUS10gUmVwbGFjZSAiTW92ZUZvcndhcmQiLyJNb3ZlQmFja3dhcmQiIHdpdGggIk1v dmVSaWdodCIvIk1vdmVMZWZ0IgorICAgICAgICB0byBtYWtlIHNwYXRpYWwgbmF2aWdhdGlvbiB3 b3JrIG9uIGlucHV0L3RleHRhcmVhLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9NDkwNTYKKworICAgICAgICAqIFdlYkNvcmVTdXBwb3J0L0VkaXRvckNs aWVudEd0ay5jcHA6CisgICAgICAgIChXZWJLaXQ6Om1vdmVDdXJzb3JDYWxsYmFjayk6CisKIDIw MTAtMTEtMDggIE5pY29sYXMgRHVmcmVzbmUgIDxuaWNvbGFzLmR1ZnJlc25lQGNvbGxhYm9yYS5j by51az4KIAogICAgICAgICBSZXZpZXdlZCBieSBNYXJ0aW4gUm9iaW5zb24uCkluZGV4OiBXZWJL aXQvZ3RrL1dlYkNvcmVTdXBwb3J0L0VkaXRvckNsaWVudEd0ay5jcHAKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g V2ViS2l0L2d0ay9XZWJDb3JlU3VwcG9ydC9FZGl0b3JDbGllbnRHdGsuY3BwCShyZXZpc2lvbiA3 MTgxMykKKysrIFdlYktpdC9ndGsvV2ViQ29yZVN1cHBvcnQvRWRpdG9yQ2xpZW50R3RrLmNwcAko d29ya2luZyBjb3B5KQpAQCAtMjEzLDYgKzIxMywxMyBAQCBzdGF0aWMgdm9pZCBtb3ZlQ3Vyc29y Q2FsbGJhY2soR3RrV2lkZ2V0CiAgICAgaWYgKCFyYXdDb21tYW5kKQogICAgICAgICByZXR1cm47 CiAKKyAgICBpZiAoaXNTcGF0aWFsTmF2aWdhdGlvbkVuYWJsZWQoY29yZShjbGllbnQtPndlYlZp ZXcoKSktPmZvY3VzQ29udHJvbGxlcigpLT5mb2N1c2VkT3JNYWluRnJhbWUoKSkgJiYgc3RlcCA9 PSAxKSB7CisgICAgICAgIGlmIChkaXJlY3Rpb24gPT0gMSkKKyAgICAgICAgICAgIHJhd0NvbW1h bmQgPSAiTW92ZVJpZ2h0IjsKKyAgICAgICAgZWxzZSBpZiAoIWRpcmVjdGlvbikKKyAgICAgICAg ICAgIHJhd0NvbW1hbmQgPSAiTW92ZUxlZnQiOworICAgIH0KKwogICAgIGZvciAoaW50IGkgPSAw OyBpIDwgYWJzKGNvdW50KTsgaSsrKQogICAgICAgICBjbGllbnQtPmFkZFBlbmRpbmdFZGl0b3JD b21tYW5kKHJhd0NvbW1hbmQpOwogfQpJbmRleDogTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIExheW91dFRlc3RzL0NoYW5nZUxvZwkocmV2aXNpb24gNzE4MTMpCisrKyBMYXlv dXRUZXN0cy9DaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAgLTEsMyArMSwxNCBAQAorMjAxMC0x MS0xMSAgQ2hhbmcgU2h1ICA8Y2hhbmcuc2h1QG5va2lhLmNvbT4KKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBbR1RLXSBVbnNraXAgdGVzdHMgdGhhdCBh cmUgcGFzc2luZyBub3cuCisgICAgICAgIGZhc3QvZXZlbnRzL3NwYXRpYWwtbmF2aWdhdGlvbi9z bmF2LWlucHV0Lmh0bWwKKyAgICAgICAgZmFzdC9ldmVudHMvc3BhdGlhbC1uYXZpZ2F0aW9uL3Nu YXYtdGV4dGFyZWEuaHRtbAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9NDkwNTYKKworICAgICAgICAqIHBsYXRmb3JtL2d0ay9Ta2lwcGVkOgorCiAyMDEw LTExLTExICBBZGFtIFJvYmVuICA8YXJvYmVuQGFwcGxlLmNvbT4KIAogICAgICAgICBTa2lwIGEg bmV3bHktY3Jhc2hpbmcgdGVzdCBvbiBXaW5kb3dzCkluZGV4OiBMYXlvdXRUZXN0cy9wbGF0Zm9y bS9ndGsvU2tpcHBlZAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9wbGF0Zm9ybS9ndGsvU2tp cHBlZAkocmV2aXNpb24gNzE4MTMpCisrKyBMYXlvdXRUZXN0cy9wbGF0Zm9ybS9ndGsvU2tpcHBl ZAkod29ya2luZyBjb3B5KQpAQCAtNDk2NCwxMSArNDk2NCw2IEBAIHNjcm9sbGJhcnMvc2Nyb2xs YmFyLW1pZGRsZWNsaWNrLW5vcGFzdGUKICMgUmVsaWVzIG9uIFdlYktpdCBBUEkgW1dlYlZpZXcg X2xvYWRCYWNrRm9yd2FyZExpc3RGcm9tT3RoZXJWaWV3Ol0KIGZhc3QvbG9hZGVyL2NyYXNoLWNv cHlpbmctYmFja2ZvcndhcmRsaXN0Lmh0bWwKIAotIyBTcGF0aWFsIE5hdmlnYXRpb24KLSMgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTQ5MDU2Ci1mYXN0L2V2ZW50cy9z cGF0aWFsLW5hdmlnYXRpb24vc25hdi1pbnB1dC5odG1sCi1mYXN0L2V2ZW50cy9zcGF0aWFsLW5h dmlnYXRpb24vc25hdi10ZXh0YXJlYS5odG1sCi0KICMgSW1wbGVtZW50IExheW91dFRlc3RDb250 cm9sbGVyOjpjYWxsU2hvdWxkQ2xvc2VPbldlYlZpZXcoKQogZmFzdC9ldmVudHMvb25iZWZvcmV1 bmxvYWQtZm9jdXNlZC1pZnJhbWUuaHRtbAogCg==