Bug 31122 - [Qt] editing/selection/shrink-selection-after-shift-pagedown.html failing
Summary: [Qt] editing/selection/shrink-selection-after-shift-pagedown.html failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Chang Shu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-04 08:27 PST by Chang Shu
Modified: 2009-11-08 11:09 PST (History)
6 users (show)

See Also:


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 Details | Formatted Diff | Diff
fix test case (2.30 KB, patch)
2009-11-05 11:43 PST, Chang Shu
no flags Details | Formatted Diff | Diff
update test description (2.23 KB, patch)
2009-11-06 11:14 PST, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chang Shu 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
Comment 1 Chang Shu 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.
Comment 2 Tor Arne Vestbø 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();
Comment 3 Chang Shu 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.
Comment 4 Chang Shu 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.
Comment 5 Enrica Casucci 2009-11-05 11:51:13 PST
The patch looks good to me.
Comment 6 WebKit Commit Bot 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>
Comment 7 WebKit Commit Bot 2009-11-05 14:47:15 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 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.
Comment 9 Chang Shu 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.
Comment 10 Chang Shu 2009-11-06 11:14:49 PST
Created attachment 42662 [details]
update test description
Comment 11 Eric Seidel (no email) 2009-11-06 11:21:54 PST
I think Chang meant to re-open the bug before posting a new patch.
Comment 12 Chang Shu 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.
Comment 13 Holger Freyther 2009-11-07 00:49:44 PST
Comment on attachment 42662 [details]
update test description

Sounds sensible.
Comment 14 WebKit Commit Bot 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>
Comment 15 WebKit Commit Bot 2009-11-08 11:09:53 PST
All reviewed patches have been landed.  Closing bug.