QtLauncher failed to pass this newly added test case. It's also reported by build bot: http://build.webkit.org/results/Qt%20Linux%20Release/r50408%20%283364%29/results.html
Created attachment 42502 [details] this patch implements the support for shift-pageup and shift-pagedown. This patch does not make the test case pass yet. But this is the 1st step and better to be in a separate patch.
Comment on attachment 42502 [details] this patch implements the support for shift-pageup and shift-pagedown. LGTM, but please wrap the else before submitting, ie: if (kevent->shiftKey()) frame->editor()->command("MovePageDownAndModifySelection").execute(); else frame->editor()->command("MovePageDown").execute();
I think the original test case has a flaw: it issues a shift-pagedown and expects exactly 7 more links are selected. This is true for Safari. However, on other WebKit browsers like QtLauncher or GTK browsers (bug 31103), or Firefox, or IE, the amount of selected lines caused by shift-pagedown event is NOT deterministic. This makes the test case fail on other platforms. The intention of the test case is to check if selection would shrink as expected after a shift-arrowup following a "shift-down". I guess this "shift-down" can either be shift-arrowdown or shift-pagedown. While shift-arrowdown generates deterministic behavior, I suggest we change the test to replace shift-pagedown with multiple shift-arrowdown. This should resolve both failures on Qt and GTK. Btw, Qt did not support shift-pageup and shift-pagedown before and this has been addressed in bug 31166 and fixed already.
Created attachment 42582 [details] fix test case The original writer of the test case, Enrica Casucci [enrica@apple.com], pointed out that shift-PageDown is required for the test case. So my previous proposal of replacing it using multiple shift-arrowdown is not an option. Then I came up with this patch. This patch fixes the test case so that it no longer depends on the exact number of lines selected. On Qt, shift-pagedown has been implemented for bug 31166 and landed. I tested both Qt and Mac and this patch worked. I also think it should fix bug 31103, a GTK regression on this test case.
The patch looks good to me.
Comment on attachment 42582 [details] fix test case Clearing flags on attachment: 42582 Committed r50579: <http://trac.webkit.org/changeset/50579>
All reviewed patches have been landed. Closing bug.
This change leaves the test case in an inconsistent state. The text telling how to run the test case says: The selection should only include the lines with "YES". But that's no longer what the test looks like.
(In reply to comment #8) > This change leaves the test case in an inconsistent state. The text telling how > to run the test case says: > > The selection should only include the lines with "YES". > > But that's no longer what the test looks like. I guess I should update this text. However, the reason for inconsistent result is because of the behavior of shift-pagedown.
Created attachment 42662 [details] update test description
I think Chang meant to re-open the bug before posting a new patch.
(In reply to comment #11) > I think Chang meant to re-open the bug before posting a new patch. Thanks for doing this for me.
Comment on attachment 42662 [details] update test description Sounds sensible.
Comment on attachment 42662 [details] update test description Clearing flags on attachment: 42662 Committed r50626: <http://trac.webkit.org/changeset/50626>