Bug 205486 – ComplexTextController::offsetForPosition returns a wrong offset for a glyph boundary in a RTL text

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Fujii Hironori]

[Win] imported/blink/editing/selection/offset-from-point-complex-scripts.html is failing since Bug 204884

ComplexTextController::offsetForPosition returns a incorrect offset for RTL text.

[Fujii Hironori]

Created attachment 386327 [details]
offset-from-point-complex-scripts-actual.txt of GTK port

GTK port is also failing since adopting ComplexTextController.

[Fujii Hironori]

Created attachment 386328 [details]
Add a new test case with Ahem

[Fujii Hironori]

This issue can be reproduced for Mac port.

Expected: 6 5 4 3 2 1 0
Actual: 6 5 6 5 4 5 4 3 4 3 2 3 2 1 2 1 0

Chrome for Mac looks working fine.

This issue can be reproduced by two conditions:

* Glyphs have integral width
* using ComplexTextController

[Fujii Hironori]

(In reply to Fujii Hironori from comment #3)
> This issue can be reproduced by two conditions:
> 
> * Glyphs have integral width
> * using ComplexTextController

And,

* RTL

[Fujii Hironori]

Created attachment 386331 [details]
rtl.html

[Fujii Hironori]

Created attachment 386332 [details]
screenshot of Chrome for Mac with Ahem font

[Fujii Hironori]

ComplexTextController::offsetForPosition has the following code:

> unsigned hitIndex = hitGlyphStart + (hitGlyphEnd - hitGlyphStart) * (m_run.ltr() ? x / adjustedAdvance : 1 - x / adjustedAdvance);

If m_run.ltr() is true and x == 0, hitIndex would become hitGlyphEnd.
This is not expected.

[Fujii Hironori]

Created attachment 386334 [details]
Patch

[Fujii Hironori]

Created attachment 386335 [details]
Patch

[Fujii Hironori]

Created attachment 386338 [details]
Patch

[Fujii Hironori]

Created attachment 386339 [details]
Patch

[Fujii Hironori]

fast/text/ellipsis-text-rtl.html failed on Mac EWS.
This is a edge case.

In RTL text, the char offset of *previous* glyph needs to be returned for glyph boundary.

For example,

characters: [A] [B]
LTR glyphs: (A) (B)
RTL glyphs: (B) (A)

Assuming glyph widths are 20px,
and argument 'h' of offsetForPosition is 20px, which is the position between (A) and (B).

In LTR, glyph (B) is hit, and offset of [B] should be returned.
In RTL, glyph (B) is hit, and offset of [B] should be returned.

In my patch, in RTL, glyph (A) is hit, and offset of [A] is returned.

[Fujii Hironori]

Created attachment 386371 [details]
Patch

[Fujii Hironori]

Created attachment 386372 [details]
Patch

[Fujii Hironori]

Created attachment 386385 [details]
Patch

[Darin Adler]

Comment on attachment 386385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386385&action=review

> Source/WebCore/platform/graphics/ComplexTextController.cpp:227
> +                        hitIndex = hitGlyphEnd - (hitGlyphEnd - hitGlyphStart) * (x / adjustedAdvance);

Is the rounding here correct? This truncates toward towards the end. Is that what’s intended?

[Fujii Hironori]

Comment on attachment 386385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386385&action=review

Thank you for reviewing my patch.

>> Source/WebCore/platform/graphics/ComplexTextController.cpp:227
>> +                        hitIndex = hitGlyphEnd - (hitGlyphEnd - hitGlyphStart) * (x / adjustedAdvance);
> 
> Is the rounding here correct? This truncates toward towards the end. Is that what’s intended?

In my understanding, Neither rounding nor truncating toward the end happen in this expression.
Only truncating toward the start can happen by assigning a float value to hitIndex. It is expected.

[Fujii Hironori]

Myles, do you have a chance to review this patch? Should I look for other reviewers?

[Ross Kirsling]

Comment on attachment 386385 [details]
Patch

While this isn't my usual reviewing domain, the change and your explanation are quite clear, a test is fixed and another added, and EWS is happy, so that makes me feel pretty good about this patch.

[Fujii Hironori]

Created attachment 386935 [details]
Patch for landing

[Fujii Hironori]

Comment on attachment 386935 [details]
Patch for landing

Clearing flags on attachment: 386935

Committed r254114: <https://trac.webkit.org/changeset/254114>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58366513>

[Fujii Hironori]

Filed: Bug 205898 – [GTK] fast/text/atsui-rtl-override-selection.html is failing since r254114