Bug 59435

<rdar://problem/9338216>

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

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

Created attachment 91359 [details] demo

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.

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.

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.

(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.

(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.

(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.

(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?

(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.

Created attachment 91409 [details] work in progress I think this would do it (at least passes all the tests).

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.

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.

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)

Created attachment 91989 [details] work in progress 3 Here's a patch that mostly works but regressed some tests in fast/forms.

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

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

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

Ping reviewers.

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

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.

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

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

Created attachment 130408 [details] Fixes one last bug

Comment on attachment 130408 [details] Fixes one last bug LGTM.

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

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.

(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.

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)

(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.

Mouse selection bug filed as Issue 80535 .

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