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.
Comment on attachment 4457 [details] WebCore part of the patch Looks good, r=me.
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.
Comment on attachment 4473 [details] Revised WebKit part Looks great, assuming all layout tests still pass. r=me
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.