Merge r254114 - ComplexTextController::offsetForPosition returns a wr…

…ong offset for a glyph boundary in a RTL text https://bugs.webkit.org/show_bug.cgi?id=205486 Reviewed by Ross Kirsling. Source/WebCore: ComplexTextController::offsetForPosition had the following code: > unsigned hitIndex = hitGlyphStart + (hitGlyphEnd - hitGlyphStart) * (m_run.ltr() ? x / adjustedAdvance : 1 - x / adjustedAdvance); If m_run.ltr() was false and x == 0, hitIndex would become hitGlyphEnd. This is not expected. It expects hitIndex < hitGlyphEnd if hitGlyphStart ≠ hitGlyphEnd. Let hitIndex be hitGlyphStart-1 in the such condition. Above change makes fast/text/ellipsis-text-rtl.html starting to fail because offsetForPosition returns the character offset of the next glyph if the argument 'h' is in a glyph boundary. In RTL text, offsetForPosition should return a character offset of the previous glyph in case of a glyph boundary. Use '<=' instead of '<' for RTL text in order to select previous glyphs for glyph boundaries. Test: fast/dom/Document/CaretRangeFromPoint/rtl.html * platform/graphics/ComplexTextController.cpp: (WebCore::ComplexTextController::offsetForPosition): Compute correct hitGlyphEnd for RTL. LayoutTests: * fast/dom/Document/CaretRangeFromPoint/rtl-expected.txt: Added. * fast/dom/Document/CaretRangeFromPoint/rtl.html: Added. * platform/gtk/TestExpectations: Unmarked imported/blink/editing/selection/offset-from-point-complex-scripts.html.
WebKit · Jan 22, 2020 · d214e17 · d214e17
1 parent c6c0cec
commit d214e17
Show file tree

Hide file tree

Showing 6 changed files with 96 additions and 3 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,14 @@
+2020-01-06  Fujii Hironori  <Hironori.Fujii@sony.com>
+
+        ComplexTextController::offsetForPosition returns a wrong offset for a glyph boundary in a RTL text
+        https://bugs.webkit.org/show_bug.cgi?id=205486
+
+        Reviewed by Ross Kirsling.
+
+        * fast/dom/Document/CaretRangeFromPoint/rtl-expected.txt: Added.
+        * fast/dom/Document/CaretRangeFromPoint/rtl.html: Added.
+        * platform/gtk/TestExpectations: Unmarked imported/blink/editing/selection/offset-from-point-complex-scripts.html.
+
 2020-01-08  Fujii Hironori  <Hironori.Fujii@sony.com>
 
         [HarfBuzz][GTK] fast/text/complex-first-glyph-with-initial-advance.html is failing

