REGRESSION: LayoutTests/editing/selection/caret-rtl-2.html fails

[Dimitri Glazkov (Google)]

This is only evident when run with --pixel-tests --tolerance 0.

The caret is between the first and second character. Is this what is supposed to be happening?

[Dimitri Glazkov (Google)]

Created attachment 46319 [details]
Actual result.

<rdar://problem/7531574>

Good: http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/editing/selection/caret-rtl-2-expected.png?rev=35404
Bad: http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/editing/selection/caret-rtl-2-expected.png?rev=38122
Worse: TOT

The change from Bad to Worse probably happened in http://trac.webkit.org/changeset/42685 and http://trac.webkit.org/changeset/42686 .

[Tony Chang]

*** Bug 34692 has been marked as a duplicate of this bug. ***

[Xiaomei Ji]

(In reply to comment #3)
> Good:
> http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/editing/selection/caret-rtl-2-expected.png?rev=35404
> Bad:
> http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/editing/selection/caret-rtl-2-expected.png?rev=38122
> Worse: TOT

Looks like the patch for 
https://bugs.webkit.org/show_bug.cgi?id=25319
will change the result from Worse to Bad.

Is from bad to good related to your comment #3 in
https://bugs.webkit.org/show_bug.cgi?id=24278 ?

[Ryosuke Niwa]

We should make this use dump-as-markup.js.  We can then detect failure without using --tolerance 0.

[Ryosuke Niwa]

The problem is that when I click to the right of "שדה בור", InlineTextBox::offsetForPosition returns offset 0 but it's ambiguous whether this is wrong or not.

This is correct if this inline text box was in a RTL context but it's wrong if this inline text box was inside a LTR context.  This bug is related to the bug 24278.

[Ryosuke Niwa]

With a lot of help from jamesr, I think I have a fix for this bug:


Index: WebCore/rendering/InlineTextBox.cpp
===================================================================
--- WebCore/rendering/InlineTextBox.cpp	(revision 73467)
+++ WebCore/rendering/InlineTextBox.cpp	(working copy)
@@ -1100,8 +1100,14 @@
     RenderText* text = toRenderText(renderer());
     RenderStyle* style = text->style(m_firstLine);
     const Font* f = &style->font();
+    if (renderer()->containingBlock()->style()->isLeftToRightDirection() != isLeftToRightDirection()) {
+        if (lineOffset - logicalLeft() > logicalWidth())
+            return isLeftToRightDirection() ? 0 : m_len;
+        else if (lineOffset - logicalLeft() < 0)
+            return isLeftToRightDirection() ? m_len : 0;
+    }
     return f->offsetForPosition(TextRun(textRenderer()->text()->characters() + m_start, m_len, textRenderer()->allowTabs(), textPos(), m_toAdd, !isLeftToRightDirection(), m_dirOverride || style->visuallyOrdered()),
-                                lineOffset - logicalLeft(), includePartialGlyphs);
+        lineOffset - logicalLeft(), includePartialGlyphs);
 }
 
I'll test this code a little more and post a patch tomorrow.

[Ryosuke Niwa]

Created attachment 75880 [details]
fixes the bug

[Ryosuke Niwa]

(In reply to comment #10)
> Created an attachment (id=75880) [details]
> fixes the bug

This patch is massive only because I added a lot of tests.  The code change in WebCore is just 6 lines.

I just verified that WebKit, with my patch, does the right thing in the middle of LTR / RTL boundary:
<div>hello<span style="padding-left: 20px;">בור</span> webkit</div>

I rather add this test as a separate patch though since this patch is already 300KB+.

[Darin Adler]

Comment on attachment 75880 [details]
fixes the bug

This seems related to the notion of a “split caret”. Have we decided what to do about that case?

Dan, I think you are the best reviewer for this patch; code change looks fine to me, but I want to make sure we are headed in the right direction on this.

[Ryosuke Niwa]

(In reply to comment #12)
> (From update of attachment 75880 [details])
> This seems related to the notion of a “split caret”. Have we decided what to do about that case?

Right.  On Mac, at least, this is the case where we show a split caret (the bug 3710).  However, there is a heated discussion about whether or not Chromium port should support split caret internally at Google, and a group of engineers in Israel are discussing the most intuitive behavior for users. We may conduct some UX research as well. Because of this, Chromium port can't adopt the split caret at the moment.  We'll get back to you once reached some consensus.

Furthermore, regardless of whether or not we adopt split caret, the bug 3710 seems to require significant amount of work given that your patch posted in 2005 is 50KB.  Since this bug (33503) is a regression and affects a large amount of users, I wanted to fix it before we can reach consensus for the bug 3710.

Comment on attachment 75880 [details]
fixes the bug

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

> WebCore/ChangeLog:10
> +        adn the offset at the end of RTL text is on the right of RTL text. For example, if we had RTL text CBA,

Typo: adn

> WebCore/ChangeLog:22
> +        The bug was caused by WebKit's not considering case 2. The same bug also existe for LTR text in a RTL block.

Typo: existe

> WebCore/rendering/InlineTextBox.cpp:1107
> +        if (lineOffset - logicalLeft() > logicalWidth())
> +            return isLeftToRightDirection() ? 0 : m_len;
> +        if (lineOffset - logicalLeft() < 0)
> +            return isLeftToRightDirection() ? m_len : 0;

Can the call to Font::offsetForPosition() always be skipped in these cases (not only when the directions differ)?

[Ryosuke Niwa]

Thanks for the review, Dan!

(In reply to comment #14)
> (From update of attachment 75880 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75880&action=review
> 
> > WebCore/ChangeLog:10
> > +        adn the offset at the end of RTL text is on the right of RTL text. For example, if we had RTL text CBA,
> 
> Typo: adn

Will fix.

> > WebCore/ChangeLog:22
> > +        The bug was caused by WebKit's not considering case 2. The same bug also existe for LTR text in a RTL block.
> 
> Typo: existe

Will fix.

> > WebCore/rendering/InlineTextBox.cpp:1107
> > +        if (lineOffset - logicalLeft() > logicalWidth())
> > +            return isLeftToRightDirection() ? 0 : m_len;
> > +        if (lineOffset - logicalLeft() < 0)
> > +            return isLeftToRightDirection() ? m_len : 0;
> 
> Can the call to Font::offsetForPosition() always be skipped in these cases (not only when the directions differ)?

That's a good point.  We need to reverse the values in that case though.  I'll try & see.

[Ryosuke Niwa]

Created attachment 75940 [details]
Fixed per Dan's comment

[Ryosuke Niwa]

Committed r73548: <http://trac.webkit.org/changeset/73548>

[Ryosuke Niwa]

Oops, I forgot to fix those typos in my commit.  Fixed in http://trac.webkit.org/changeset/73553.