Bug 59435 – Can't select a line of RTL text on Facebook

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jeremy Moskovich]

Steps to reproduce:
* Log into Facebook and try to select a full line of RTL text by dragging from one end of the text to the other.

Expected result:
* Entire line should be selected

Actual result:
* Everything works fine until you try to drag over the last letter at which point the selection is cleared :(

Browsers tested:
Safari 5.0.3: OK
WebKit r84622: FAIL

[Alexey Proskuryakov]

<rdar://problem/9338216>

[Levi Weintraub]

This seems very similar to https://bugs.webkit.org/show_bug.cgi?id=57340

[Xiaomei Ji]

Looks like it (and 57340) started regress since
http://trac.webkit.org/changeset/74971/trunk/WebCore/rendering/InlineTextBox.cpp

[Ryosuke Niwa]

Created attachment 91359 [details]
demo

[Ryosuke Niwa]

Mn... odd. we're getting offset 3 for both of the following markups when I click on the right of the text:
<div contenteditable>ابص</p>
<div contenteditable><span dir="rtl">ابص<br><br></span></p>

but the caret is rendered on the left for the second case. Let me investigate it further.

[Ryosuke Niwa]

The root cause of this bug is inline text boxes created by <br>.  It seems like we need to ignore those inline boxes in Position::getInlineBoxAndOffset when looking for next/previous leaf child.

[Xiaomei Ji]

Created attachment 91394 [details]
test case

I attached this simple html with a <p> and <div> (no <br>), 
but the selection is not correct in the ways that:
1. select from left to right, you can not select the rightmost character.
2. select from rightmost to left, you can not select the rightmost character.

[Ryosuke Niwa]

(In reply to comment #7)
> I attached this simple html with a <p> and <div> (no <br>), 
> but the selection is not correct in the ways that:
> 1. select from left to right, you can not select the rightmost character.
> 2. select from rightmost to left, you can not select the rightmost character.

This should be a separate bug.

[Xiaomei Ji]

(In reply to comment #7)
> Created an attachment (id=91394) [details]
> test case
> 
> I attached this simple html with a <p> and <div> (no <br>), 
> but the selection is not correct in the ways that:
> 1. select from left to right, you can not select the rightmost character.
> 2. select from rightmost to left, you can not select the rightmost character.

For example, given the logical text "abcABC" in LTR context, whose display is "abcCBA", when caret is placed rightmost at "abcCBA|", the returned offset from 
http://trac.webkit.org/changeset/74971/trunk/WebCore/rendering/InlineTextBox.cpp is swapped from 0 to 3 due to the box and its containing block are in different directionality. Select left and stops at "abcCB|A", the offset returned is 1, which is correct. The correct selection range should be from offset 0 to 1 and selects 'A', instead, the selection range is from offset 3 to 1 and "CB" is selected.

Similar when select left to right.

[Xiaomei Ji]

(In reply to comment #8)
> (In reply to comment #7)
> > I attached this simple html with a <p> and <div> (no <br>), 
> > but the selection is not correct in the ways that:
> > 1. select from left to right, you can not select the rightmost character.
> > 2. select from rightmost to left, you can not select the rightmost character.
> 
> This should be a separate bug.

I think you are right. This bug is about "selection is cleared", not "selection is mis-placed".

Then, I do not know whether it is caused by 74971. Sorry for the false alarm.

[Xiaomei Ji]

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > I attached this simple html with a <p> and <div> (no <br>), 
> > > but the selection is not correct in the ways that:
> > > 1. select from left to right, you can not select the rightmost character.
> > > 2. select from rightmost to left, you can not select the rightmost character.
> > 
> > This should be a separate bug.
> 
> I think you are right. This bug is about "selection is cleared", not "selection is mis-placed".
> 
> Then, I do not know whether it is caused by 74971. Sorry for the false alarm.

You might be right about the <br> in comment #6. 
But blindly removing the offset swapping at line 1165 in http://trac.webkit.org/changeset/74971/trunk/WebCore/rendering/InlineTextBox.cpp will make your demo test case work (selection could select the whole line).

I am wondering whether the logical at line 1165 is correct or could be as simple as that?

[Ryosuke Niwa]

(In reply to comment #11)
> You might be right about the <br> in comment #6. 
> But blindly removing the offset swapping at line 1165 in http://trac.webkit.org/changeset/74971/trunk/WebCore/rendering/InlineTextBox.cpp will make your demo test case work (selection could select the whole line).

Sure but that's because the bug I fixed happens to cancel-out this bug.

> I am wondering whether the logical at line 1165 is correct or could be as simple as that?

I believe the logic in line 1165 is correct.

[Ryosuke Niwa]

Created attachment 91409 [details]
work in progress

I think this would do it (at least passes all the tests).

[Ryosuke Niwa]

This bug is about getInlineBoxAndOffset giving a wrong result when we're at the end of a line and the line break's text direction doesn't match that of the containing block.

[Ryosuke Niwa]

An alternative approach is to use the containing block's direction to create inline boxes for line breaks.  That might just work although I'll have to check if that's even allowed in UBA/HTML5.

[Ryosuke Niwa]

Created attachment 91623 [details]
work in progress 2

It turned out that fixing this bug requires fixing a whole bunch of other bugs.  In general, we need to avoid getting an inline text box that's a line break in hit testing and caret rendering.

Xiaomei & Dan, could you take a look at the current patch and see if you can find any serious issues with it? (passes all layout tests)

[Ryosuke Niwa]

Created attachment 91989 [details]
work in progress 3

Here's a patch that mostly works but regressed some tests in fast/forms.

[Ryosuke Niwa]

Xiaomei's test case now works after r95964 but my test case doesn't.

[Ryosuke Niwa]

Created attachment 120150 [details]
Finally, it's time to fix this old bug

[Ryosuke Niwa]

Created attachment 120287 [details]
Added more descriptions to change log and some comment in InlineBox.h per Eric's suggestion

[Ryosuke Niwa]

Ping reviewers.

[Ryosuke Niwa]

This isn't really a regression anymore. It improves the selection.

[Eric Seidel (no email)]

Comment on attachment 120287 [details]
Added more descriptions to change log and some comment in InlineBox.h per Eric's suggestion

Thank you for the in-person explanation.  This looks fine.

[Ryosuke Niwa]

Committed r109593: <http://trac.webkit.org/changeset/109593>

[Ryosuke Niwa]

Oops, re-open the bug since there's one place I need to deploy *ChildIgnoringLineBreak.

[Ryosuke Niwa]

Created attachment 130408 [details]
Fixes one last bug

[Eric Seidel (no email)]

Comment on attachment 130408 [details]
Fixes one last bug

LGTM.

[Ryosuke Niwa]

Committed r109984: <http://trac.webkit.org/changeset/109984>

[Jeremy Moskovich]

Thanks for fixing this Ryosuke!

One weird case I still see is that in the testcase, if I drag a selection starting from 'a' then when I reach the 'ב', I'd expect the whole run to be selected but instead I get the whole run selected apart from the 'ב'.

The only way to select the whole string is to drag all the way to the right of the string.

IMHO FF's behavior where dragging to the center of the string selects the whole string is better for this corner case.

[Ryosuke Niwa]

(In reply to comment #29)
> One weird case I still see is that in the testcase, if I drag a selection starting from 'a' then when I reach the 'ב', I'd expect the whole run to be selected but instead I get the whole run selected apart from the 'ב'.

Could you file a separate bug for it?  As far as I understand it, this is a bug in our hit testing code and the fix will be touching quite different places in the codebase.

[Yair Yogev]

i think you described http://crbug.com/90580
(i was waiting for the canary to catch this change to see if anything changed regarding it)

[Ryosuke Niwa]

(In reply to comment #31)
> i think you described http://crbug.com/90580
> (i was waiting for the canary to catch this change to see if anything changed regarding it)

This patch won't fix that Chromium issue. The issue had been incorrectly marked as related to the bug 57340 and a regression so I've removed those labels. We need a separate bug if we wanted to fix that issue.

[Jeremy Moskovich]

Mouse selection bug filed as Issue 80535 .

[Ahmad Saleem]

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