[Qt] editing/selection/shrink-selection-after-shift-pagedown.html failing

[Chang Shu]

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

[Chang Shu]

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 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]

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]

Created attachment 42582 [details]
fix test case

The original writer of the test case, Enrica Casucci [[e-mail]], 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]

The patch looks good to me.

[WebKit Commit Bot]

Comment on attachment 42582 [details]
fix test case

Clearing flags on attachment: 42582

Committed r50579: <http://trac.webkit.org/changeset/50579>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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]

(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]

Created attachment 42662 [details]
update test description

[Eric Seidel (no email)]

I think Chang meant to re-open the bug before posting a new patch.

[Chang Shu]

(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 on attachment 42662 [details]
update test description

Sounds sensible.

[WebKit Commit Bot]

Comment on attachment 42662 [details]
update test description

Clearing flags on attachment: 42662

Committed r50626: <http://trac.webkit.org/changeset/50626>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.