Bug 59435 - Can't select a line of RTL text on Facebook
Summary: Can't select a line of RTL text on Facebook
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar, NeedsReduction
Depends on: 54929 55481 59811 60000 73056
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-26 00:49 PDT by Jeremy Moskovich
Modified: 2012-03-07 13:20 PST (History)
7 users (show)

See Also:


Attachments
demo (125 bytes, text/html)
2011-04-27 15:21 PDT, Ryosuke Niwa
no flags Details
test case (289 bytes, text/html)
2011-04-27 18:19 PDT, Xiaomei Ji
no flags Details
work in progress (4.29 KB, patch)
2011-04-27 19:25 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
work in progress 2 (19.64 KB, patch)
2011-04-28 19:56 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
work in progress 3 (17.80 KB, patch)
2011-05-02 15:03 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Finally, it's time to fix this old bug (24.16 KB, patch)
2011-12-20 23:25 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Added more descriptions to change log and some comment in InlineBox.h per Eric's suggestion (24.92 KB, patch)
2011-12-22 01:13 PST, Ryosuke Niwa
eric: review+
Details | Formatted Diff | Diff
Fixes one last bug (4.12 KB, patch)
2012-03-06 11:03 PST, Ryosuke Niwa
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Moskovich 2011-04-26 00:49:51 PDT
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
Comment 1 Alexey Proskuryakov 2011-04-26 09:35:05 PDT
<rdar://problem/9338216>
Comment 2 Levi Weintraub 2011-04-26 11:15:23 PDT
This seems very similar to https://bugs.webkit.org/show_bug.cgi?id=57340
Comment 3 Xiaomei Ji 2011-04-27 13:05:25 PDT
Looks like it (and 57340) started regress since
http://trac.webkit.org/changeset/74971/trunk/WebCore/rendering/InlineTextBox.cpp
Comment 4 Ryosuke Niwa 2011-04-27 15:21:45 PDT
Created attachment 91359 [details]
demo
Comment 5 Ryosuke Niwa 2011-04-27 17:11:36 PDT
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.
Comment 6 Ryosuke Niwa 2011-04-27 18:08:44 PDT
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.
Comment 7 Xiaomei Ji 2011-04-27 18:19:44 PDT
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.
Comment 8 Ryosuke Niwa 2011-04-27 18:22:16 PDT
(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.
Comment 9 Xiaomei Ji 2011-04-27 18:34:05 PDT
(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.
Comment 10 Xiaomei Ji 2011-04-27 18:40:26 PDT
(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.
Comment 11 Xiaomei Ji 2011-04-27 18:47:39 PDT
(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?
Comment 12 Ryosuke Niwa 2011-04-27 19:22:48 PDT
(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.
Comment 13 Ryosuke Niwa 2011-04-27 19:25:20 PDT
Created attachment 91409 [details]
work in progress

I think this would do it (at least passes all the tests).
Comment 14 Ryosuke Niwa 2011-04-27 19:42:04 PDT
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.
Comment 15 Ryosuke Niwa 2011-04-27 19:51:21 PDT
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.
Comment 16 Ryosuke Niwa 2011-04-28 19:56:41 PDT
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)
Comment 17 Ryosuke Niwa 2011-05-02 15:03:17 PDT
Created attachment 91989 [details]
work in progress 3

Here's a patch that mostly works but regressed some tests in fast/forms.
Comment 18 Ryosuke Niwa 2011-10-04 14:43:12 PDT
Xiaomei's test case now works after r95964 but my test case doesn't.
Comment 19 Ryosuke Niwa 2011-12-20 23:25:04 PST
Created attachment 120150 [details]
Finally, it's time to fix this old bug
Comment 20 Ryosuke Niwa 2011-12-22 01:13:14 PST
Created attachment 120287 [details]
Added more descriptions to change log and some comment in InlineBox.h per Eric's suggestion
Comment 21 Ryosuke Niwa 2011-12-26 22:01:42 PST
Ping reviewers.
Comment 22 Ryosuke Niwa 2012-03-01 14:12:29 PST
This isn't really a regression anymore. It improves the selection.
Comment 23 Eric Seidel (no email) 2012-03-01 14:23:49 PST
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.
Comment 24 Ryosuke Niwa 2012-03-02 11:54:46 PST
Committed r109593: <http://trac.webkit.org/changeset/109593>
Comment 25 Ryosuke Niwa 2012-03-06 10:49:38 PST
Oops, re-open the bug since there's one place I need to deploy *ChildIgnoringLineBreak.
Comment 26 Ryosuke Niwa 2012-03-06 11:03:32 PST
Created attachment 130408 [details]
Fixes one last bug
Comment 27 Eric Seidel (no email) 2012-03-06 12:21:51 PST
Comment on attachment 130408 [details]
Fixes one last bug

LGTM.
Comment 28 Ryosuke Niwa 2012-03-06 17:14:03 PST
Committed r109984: <http://trac.webkit.org/changeset/109984>
Comment 29 Jeremy Moskovich 2012-03-07 09:24:43 PST
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.
Comment 30 Ryosuke Niwa 2012-03-07 09:48:45 PST
(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.
Comment 31 Yair Yogev 2012-03-07 09:53:26 PST
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)
Comment 32 Ryosuke Niwa 2012-03-07 10:21:54 PST
(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.
Comment 33 Jeremy Moskovich 2012-03-07 13:20:20 PST
Mouse selection bug filed as Issue 80535 .