Bug 5479 - Can't select text with RTL override rendered by ATSUI
Summary: Can't select text with RTL override rendered by ATSUI
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-10-24 09:26 PDT by mitz
Modified: 2005-12-18 08:20 PST (History)
0 users

See Also:


Attachments
WebCore part of the patch (6.74 KB, patch)
2005-10-24 09:45 PDT, mitz
darin: review+
Details | Formatted Diff | Diff
WebKit part of the patch (9.06 KB, patch)
2005-10-24 09:54 PDT, mitz
darin: review-
Details | Formatted Diff | Diff
Revised WebKit part (10.38 KB, patch)
2005-10-25 08:04 PDT, mitz
darin: review+
Details | Formatted Diff | Diff
regression test (4.09 KB, patch)
2005-11-29 05:12 PST, mitz
no flags Details | Formatted Diff | Diff
expected result for the pixel test (9.33 KB, image/png)
2005-11-29 05:13 PST, mitz
no flags Details
regression test (4.15 KB, patch)
2005-11-30 10:23 PST, mitz
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2005-10-24 09:26:22 PDT
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.
Comment 1 mitz 2005-10-24 09:45:55 PDT
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.
Comment 2 mitz 2005-10-24 09:54:17 PDT
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 3 Darin Adler 2005-10-24 18:10:18 PDT
Comment on attachment 4457 [details]
WebCore part of the patch

Looks good, r=me.
Comment 4 Darin Adler 2005-10-24 18:15:30 PDT
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.
Comment 5 mitz 2005-10-25 00:33:12 PDT
(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).
Comment 6 mitz 2005-10-25 08:04:36 PDT
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 7 Darin Adler 2005-10-25 08:20:36 PDT
Comment on attachment 4473 [details]
Revised WebKit part

Looks great, assuming all layout tests still pass. r=me
Comment 8 Eric Seidel (no email) 2005-11-29 02:22:15 PST
Is it possible to make a test case for this?  If so, please add one. (In patch for would be ideal).
Comment 9 mitz 2005-11-29 05:12:02 PST
Created attachment 4850 [details]
regression test

Despite the editing delegate messages, you need to run this in pixel mode to
detect the highlighting regression.
Comment 10 mitz 2005-11-29 05:13:08 PST
Created attachment 4851 [details]
expected result for the pixel test
Comment 11 mitz 2005-11-29 09:00:23 PST
I think there can be a better testcase (with regressions detectable by the editing delegate messages) once 
bug 5878 is fixed.
Comment 12 mitz 2005-11-30 08:51:10 PST
Comment on attachment 4851 [details]
expected result for the pixel test

Strike my previous comment. I think this is a good test.
Comment 13 mitz 2005-11-30 10:23:52 PST
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.