Summary: | REGRESSION: LayoutTests/editing/selection/caret-rtl-2.html fails | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dimitri Glazkov (Google) <dglazkov> | ||||||||
Component: | Text | Assignee: | Ryosuke Niwa <rniwa> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | darin, hyatt, jamesr, mitz, rniwa, tony, xji | ||||||||
Priority: | P1 | Keywords: | InRadar, LayoutTestFailure, Regression | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Dimitri Glazkov (Google)
2010-01-11 16:41:00 PST
Created attachment 46319 [details]
Actual result.
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 . *** Bug 34692 has been marked as a duplicate of this bug. *** (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 ? We should make this use dump-as-markup.js. We can then detect failure without using --tolerance 0. 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. 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. Created attachment 75880 [details]
fixes the bug
(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+. 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.
(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)? 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. Created attachment 75940 [details]
Fixed per Dan's comment
Committed r73548: <http://trac.webkit.org/changeset/73548> Oops, I forgot to fix those typos in my commit. Fixed in http://trac.webkit.org/changeset/73553. |