Bug 105451 – Unable to place caret in RTL Override Characters by mouse click

• Component: HTML Editing
• Status: NEW

[Yi Shen]

This issue is originally mentioned in Bug 103072 - in a multiple lines textarea, the first line has two parts: a LTR text followed by some RTL characters. However, user is unable to place caret in the second part of the line using the mouse - that part of the line seems not "clickable". Since the issues described in Bug 103072 have different causes, I created this bug which just focus on addressing this "clickable" problem.

[Yi Shen]

Created attachment 180204 [details]
first try

[Radar WebKit Bug Importer]

<rdar://problem/12913146>

[Ryosuke Niwa]

Comment on attachment 180204 [details]
first try

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

> Source/WebCore/rendering/RenderText.cpp:513
> -    if (!box->nextLeafChildIgnoringLineBreak()) {
> +    if (!box->nextLeafChildIgnoringLineBreak(false)) {

Just call nextLeafChild() instead. r- due to unnecessary changes to nextLeafChildIgnoringLineBreak and prevLeafChildIgnoringLineBreak.
Did you make sure selection made by mouse drag on http://trac.webkit.org/browser/trunk/LayoutTests/editing/selection/select-line-break-with-opposite-directionality.html
makes sense for all cases? (in addition to ones that are already tested).

[Yi Shen]

Thanks for reviewing :]

The reason why I decide not to use nextLeafChild here is because I don't want to break the original code logic.  For example, let's say we have two line boxes, abc'\n'. If we use nextLeafChild to check whether current box is the last in the line, the line break box will get a chance to be checked by lineDirectionPointFitsInBox, right? If we use nextLeafChildIgnoringLineBreak, this won't happen.

In addition, I think the names of the prev/nextLeafChildIgnoringLineBreak are kind of confusing. It surprised me that they return 0 if there is a line break, instead of the leaf child before or after the line break.

The test select-line-break-with-opposite-directionality.html makes sense for me. In addition we can add a new test case as below (let me know if you want me to add it)

<li><div contenteditable contenteditable title="0 3">ab&#8238;c<br></div></li>

(In reply to comment #3)
> (From update of attachment 180204 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180204&action=review
> 
> > Source/WebCore/rendering/RenderText.cpp:513
> > -    if (!box->nextLeafChildIgnoringLineBreak()) {
> > +    if (!box->nextLeafChildIgnoringLineBreak(false)) {
> 
> Just call nextLeafChild() instead. r- due to unnecessary changes to nextLeafChildIgnoringLineBreak and prevLeafChildIgnoringLineBreak.
> Did you make sure selection made by mouse drag on http://trac.webkit.org/browser/trunk/LayoutTests/editing/selection/select-line-break-with-opposite-directionality.html
> makes sense for all cases? (in addition to ones that are already tested).

[Ryosuke Niwa]

Comment on attachment 180204 [details]
first try

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

> Source/WebCore/ChangeLog:8
> +        For RTL override characters, its line break box may not be the last box on line. This patch modified the

Oh, I think I misunderstood the bug. It appears to me that we have a bug in line layout code then. I don't think a line box for a line break should appear in the middle of a line.

[Ryosuke Niwa]

Or are you saying that RTL override character creates a line box with isLineBreak() being true?

[Yi Shen]

yes, in current code, a line box for '\n' might appear in the middle of a line. I am not sure it is a bug or not.

(In reply to comment #5)
> (From update of attachment 180204 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180204&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        For RTL override characters, its line break box may not be the last box on line. This patch modified the
> 
> Oh, I think I misunderstood the bug. It appears to me that we have a bug in line layout code then. I don't think a line box for a line break should appear in the middle of a line.

[Yi Shen]

Created attachment 182230 [details]
reset line break bidi level to avoid it be shifted to the middle

[Ryosuke Niwa]

Comment on attachment 182230 [details]
reset line break bidi level to avoid it be shifted to the middle

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

The description makes sense but I need to re-familiarize myself with bidi resolver before I can review this patch.

> Source/WebCore/platform/text/BidiResolver.h:509
> +        if (run && (run->stop() - run->start() == 1)) {

Why do we do this only when the difference between the start and the stop is 1?

> Source/WebCore/platform/text/BidiResolver.h:510
> +            BidiContext* c = context();

Please don't use one letter variable.

> Source/WebCore/platform/text/BidiResolver.h:511
> +            while (c->parent())

Is this correct with respect to the way we implement unicode-bidi: -webkit-isolate and unicode-bidi: -webkit-plaintext?
I definitely would like to see tests for these.

> LayoutTests/editing/selection/click-on-rtl-text.html:10
> +<div id="description">This test that clicking on the editable RTL text correctly places the caret between the RTL characters.</div>
> +<textarea id="editor" cols="100" rows="3">a&#8238;AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA

Could you create a LTR equivalent?

[Yi Shen]

Thanks for the comments.
(In reply to comment #9)
> (From update of attachment 182230 [details])
> > Source/WebCore/platform/text/BidiResolver.h:509
> > +        if (run && (run->stop() - run->start() == 1)) {
> 
> Why do we do this only when the difference between the start and the stop is 1?
According to http://unicode.org/reports/tr9/#L1, we need to reset the level for 1) Segment separators, 2) Paragraph separators, 3) Any sequence of whitespace characters preceding a segment separator or paragraph separator, and 4)Any sequence of white space characters at the end of the line.

It seems the BidiResolver won't create runs for 3) and 4), so I added a condition here to say we only reset the level for the run which only has one character. I will double check this.
> 
> > Source/WebCore/platform/text/BidiResolver.h:510
> > +            BidiContext* c = context();
> 
> Please don't use one letter variable.
Will fix it.
> 
> > Source/WebCore/platform/text/BidiResolver.h:511
> > +            while (c->parent())
> 
> Is this correct with respect to the way we implement unicode-bidi: -webkit-isolate and unicode-bidi: -webkit-plaintext?
> I definitely would like to see tests for these.
Will check this.
> 
> > LayoutTests/editing/selection/click-on-rtl-text.html:10
> > +<div id="description">This test that clicking on the editable RTL text correctly places the caret between the RTL characters.</div>
> > +<textarea id="editor" cols="100" rows="3">a&#8238;AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
> 
> Could you create a LTR equivalent?
Will add it.

[Yi Shen]

Created attachment 197354 [details]
updated with rniwa's comment

[Ryosuke Niwa]

Comment on attachment 197354 [details]
updated with rniwa's comment

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

> LayoutTests/editing/selection/click-on-rtl-text-in-textarea-expected.txt:8
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS

This output isn't helpful. It doesn't tell us what kind of test cases we have and why we're passing.

> LayoutTests/editing/selection/click-on-rtl-text-in-textarea.html:44
> +    clickOnText(editor.offsetLeft + 1, editor.offsetTop + 5);
> +    if (editor.selectionStart != 0)
> +        result = "FAIL";

You can replace this code by shouldBe("clickOnText(editor.offsetLeft + 1, editor.offsetTop + 5); editor.selectionStart", "0");
after including js-test-pre.js. This way, we can see what we're testing and what we're checking.
It would better if we included element id names so that we can distinguish each test case.

> LayoutTests/editing/selection/click-on-rtl-text-in-textarea.html:48
> +    clickOnText(editor.offsetLeft + 30, editor.offsetTop + 5);
> +    if (editor.selectionStart <= 1)
> +        result = "FAIL";

Ditto.

> Source/WebCore/platform/text/BidiResolver.h:149
> +    bool onlyHasNewlineCharacter() const { return false; }

Why is it okay to always return false here?

> Source/WebCore/platform/text/BidiResolver.h:508
> +    // According to the Bidi spec L1, reset the embedding level of the
> +    // line break character to the paragraph embedding level.

Why do we need to do that? We need "why" comments, not "what" comments.

> Source/WebCore/rendering/BidiRun.cpp:78
> +bool BidiRun::onlyHasNewlineCharacter() const
> +{
> +    return (m_stop - m_start == 1) && m_object->isText() && toRenderText(m_object)->characterAt(m_start) == '\n';
> +}

This should definitely be an inline function.

[Yi Shen]

Thanks for reviewing. I will fix all you mentioned above.

> > +    bool onlyHasNewlineCharacter() const { return false; }

The only reason I make BidiCharacterRun::onlyHasNewlineCharacter to return false is because a BidiCharacterRun doesn't have enough knowledge to know whether its content contains '\n'. 

Yet BidiCharacterRun never been used in the BidiResolver::recorderRunsFromlevels in our code, it doesn't impact the reordering procedure. Maybe I should rename that function to avoid the confusion. What do you think?

[Yi Shen]

Created attachment 197684 [details]
updated the test and comments

[Ryosuke Niwa]

Comment on attachment 197684 [details]
updated the test and comments

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

> Source/WebCore/platform/text/BidiResolver.h:511
> +        if (run && run->onlyHasNewlineCharacter()) {

According to L1, we need to reset the embedding level of "[a]ny sequence of white space characters at the end of the line" as well.
Shouldn't we checking that condition instead? Or do we create a run consisting of exactly LF in that case?

[Yi Shen]

Yes, WebKit does create a run consisting of exactly LF in that case. 

Regarding the ending white space characters, I totally agree that we should check this condition also. But before that, we have to rework the code to generate a run that only consists the ending white space in that case -- currently, the run that contains the ending white space also contain other characters, while we don't want to reset the embedding level for the non-space characters in that run. I would like to create another bug to address that issue :)
(In reply to comment #15)
> (From update of attachment 197684 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197684&action=review
> 
> > Source/WebCore/platform/text/BidiResolver.h:511
> > +        if (run && run->onlyHasNewlineCharacter()) {
> 
> According to L1, we need to reset the embedding level of "[a]ny sequence of white space characters at the end of the line" as well.
> Shouldn't we checking that condition instead? Or do we create a run consisting of exactly LF in that case?

[Andreas Kling]

Comment on attachment 197684 [details]
updated the test and comments

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.