diff --git a/LayoutTests/fast/dom/Document/CaretRangeFromPoint/rtl-expected.txt b/LayoutTests/fast/dom/Document/CaretRangeFromPoint/rtl-expected.txt
@@ -0,0 +1 @@
+6 5 4 3 2 1 0
diff --git a/LayoutTests/fast/dom/Document/CaretRangeFromPoint/rtl.html b/LayoutTests/fast/dom/Document/CaretRangeFromPoint/rtl.html
@@ -0,0 +1,42 @@
+<!DOCTYPE html>
+<head>
+  <meta charset="utf-8">
+  <style>
+      p {
+          font-family: "Ahem";
+          direction: rtl;
+          unicode-bidi: bidi-override;
+      }
+  </style>
+</head>
+<div id="container">
+    <p>ABCDEF</p>
+</div>
+<div id=log></div>
+<script>
+testOffsetFromPoint(container.firstElementChild);
+
+function testOffsetFromPoint(element) {
+    var y = element.offsetTop + element.offsetHeight / 2;
+    var xmin = element.offsetLeft;
+    var xmax = xmin + element.offsetWidth;
+    var lastCharacterOffset = null;
+    var results = [];
+    for (var x = xmin - 1; x <= xmax + 1; ++x) {
+        var result = document.caretRangeFromPoint(x, y);
+        var characterOffset = result ? result.startOffset : null;
+        if (characterOffset === lastCharacterOffset)
+            continue;
+        results.push(characterOffset);
+        lastCharacterOffset = characterOffset;
+    }
+    var div = document.createElement("div");
+    div.innerText = results.join(" ");
+    log.appendChild(div);
+}
+
+if (window.testRunner) {
+    container.style.display = "none";
+    testRunner.dumpAsText();
+}
+</script>
diff --git a/LayoutTests/platform/gtk/TestExpectations b/LayoutTests/platform/gtk/TestExpectations
@@ -3372,7 +3372,6 @@ webkit.org/b/177934 http/tests/preconnect/link-header-rel-preconnect-http.html [
 webkit.org/b/177936 fast/text/all-small-caps-whitespace.html [ ImageOnlyFailure ]
 webkit.org/b/177936 fast/text/regional-indicator-symobls.html [ Failure ]
 webkit.org/b/177936 fast/text/selection-in-initial-advance-region.html [ Failure ]
-webkit.org/b/177936 imported/blink/editing/selection/offset-from-point-complex-scripts.html [ Failure ]
 
 webkit.org/b/177937 fast/text/soft-hyphen-min-preferred-width.html [ ImageOnlyFailure ]
 webkit.org/b/177937 fast/text/unknown-char-notdef.html [ ImageOnlyFailure ]

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,31 @@
+2020-01-06  Fujii Hironori  <Hironori.Fujii@sony.com>
+
+        ComplexTextController::offsetForPosition returns a wrong offset for a glyph boundary in a RTL text
+        https://bugs.webkit.org/show_bug.cgi?id=205486
+
+        Reviewed by Ross Kirsling.
+
+        ComplexTextController::offsetForPosition had the following code:
+
+        > unsigned hitIndex = hitGlyphStart + (hitGlyphEnd - hitGlyphStart) * (m_run.ltr() ? x / adjustedAdvance : 1 - x / adjustedAdvance);
+
+        If m_run.ltr() was false and x == 0, hitIndex would become hitGlyphEnd.
+        This is not expected. It expects hitIndex < hitGlyphEnd if hitGlyphStart ≠ hitGlyphEnd.
+        Let hitIndex be hitGlyphStart-1 in the such condition.
+
+        Above change makes fast/text/ellipsis-text-rtl.html starting to
+        fail because offsetForPosition returns the character offset of the
+        next glyph if the argument 'h' is in a glyph boundary. In RTL
+        text, offsetForPosition should return a character offset of the
+        previous glyph in case of a glyph boundary. Use '<=' instead of '<'
+        for RTL text in order to select previous glyphs for glyph
+        boundaries.
+
+        Test: fast/dom/Document/CaretRangeFromPoint/rtl.html
+
+        * platform/graphics/ComplexTextController.cpp:
+        (WebCore::ComplexTextController::offsetForPosition): Compute correct hitGlyphEnd for RTL.
+
 2020-01-08  Fujii Hironori  <Hironori.Fujii@sony.com>
 
         [HarfBuzz][GTK] fast/text/complex-first-glyph-with-initial-advance.html is failing

diff --git a/Source/WebCore/platform/graphics/ComplexTextController.cpp b/Source/WebCore/platform/graphics/ComplexTextController.cpp
@@ -205,7 +205,8 @@ unsigned ComplexTextController::offsetForPosition(float h, bool includePartialGl
         for (unsigned j = 0; j < complexTextRun.glyphCount(); ++j) {
             unsigned index = offsetIntoAdjustedGlyphs + j;
             float adjustedAdvance = m_adjustedBaseAdvances[index].width();
-            if (x < adjustedAdvance) {
+            bool hit = m_run.ltr() ? x < adjustedAdvance : (x <= adjustedAdvance && adjustedAdvance);
+            if (hit) {
                 unsigned hitGlyphStart = complexTextRun.indexAt(j);
                 unsigned hitGlyphEnd;
                 if (m_run.ltr())
@@ -215,7 +216,18 @@ unsigned ComplexTextController::offsetForPosition(float h, bool includePartialGl
 
                 // FIXME: Instead of dividing the glyph's advance equally between the characters, this
                 // could use the glyph's "ligature carets". This is available in CoreText via CTFontGetLigatureCaretPositions().
-                unsigned hitIndex = hitGlyphStart + (hitGlyphEnd - hitGlyphStart) * (m_run.ltr() ? x / adjustedAdvance : 1 - x / adjustedAdvance);
+                unsigned hitIndex;
+                if (m_run.ltr())
+                    hitIndex = hitGlyphStart + (hitGlyphEnd - hitGlyphStart) * (x / adjustedAdvance);
+                else {
+                    if (hitGlyphStart == hitGlyphEnd)
+                        hitIndex = hitGlyphStart;
+                    else if (x)
+                        hitIndex = hitGlyphEnd - (hitGlyphEnd - hitGlyphStart) * (x / adjustedAdvance);
+                    else
+                        hitIndex = hitGlyphEnd - 1;
+                }
+
                 unsigned stringLength = complexTextRun.stringLength();
                 CachedTextBreakIterator cursorPositionIterator(StringView(complexTextRun.characters(), stringLength), TextBreakIterator::Mode::Caret, nullAtom());
                 unsigned clusterStart;