Bug 5479 – Can't select text with RTL override rendered by ATSUI

• Status: VERIFIED
• Resolution: FIXED
• Component: Layout and Rendering

Mouse-position-to-char-offset translation is broken in the ATSUI code path for text with RTL override. For 
example, selecting Use ATSUI For All Text and going to http://bugzilla.opendarwin.org/attachment.cgi?
id=4448 the selection highlight flickers and jumps around as you try to make a selection in the last line. 
The same happens with visually-ordered Hebrew.

Created attachment 4457 [details]
WebCore part of the patch

Passes the override flag to WebTextRenderer for the ATSUI case. Also avoids
passing characters not belonging to the text object.

Created attachment 4458 [details]
WebKit part of the patch

What I thought was a bug in ATSUI was actually due to misinterpretation of the
originalOffset member in the override. The change in the CG code path in this
patch is to fix the drag image of partial RTL selections.

[Darin Adler]

Comment on attachment 4457 [details]
WebCore part of the patch

Looks good, r=me.

[Darin Adler]

Comment on attachment 4458 [details]
WebKit part of the patch

Looks good. Will have to be merged with the change from methods to functions.

I have two questions:

1) I'm not entirely sure that roundf is the right thing to call for
selectedLeftX and backgroundWidth. Why rounding and not ceiling or floor?

2) I don't understand why the ATSUPositionToOffset workaround can be removed.

(In reply to comment #4)
> 1) I'm not entirely sure that roundf is the right thing to call for
> selectedLeftX and backgroundWidth. Why rounding and not ceiling or floor?

The idea is to match InlineTextBox::selectionRect()'s behavior, or actually, never to extend beyond the 
rects it computes (that's how trails happen). selectionRect() works by calling floatWidthForRun to get 
two widths: up to the selection start, and the selection itself, corresponding to selectedLeftX and 
backgroundWidth). If floatWidthForRun happens to return fractions, they are lroundf()ed (in 
QFontMetrics::width).

That explains selectedLeftX: the rounding hack is applies as appropriate, and the result is rounded. For 
backgroundWidth the rounding hack is ignored, since applying it would result in the right edge of the 
highlight jumping between two adjacent positions as you dragged the left edge around (this is in fact 
what the selectionRect()'s left edge does). For backgruondWidth, simple rounding assures that it doesn't 
extend beyond the selectionRect, that it's stable, and that it's an integer (I don't think Cocoa does 
antialiased selection edges, so WebKit shouldn't either).

> 2) I don't understand why the ATSUPositionToOffset workaround can be removed.

I was wrong. There was not ATSUI bug to work around. What happened was that due to looking at the 
wrong characters in the override, the rounding hack was applied incorrectly when (run->from != 0). It 
happened to be more noticeable in positionToOffset because WebCore wasn't trimming the buffer (but 
with this patch it will).

Created attachment 4473 [details]
Revised WebKit part

Includes the change from methods to functions. Revised the ATSUI highlighting
code. Since the rounding hack is always applied from left to right in the ATSUI
path, the extra call for the total width is necessary in the RTL case. The
principles are: compute the "start" edge to match
InlineTextBox::selectionRect(); compute the "end" edge so that it fits inside
the selectionRect() and does not depend on the "start" edge; ensure that both
edges are integers.

[Darin Adler]

Comment on attachment 4473 [details]
Revised WebKit part

Looks great, assuming all layout tests still pass. r=me

[Eric Seidel (no email)]

Is it possible to make a test case for this?  If so, please add one. (In patch for would be ideal).

Created attachment 4850 [details]
regression test

Despite the editing delegate messages, you need to run this in pixel mode to
detect the highlighting regression.

Created attachment 4851 [details]
expected result for the pixel test

I think there can be a better testcase (with regressions detectable by the editing delegate messages) once 
bug 5878 is fixed.

Comment on attachment 4851 [details]
expected result for the pixel test

Strike my previous comment. I think this is a good test.

Created attachment 4877 [details]
regression test

Looks like cvs-create-patch --include-unknown mangled the paths. The test is
supposed to go into fast/text.