Bug 50992 – Clicking on the first or the last letter of LTR/RTL text in a RTL/LTR block puts caret on the opposite side

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

This is a followup to the bug 33503.  I fixed the case where you click on the left or the right of LTR/RTL text in a RTL/LTR block but I missed the case where we clicked the first or the last letter.

Can you give an example of what you mean?

[Ryosuke Niwa]

(In reply to comment #1)
> Can you give an example of what you mean?

Open caret-ltr-2.html or caret-rtl-2.html and click on the first or the last letter of text.  The caret is placed on the opposite side.

(In reply to comment #2)
> (In reply to comment #1)
> > Can you give an example of what you mean?
> 
> Open caret-ltr-2.html or caret-rtl-2.html and click on the first or the last letter of text.  The caret is placed on the opposite side.

I see correct behavior: in caret-ltr-2.html, if I click in the left half of the W, the insertion point appears to the right of “2”. If I click in the right half of the “2”, the insertion point appears to the left of “W”.

(This is with r73922).

Never mind. I wasn’t testing the right build. However, it now appears that in r73922, clicking inside a letter behaves correctly, but clicking on either side of the text behaves incorrectly.

I am no longer certain http://trac.webkit.org/changeset/73548 was right.

One last comment (for now): this bug is valid. It suggest to me that <http://trac.webkit.org/changeset/73548> was not the right way to fix bug 33503.

[Ryosuke Niwa]

Created attachment 76468 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 76468 [details]
Patch

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

> WebCore/rendering/InlineTextBox.cpp:1116
> +    if ((offset == 0 || offset == m_len) && renderer()->containingBlock()->style()->isLeftToRightDirection() != isLeftToRightDirection())

I know I should be doing !offset but that looks odd in this context.

[WebKit Review Bot]

Attachment 76468 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/editing/selection/caret-bidi-first-and-last-letters-expected.txt', u'LayoutTests/editing/selection/caret-bidi-first-and-last-letters.html', u'WebCore/ChangeLog', u'WebCore/rendering/InlineTextBox.cpp']" exit_code: 1
WebCore/rendering/InlineTextBox.cpp:1116:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

(In reply to comment #5)
> Never mind. I wasn’t testing the right build. However, it now appears that in r73922, clicking inside a letter behaves correctly, but clicking on either side of the text behaves incorrectly.

By "behaves incorrectly", what do you exactly mean?  As far as I checked, when I click on the left of text, the caret is placed on the left, and when I click on the right of text, it's placed on the right as expected regardless of direction of text or the block.

Or do you expect a different behavior?

(In reply to comment #11)
> (In reply to comment #5)
> > Never mind. I wasn’t testing the right build. However, it now appears that in r73922, clicking inside a letter behaves correctly, but clicking on either side of the text behaves incorrectly.
> 
> By "behaves incorrectly", what do you exactly mean?  As far as I checked, when I click on the left of text, the caret is placed on the left, and when I click on the right of text, it's placed on the right as expected regardless of direction of text or the block.
> 
> Or do you expect a different behavior?

No, that is the expected behavior.

[Ryosuke Niwa]

(In reply to comment #12)
> > Or do you expect a different behavior?
> 
> No, that is the expected behavior.

Ok. Then I think http://trac.webkit.org/changeset/73548 was an improvement but I didn't take into account the case where Font::positionForPoint returns 0 or m_len, and this patch addresses those two cases.

[Ryosuke Niwa]

Created attachment 76475 [details]
Modified caret-ltr* caret-rtl* per Xiaomei's request

[Xiaomei Ji]

The Layout test result is confusing.
Given caret-ltr-2.html as example.

If I open the page in browser,
When I click the left empty space of "WebKit2", the caret is placed at the left of 'W', which is correct.
But the test printed out FAIL: wrong offset 7, expected 0".

If I click the left half of 'W', the caret is places at very right,  which is wrong.
But the test printed out "PASS".

The test printed out correct information if I click the right empty space of "WebKit2" or the right half of '2'.

The test also prints out incorrect message when clicking inside the text.

Maybe the test need to be changed to at least not printing wrong message.
 
Thanks for changing the test.

[WebKit Review Bot]

Attachment 76475 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/editing/selection/caret-bidi-first-and-last-letters-expected.txt', u'LayoutTests/editing/selection/caret-bidi-first-and-last-letters.html', u'LayoutTests/editing/selection/resources/caret-edge-shared.js', u'WebCore/ChangeLog', u'WebCore/rendering/InlineTextBox.cpp']" exit_code: 1
WebCore/rendering/InlineTextBox.cpp:1116:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 76476 [details]
Refactored some code

[Xiaomei Ji]

Created attachment 76558 [details]
edge cases

seems not working for some edge cases.

[Ryosuke Niwa]

(In reply to comment #18)
> Created an attachment (id=76558) [details]
> edge cases
> 
> seems not working for some edge cases.

Yeah, we need to overhaul most of positionForPoint code for this case to work properly.  However, neither of my changes regress for this edge case.  So as you said, we can probably improve the behavior incrementally.

[Ryosuke Niwa]

Comment on attachment 76476 [details]
Refactored some code

Clearing flags on attachment: 76476

Committed r74971: <http://trac.webkit.org/changeset/74971>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Thanks for the review, Dan.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74971 might have broken Qt Linux Release
The following tests are not passing:
editing/selection/caret-bidi-first-and-last-letters.html