Fix cursor placement at left and right ends of bidi text

In a text line that has a change of direction at either end of the text,
the cursor needs to be positioned where the next character is inserted,
or where backspace deletes the previous character. In bidi text, this is
ambiguous as illustrated by this example:

abcشزذ

Depending on whether this string was typed in a left-to-right document
or in a right-to-left document, it could be first latin, then arabic; or
it could be first arabic, then latin.

If a general left-to-right context, cursor position 0 should be in front
of the 'a', and cursor position 6 should be at the end of the arabic
text, in the visual middle of the line. Cursor position 3 can be either
after the 'c' if the next character typed would be latin, or at the
visual end of the line if the next character will be arabic.

Qt calculated the cursor position past the right end of the text as 3
(which is not wrong, but 3 has two visual positions), and placed the
cursor at the visual end of the line (favoring the right-to-left
alternative). Backspace would then delete the 'c', writing a new
latin character would insert a 'd' next to the 'c', writing a new arabic
character would insert it also in the middle - none of these operations
happen at the visual end of the line, where the cursor was blinking.

To fix this, we take into account the general layout of the text, which
is typically based on the document, or the user's locale setting and UI
translation, and calculate the cursor position accordingly: if we are
past the visual end of the document on either side, then the cursor
position is either 0 or the last character of the text, depending on the
direction of the QTextEngine used. This way, the cursor ends up in the
middle of the document when we click beyond the end of the line, which
is where characters are removed and inserted. Typing a 'd' at this point
will make the cursor jump to the end, where the d is added.

There are still corner cases: clicking on the right-most arabic character
calculates the cursor position as 3, which is then ambiguous, as it can
be either at the visual end of the string, or next to the 'c'. َQt makes
the inconsistent choice to place the cursor at the visual end, showing
the left-to-right indicator, but pressing a 'd' adds the 'd' after the
'c' in the middle of the text.

Fixes: QTBUG-88529
Pick-to: 6.2
Change-Id: Idccd4c4deead2bce0e858189f9aef414857eb8af
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

src/gui/text/qtextlayout.cpp

@@ -2909,18 +2909,10 @@ int QTextLine::xToCursor(qreal _x, CursorPosition cpos) const
     bool visual = eng->visualCursorMovement();
     if (x <= 0) {
         // left of first item
-        int item = visualOrder[0]+firstItem;
-        QScriptItem &si = eng->layoutData->items[item];
-        if (!si.num_glyphs)
-            eng->shape(item);
-        int pos = si.position;
-        if (si.analysis.bidiLevel % 2)
-            pos += eng->length(item);
-        pos = qMax(line.from, pos);
-        pos = qMin(line.from + line_length, pos);
-        return pos;
-    } else if (x < line.textWidth
-               || (line.justified && x < line.width)) {
+        if (eng->isRightToLeft())
+            return line.from + line_length;
+        return line.from;
+    }   else if (x < line.textWidth || (line.justified && x < line.width)) {
         // has to be in one of the runs
         QFixed pos;
         bool rtl = eng->isRightToLeft();
@@ -3070,26 +3062,17 @@ int QTextLine::xToCursor(qreal _x, CursorPosition cpos) const
         }
     }
     // right of last item
-//     qDebug("right of last");
-    int item = visualOrder[nItems-1]+firstItem;
-    QScriptItem &si = eng->layoutData->items[item];
-    if (!si.num_glyphs)
-        eng->shape(item);
-    int pos = si.position;
-    if (!(si.analysis.bidiLevel % 2))
-        pos += eng->length(item);
-    pos = qMax(line.from, pos);
-
-    int maxPos = line.from + line_length;
+    int pos = line.from;
+    if (!eng->isRightToLeft())
+        pos += line_length;
 
     // except for the last line we assume that the
     // character between lines is a space and we want
     // to position the cursor to the left of that
     // character.
-    if (this->index < eng->lines.count() - 1)
-        maxPos = eng->previousLogicalPosition(maxPos);
+    if (index < eng->lines.count() - 1)
+        pos = qMin(eng->previousLogicalPosition(pos), pos);
 
-    pos = qMin(pos, maxPos);
     return pos;
 }
 

tests/auto/gui/text/qtextlayout/tst_qtextlayout.cpp

@@ -68,11 +68,13 @@ private slots:
     void forcedBreaks();
     void breakAny();
     void noWrap();
+
     void cursorToXForInlineObjects();
     void cursorToXForSetColumns();
     void cursorToXForTrailingSpaces_data();
     void cursorToXForTrailingSpaces();
     void cursorToXInvalidInput();
+
     void horizontalAlignment_data();
     void horizontalAlignment();
     void horizontalAlignmentMultiline_data();
@@ -86,6 +88,8 @@ private slots:
 #ifdef QT_BUILD_INTERNAL
     void xToCursorAtEndOfLine();
 #endif
+    void xToCursorForBidiEnds_data();
+    void xToCursorForBidiEnds();
     void boundingRectTopLeft();
     void graphemeBoundaryForSurrogatePairs();
     void tabStops();
@@ -1126,6 +1130,56 @@ void tst_QTextLayout::xToCursorAtEndOfLine()
 }
 #endif
 
+
+void tst_QTextLayout::xToCursorForBidiEnds_data()
+{
+    QTest::addColumn<Qt::LayoutDirection>("textDirection");
+    QTest::addColumn<QString>("text");
+    QTest::addColumn<int>("leftPosition");
+    QTest::addColumn<int>("rightPosition");
+
+    QTest::addRow("LTR, abcشزذ") << Qt::LeftToRight << "abcشزذ"
+        << 0 << 6;
+    QTest::addRow("RTL, abcشزذ") << Qt::RightToLeft << "abcشزذ"
+        << 6 << 0;
+    QTest::addRow("LTR, شزذabc") << Qt::LeftToRight << "شزذabc"
+        << 0 << 6;
+    QTest::addRow("RTL, شزذabc") << Qt::RightToLeft << "شزذabc"
+        << 6 << 0;
+
+    QTest::addRow("LTR, abcشزذabc") << Qt::LeftToRight << "abcشزذabc"
+        << 0 << 9;
+    QTest::addRow("RTL, abcشزذabc") << Qt::RightToLeft << "abcشزذabc"
+        << 9 << 0;
+    QTest::addRow("LTR, شزذabcشزذ") << Qt::LeftToRight << "شزذabcشزذ"
+        << 0 << 9;
+    QTest::addRow("RTL, شزذabcشزذ") << Qt::RightToLeft << "شزذabcشزذ"
+        << 9 << 0;
+}
+
+void tst_QTextLayout::xToCursorForBidiEnds()
+{
+    QFETCH(Qt::LayoutDirection, textDirection);
+    QFETCH(QString, text);
+    QFETCH(int, leftPosition);
+    QFETCH(int, rightPosition);
+
+    QTextOption option;
+    option.setTextDirection(textDirection);
+
+    QTextLayout layout(text, testFont);
+    layout.setTextOption(option);
+    layout.beginLayout();
+
+    QTextLine line = layout.createLine();
+    line.setLineWidth(0x10000);
+
+    QCOMPARE(line.xToCursor(0), leftPosition);
+    QCOMPARE(line.xToCursor(line.width()), rightPosition);
+
+    layout.endLayout();
+}
+
 void tst_QTextLayout::boundingRectTopLeft()
 {
     QString text = "FirstLine\nSecondLine";