[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.
Created attachment 386327 [details] offset-from-point-complex-scripts-actual.txt of GTK port GTK port is also failing since adopting ComplexTextController.
Created attachment 386328 [details] Add a new test case with Ahem
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
(In reply to Fujii Hironori from comment #3) > This issue can be reproduced by two conditions: > > * Glyphs have integral width > * using ComplexTextController And, * RTL
Created attachment 386331 [details] rtl.html
Created attachment 386332 [details] screenshot of Chrome for Mac with Ahem font
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.
Created attachment 386334 [details] Patch
Created attachment 386335 [details] Patch
Created attachment 386338 [details] Patch
Created attachment 386339 [details] Patch
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.
Created attachment 386371 [details] Patch
Created attachment 386372 [details] Patch
Created attachment 386385 [details] Patch
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?
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.
Myles, do you have a chance to review this patch? Should I look for other reviewers?
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.
Created attachment 386935 [details] Patch for landing
Comment on attachment 386935 [details] Patch for landing Clearing flags on attachment: 386935 Committed r254114: <https://trac.webkit.org/changeset/254114>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58366513>
Filed: Bug 205898 – [GTK] fast/text/atsui-rtl-override-selection.html is failing since r254114