Bug 72048

Summary: --webkit-visual-word should be able to reach end of text, instead of end of line
Product: WebKit Reporter: Xiaomei Ji <xji>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: progame+wk, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 25298    
Attachments:
Description Flags
patch w/ layout test
none
patch w/ layout test rniwa: review+

Description Xiaomei Ji 2011-11-10 13:02:22 PST
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).
Comment 1 Yair Yogev 2011-11-10 14:05:37 PST
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.
Comment 2 Xiaomei Ji 2011-11-10 14:49:23 PST
Created attachment 114580 [details]
patch w/ layout test
Comment 3 Ryosuke Niwa 2011-11-25 18:06:43 PST
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):
> +
> +2011-11-10  Xiaomei Ji  <xji@chromium.org>
> +

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?
Comment 4 Xiaomei Ji 2011-11-29 13:51:21 PST
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.
Comment 5 Xiaomei Ji 2011-11-29 13:52:31 PST
Created attachment 117040 [details]
patch w/ layout test
Comment 6 Ryosuke Niwa 2011-11-29 13:58:59 PST
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.
Comment 7 Ryosuke Niwa 2011-11-29 13:59:41 PST
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.
Comment 8 Xiaomei Ji 2011-11-29 16:29:40 PST
Committed r101430: <http://trac.webkit.org/changeset/101430>