RESOLVED FIXED 31122
[Qt] editing/selection/shrink-selection-after-shift-pagedown.html failing
https://bugs.webkit.org/show_bug.cgi?id=31122
Summary [Qt] editing/selection/shrink-selection-after-shift-pagedown.html failing
Chang Shu
Reported 2009-11-04 08:27:31 PST
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
Attachments
this patch implements the support for shift-pageup and shift-pagedown. (1.77 KB, patch)
2009-11-04 11:29 PST, Chang Shu
no flags
fix test case (2.30 KB, patch)
2009-11-05 11:43 PST, Chang Shu
no flags
update test description (2.23 KB, patch)
2009-11-06 11:14 PST, Chang Shu
no flags
Chang Shu
Comment 1 2009-11-04 11:29:31 PST
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.
Tor Arne Vestbø
Comment 2 2009-11-05 06:44:42 PST
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();
Chang Shu
Comment 3 2009-11-05 08:41:31 PST
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.
Chang Shu
Comment 4 2009-11-05 11:43:56 PST
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.
Enrica Casucci
Comment 5 2009-11-05 11:51:13 PST
The patch looks good to me.
WebKit Commit Bot
Comment 6 2009-11-05 14:47:08 PST
Comment on attachment 42582 [details] fix test case Clearing flags on attachment: 42582 Committed r50579: <http://trac.webkit.org/changeset/50579>
WebKit Commit Bot
Comment 7 2009-11-05 14:47:15 PST
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 8 2009-11-06 09:42:14 PST
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.
Chang Shu
Comment 9 2009-11-06 09:58:11 PST
(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.
Chang Shu
Comment 10 2009-11-06 11:14:49 PST
Created attachment 42662 [details] update test description
Eric Seidel (no email)
Comment 11 2009-11-06 11:21:54 PST
I think Chang meant to re-open the bug before posting a new patch.
Chang Shu
Comment 12 2009-11-06 11:23:16 PST
(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.
Holger Freyther
Comment 13 2009-11-07 00:49:44 PST
Comment on attachment 42662 [details] update test description Sounds sensible.
WebKit Commit Bot
Comment 14 2009-11-08 11:09:48 PST
Comment on attachment 42662 [details] update test description Clearing flags on attachment: 42662 Committed r50626: <http://trac.webkit.org/changeset/50626>
WebKit Commit Bot
Comment 15 2009-11-08 11:09:53 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.