Bug 31103 – [GTK] editing/selection/shrink-selection-after-shift-pagedown.html failing

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Xan Lopez]

This test is failing for at least two reasons. First of all, it tries to modify a textarea without focusing it first, assuming that the DRT will focus the view when starting. Our DRT does not do this, so it fails. Patch attached to fix that.

After that is done, the test still fails. Apparently the height we give for the area is too big, so the scrolling code in EditorCommand.cpp goes too far and we end up selecting one row too many (one 'NO' too many), so after doing a shift-up we have still one NO selected. One way to workaround this is to tweak our lineSpacing in our Font code, so I believe this might be related to our age-old bug in that code where our lineSpacing seems to be miscalculated in some cases. I have a small cairo testcase for that (attached) and I'll send it to the cairo list, but in any case this still needs investigation.

[Xan Lopez]

Created attachment 42458 [details]
focusdrt.diff

Focus the webviews in DRT when created.

Requesting cq also, since I'm not going to be around for a few days.

[Xan Lopez]

Created attachment 42459 [details]
testcairofont.c

Small testcase showing how cairo gets linespacing wrong in some situations.

[Jan Alonzo]

Comment on attachment 42458 [details]
focusdrt.diff

r=me but cq- so Xan can watch for any regressions on the bot, if any. Also, please change the title of this bug and changelog when landing since the patch doesn't fully fix it anyway.

[Chang Shu]

(In reply to comment #0)

> After that is done, the test still fails. Apparently the height we give for the
> area is too big, so the scrolling code in EditorCommand.cpp goes too far and we
> end up selecting one row too many (one 'NO' too many), so after doing a
> shift-up we have still one NO selected. One way to workaround this is to tweak
> our lineSpacing in our Font code, so I believe this might be related to our
> age-old bug in that code where our lineSpacing seems to be miscalculated in
> some cases. I have a small cairo testcase for that (attached) and I'll send it
> to the cairo list, but in any case this still needs investigation.

Somehow, I observe the same situation in Qt (see bug 31122).
"Shift-PageDown" selected one row more than expected (11 rows all together). However, if I manually do the "Shift-Pagedown" on QtLauncher, the selection is much less: 8 rows. I haven't figured out why.

[Chang Shu]

I did some research and found the pagedown scroll amount is platform-dependent. See the following funciton in EditorCommand.cpp:
static int verticalScrollDistance(Frame* frame)
{
...
    int height = toRenderBox(renderer)->clientHeight();
    return max((height + 1) / 2, height - cAmountToKeepWhenPaging);
}

I believe height itself depends on the font size. However, cAmountToKeepWhenPaging is hardcoded to 40. Thus, the pagedown event does not guarantee the scroll amount to be the multiplication of line height. Unfortunately, on Qt and GTK, it selected one more line and the test case failed.

[Chang Shu]

I fixed the Qt side bug 31122 by changing the test case itself because I think the shift-pagedown behavior on Qt is fine. The patch should fix GTK too. You guys can check once it's landed.

[Eric Seidel (no email)]

Ping?  Shoudl this bug still be open?

[Xan Lopez]

Landed the patch, and we seem to be passing the test. Thanks Chang!