Bug 31103

Summary: [GTK] editing/selection/shrink-selection-after-shift-pagedown.html failing
Product: WebKit Reporter: Xan Lopez <xan.lopez>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cshu, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
focusdrt.diff
jmalonzo: review+, jmalonzo: commit-queue-
testcairofont.c none

Description Xan Lopez 2009-11-04 00:23:27 PST
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.
Comment 1 Xan Lopez 2009-11-04 00:28:05 PST
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.
Comment 2 Xan Lopez 2009-11-04 00:29:06 PST
Created attachment 42459 [details]
testcairofont.c

Small testcase showing how cairo gets linespacing wrong in some situations.
Comment 3 Jan Alonzo 2009-11-04 03:38:52 PST
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.
Comment 4 Chang Shu 2009-11-04 11:37:54 PST
(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.
Comment 5 Chang Shu 2009-11-04 14:58:11 PST
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.
Comment 6 Chang Shu 2009-11-05 14:42:55 PST
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.
Comment 7 Eric Seidel (no email) 2009-12-09 14:05:23 PST
Ping?  Shoudl this bug still be open?
Comment 8 Xan Lopez 2009-12-10 00:29:58 PST
Landed the patch, and we seem to be passing the test. Thanks Chang!