Bug 49111 – [RTL] Arabic/AB - after typing a date, cursors doesn't go back

• Component: HTML Editing
• Status: RESOLVED FIXED

[Enrica Casucci]

1. Open any html editor
3. type  ١١
4. then  مارس
5. then ٢٠١٠
which would stand for 11 mars 2010.
* RESULTS
try to move the cursors to another place, to erase the مارس string for example.

It is impossible.

[Darin Adler]

Dan suggested this may be a duplicate of bug 33503. Is there someone interested in working on this?

[Ryosuke Niwa]

This bug is a combination of a bug in VisiblePosition::rightVisuallyDistinctCandidate and in the caret rendering.

Let me elaborate on the caret rendering first:
Suppose we're rendering ١١مارس,
then the caret at offset 2 is rendered between ١١ and مارس in a RTL block but rendered at the right edge in a LTR block. However, the caret is rendered at the right edge for offset 6 as well.  As a result, there's no way for us to render the caret between ١١ and مارس.

[Ryosuke Niwa]

The problem comes from the fact arabic numerals are rendered left to right yet flow like RTL text.  Say we had 12ABC where 12 are arabic numerals and ABC are arabic letters.  Then it's rendered as:
CBA12.

We if only had ABC in a LTR block, then the corresponding offsets are
(0)C(2)B(1)A(3).

Similarly, if we had 12 in a LTR block, then the offsets are:
(0)1(1)2(2).

Now combine these two, we get (on WebKit TOT):
(0)C(4)B(3)A1(1)2(2)(5). 

Clearly, either offset 2 or 5 must be between A and 1.  On Firefox, the offset 2 is between A and 1 so I'm inclined to say that we should do the same but this would break a lot of assumptions we make and likely to require a lot of code change.

[Ryosuke Niwa]

It seems that we need to fix InlineTextBox::positionForOffset to return the correct offset for the caret to be rendered properly.  But we're stuck here because InlineTextBox of "١١" doesn't know anything about Arabic numerals and it's just a regular LTR text box.

But for this very specific case, returning x() instead of right() of the enclosingIntRect fix the problem.  So I'm almost certain that this is where we need a fix.

Dan & Xiaomei, do you know a properly way to detect that an inline text box flows right to left (i.e. a LTR inline text box that is in a RTL embedding)?

[Ryosuke Niwa]

I think the following change fix the caret rendering:

diff --git a/WebCore/rendering/InlineTextBox.cpp b/WebCore/rendering/InlineTextBox.cpp
index 5196854..a528730 100644
--- a/WebCore/rendering/InlineTextBox.cpp
+++ b/WebCore/rendering/InlineTextBox.cpp
@@ -1174,8 +1179,13 @@ int InlineTextBox::positionForOffset(int offset) const
     int from = !isLeftToRightDirection() ? offset - m_start : 0;
     int to = !isLeftToRightDirection() ? m_len : offset - m_start;
     // FIXME: Do we need to add rightBearing here?
-    return enclosingIntRect(f.selectionRectForText(TextRun(text->text()->characters() + m_start, m_len, textRenderer()->allowTabs(), textPos(), m_toAdd, !isLeftToRight
-                                                   IntPoint(logicalLeft(), 0), 0, from, to)).right();
+    IntRect rect = enclosingIntRect(f.selectionRectForText(TextRun(text->text()->characters() + m_start, m_len, textRenderer()->allowTabs(), textPos(), m_toAdd, !isLef
+                                                   IntPoint(logicalLeft(), 0), 0, from, to));
+    if (prevTextBox() && prevTextBox()->bidiLevel() < bidiLevel() && offset - m_start == m_len)
+        return rect.x();
+    if (nextTextBox() && nextTextBox()->bidiLevel() < bidiLevel() && offset == m_start)
+        return x() + width();
+    return rect.right();
 }

We also need to fix InlineTextBox::offsetForPosition and RenderText::positionForPoint or otherwise there will be no way for a user to put caret properly.  Right now, RenderText::positionForPoint calls offsetForPosition on a wrong inline text box.

The way I see it, we should split the fix into 3 pieces:
1. Fix caret rendering (above changes)
2. Fix offsetForPosition / positionForPoint
3. Fix VisiblePosition code (this bug)

[Ryosuke Niwa]

Firefox does:
(4)B(3)A(0)1(1)2(2)

IE does:

(4)B(3)A(0)1(1)2(2)

However

Firefox does:
(2)1(3)2(4)(2)B(1)A(0)
(at the boundary, gives 4 when moving from BA and g2 when moving from 12)

IE does:
1(3)2(4)(2)B(1)A(0)

We should decide on what's the correct behavior.

[Adele Peterson]

<rdar://problem/7639888>

[Ryosuke Niwa]

Since the bug 53696 turned out be invalid, I'm going to fix this bug without fixing caret rendering.

[Ryosuke Niwa]

Created attachment 87726 [details]
fixes the bug

[Ryosuke Niwa]

Created attachment 87749 [details]
fixed warning

[Ryosuke Niwa]

Comment on attachment 87749 [details]
fixed warning

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

> Source/WebCore/editing/VisiblePosition.cpp:205
> +                    if (primaryDirection == LTR)
> +                        boxWithMinimumOffsetOnSameLine(box, offset);
> +                    else
> +                        boxWithMaximumOffsetOnSameLine(box, offset);

Ugh... this change is wrong.

[Ryosuke Niwa]

Comment on attachment 87726 [details]
fixes the bug

I'll r? the original patch since its change isn't wrong although it doesn't fix all the bugs.

[Ryosuke Niwa]

Created attachment 88010 [details]
fixes 3 bugs so that my test case finally works

[Ryosuke Niwa]

Even with my patch, we still loop in the following case:
<div dir="rtl">x<span dir="rtl">abcקקקdef</span>y</div>

But I guess we can fix that in a separate patch.

[Ryosuke Niwa]

Comment on attachment 88010 [details]
fixes 3 bugs so that my test case finally works

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

> Source/WebCore/ChangeLog:13
> +        The bug was caused by our ignoring such cases. Fixed the bug by comparing
> +        previous/next position's inline box to the current box. If they match,

Oops, I should be talking about root inline boxes here instead.

> Source/WebCore/ChangeLog:19
> +        Also fixed a bug that WebKit uses offsets that are not extrema when moved to
> +        the left edge or to the right edge, and a bug that WebKit could not move to
> +        the left from 12^3 CBA abc to 123 C^BA abc (there is no offset between 3 and C).

I wanted to elaborate more on these points but these cases are so obscure that I don't know how to explain it well other than pointing to the code.

[Ryosuke Niwa]

Yay! Thanks for the review, Dan!

[WebKit Commit Bot]

Comment on attachment 88010 [details]
fixes 3 bugs so that my test case finally works

Clearing flags on attachment: 88010

Committed r84919: <http://trac.webkit.org/changeset/84919>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.