Bug 72048 – --webkit-visual-word should be able to reach end of text, instead of end of line

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Xiaomei Ji]

Turns out that I mis-understood Yair on issue 61346.

He meant ctrl(alt)-arrow should reach end of txt, not end of line.

Following is Yair's summary on how different browsers handle wrapped and non-wrapped end-of-line for textarea and div.


Opera:
auto-wrapped:
stops at the end?: Y
beginning of the next line?: N
not auto-wrapped: 
stops at the end?: Y
beginning of the next line?: Y

FF:
auto-wrapped: 
stops at the end?: N
beginning of the next line?: Y
not auto-wrapped: 
stops at the end?: N
beginning of the next line?: Y

IE:
auto-wrapped:
stops at the end?: N 
beginning of the next line?: Y
not auto-wrapped:
stops at the end?: Y
beginning of the next line?: Y

 "visual-word-movement" enabled chrome:
auto-wrapped:
textarea
stops at the end?: N
beginning of the next line?: Y
gmail richtext
stops at the end?: N
beginning of the next line?: Y
not auto-wrapped:
textarea
stops at the end?: N
beginning of the next line?: Y
gmail richtext
stops at the end?: Y
beginning of the next line?: Y


We will try to do the following (for both textarea and div).
ctrl(alt)-arrow only stops at left start of word, it wont stop at line end for wrapped line or line-break, and it wont stop at an empty line (a line with only line break). When there is no more words in this editable content, it stops at content end. (I do not know how to handle non-editable content area).

[Yair Yogev]

Some notes for the sake of documentation:
The above mentioned comparison was made under Windows only.
Also, we learned that IE\FF\Opera stop at empty lines with the exception of FF in divs (FF does stop for textareas).
However,
FF in divs does stop at the beginning of lines which only contain spaces.

[Xiaomei Ji]

Created attachment 114580 [details]
patch w/ layout test

[Ryosuke Niwa]

Comment on attachment 114580 [details]
patch w/ layout test

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

> Source/WebCore/ChangeLog:130
> +        Need a short description and bug URL (OOPS!)
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * editing/FrameSelection.cpp:
> +        (WebCore::FrameSelection::modifyMovingRight):
> +        * editing/visible_units.cpp:
> +        (WebCore::collectWordBreaksInBoxInsideBlockWithSameDirectionality):
> +        (WebCore::collectWordBreaksInBoxInsideBlockWithDifferntDirectionality):
> +        (WebCore::leftWordPosition):
> +        (WebCore::rightWordPosition):
> +
> [phone]  Xiaomei Ji  <[e-mail]>
> +

You should remove this entry.

> LayoutTests/editing/selection/move-by-word-visually-multi-line-expected.txt:42
> -"abc def ghi jkl mn "[0, 3, 8, 11, 16, 18], "opq rst uvw xyz"[0, 3, 8, 11, 15]
> +"abc def ghi jkl mn "[0, 3, 8, 11, 16, 18], "opq rst uvw xyz"[0, 3, 8, 11, 15]    FAIL expected: ["abc def ghi jkl mn "[ 0,  3,  8,  11,  16, ]"opq rst uvw xyz"[ 0,  3,  8,  11,  15]
> +"abc def ghi jkl mn "[16, 18]   FAIL expected "opq rst uvw xyz"[ 0]
> +"abc def ghi jkl mn "[17, 18]   FAIL expected "opq rst uvw xyz"[ 0]

Why are these cases failing?

[Xiaomei Ji]

Comment on attachment 114580 [details]
patch w/ layout test

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

>> Source/WebCore/ChangeLog:130
>> +
> 
> You should remove this entry.

done

>> LayoutTests/editing/selection/move-by-word-visually-multi-line-expected.txt:42
>> +"abc def ghi jkl mn "[17, 18]   FAIL expected "opq rst uvw xyz"[ 0]
> 
> Why are these cases failing?

it is <div dir=rtl>abc def ghi jkl mn <div><br/></div><div><br/></div><div><br/></div>opq rst uvw xyz</div>

Let's say it is visually display as:
abc def
ghi jkl
     mn


opq rst
....


Ideally, when cursor is at mn| and press ctrl(alt)-left-arrow, cursor should move to rst|.
But in current algorithm, it founds a word break at |mn and returned it.

It is  not a regression introduced in this patch. The expectation changed (if you look at id="ml_8" in move-by-word-visually-multi-line.html) based on that we'd want word break stops at end of text only, not end of line.

It is not ideal, but I think it is not that a big deal either.

[Xiaomei Ji]

Created attachment 117040 [details]
patch w/ layout test

[Ryosuke Niwa]

Comment on attachment 117040 [details]
patch w/ layout test

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

> Source/WebCore/editing/visible_units.cpp:1615
> +    // FIXME: how to handle non-editable position?

"How should we handle a non-editable position"

> Source/WebCore/editing/visible_units.cpp:1616
> +    if (leftWordBreak.isNull() && isEditablePosition(visiblePosition.deepEquivalent())) {

Please use visiblePosition.rootEditableElement() instead.

> Source/WebCore/editing/visible_units.cpp:1631
> +    // FIXME: how to handle non-editable position?

Ditto.

> Source/WebCore/editing/visible_units.cpp:1632
> +    if (rightWordBreak.isNull() && isEditablePosition(visiblePosition.deepEquivalent())) {

Ditto.

[Ryosuke Niwa]

Comment on attachment 117040 [details]
patch w/ layout test

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

>> Source/WebCore/editing/visible_units.cpp:1615
>> +    // FIXME: how to handle non-editable position?
> 
> "How should we handle a non-editable position"

"How should we handle a non-editable position"

>> Source/WebCore/editing/visible_units.cpp:1616
>> +    if (leftWordBreak.isNull() && isEditablePosition(visiblePosition.deepEquivalent())) {
> 
> Please use visiblePosition.rootEditableElement() instead.

Please use visiblePosition.rootEditableElement() instead.

>> Source/WebCore/editing/visible_units.cpp:1631
>> +    // FIXME: how to handle non-editable position?
> 
> Ditto.

Ditto.

>> Source/WebCore/editing/visible_units.cpp:1632
>> +    if (rightWordBreak.isNull() && isEditablePosition(visiblePosition.deepEquivalent())) {
> 
> Ditto.

Ditto.

> LayoutTests/editing/selection/move-by-word-visually-textarea-expected.txt:1
> +PASS Pass

One more thing, it's odd to have "PASS pass".

You should also a description as to what you're testing here.

[Xiaomei Ji]

Committed r101430: <http://trac.webkit.org/changeset/101430>