Bug 49111

Summary: [RTL] Arabic/AB - after typing a date, cursors doesn't go back
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Severity: Normal CC: adele, commit-queue, darin, mitz, playmobil, rniwa, xji
Priority: P2 Keywords: InRadar, PlatformOnly
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Bug Depends on: 53696    
Bug Blocks:    
Description Flags
fixes the bug
fixed warning
fixes 3 bugs so that my test case finally works none

Description Enrica Casucci 2010-11-05 16:22:56 PDT
1. Open any html editor
3. type  ١١
4. then  مارس
5. then ٢٠١٠
which would stand for 11 mars 2010.
try to move the cursors to another place, to erase the مارس string for example.

It is impossible.
Comment 1 Darin Adler 2010-11-17 14:36:10 PST
Dan suggested this may be a duplicate of bug 33503. Is there someone interested in working on this?
Comment 2 Ryosuke Niwa 2010-12-20 11:24:58 PST
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 مارس.
Comment 3 Ryosuke Niwa 2010-12-20 12:29:16 PST
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:

We if only had ABC in a LTR block, then the corresponding offsets are

Similarly, if we had 12 in a LTR block, then the offsets are:

Now combine these two, we get (on WebKit TOT):

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.
Comment 4 Ryosuke Niwa 2010-12-20 14:14:40 PST
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)?
Comment 5 Ryosuke Niwa 2010-12-21 12:07:59 PST
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)
Comment 6 Ryosuke Niwa 2011-01-10 16:03:55 PST
Firefox does:

IE does:



Firefox does:
(at the boundary, gives 4 when moving from BA and g2 when moving from 12)

IE does:

We should decide on what's the correct behavior.
Comment 7 Adele Peterson 2011-03-01 13:51:21 PST
Comment 8 Ryosuke Niwa 2011-03-31 07:21:40 PDT
Since the bug 53696 turned out be invalid, I'm going to fix this bug without fixing caret rendering.
Comment 9 Ryosuke Niwa 2011-03-31 07:41:35 PDT
Created attachment 87726 [details]
fixes the bug
Comment 10 Ryosuke Niwa 2011-03-31 09:31:40 PDT
Created attachment 87749 [details]
fixed warning
Comment 11 Ryosuke Niwa 2011-03-31 14:05:48 PDT
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.
Comment 12 Ryosuke Niwa 2011-03-31 14:07:42 PDT
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.
Comment 13 Ryosuke Niwa 2011-04-03 08:10:50 PDT
Created attachment 88010 [details]
fixes 3 bugs so that my test case finally works
Comment 14 Ryosuke Niwa 2011-04-03 08:16:37 PDT
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.
Comment 15 Ryosuke Niwa 2011-04-03 10:27:30 PDT
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.
Comment 16 Ryosuke Niwa 2011-04-26 07:56:00 PDT
Yay! Thanks for the review, Dan!
Comment 17 WebKit Commit Bot 2011-04-26 09:40:29 PDT
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>
Comment 18 WebKit Commit Bot 2011-04-26 09:40:33 PDT
All reviewed patches have been landed.  Closing bug.