Bug 58504 – move caret by word in visual order returns wrong result when caret itself is at word boundary

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

[Xiaomei Ji]

positionBeforeNext() does not work for all cases.

Given "abc def hij" as an example:
if caret is between "b' and 'c', it returns position between ' ' and 'd'.
But if caret is between 'c' and ' ', it returns position between ' ' and 'h', which is wrong.

Same problem for positionAfterPreviousWord().

[Xiaomei Ji]

Created attachment 89889 [details]
patch w/ layout test

[Xiaomei Ji]

Created attachment 90437 [details]
patch w/ layout test

[Ryosuke Niwa]

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

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

> LayoutTests/editing/selection/move-by-word-visually-expected.txt:178
> +"FFZ LIG"[3, 3]   FAIL expected "ABD DSU "[ 0]

Why do we have different strings in expected result?  That seems wrong.

[Xiaomei Ji]

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

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

> LayoutTests/editing/selection/move-by-word-visually-expected.txt:179
> +"FFZ LIG"[4, 4]   FAIL expected "ABD DSU "[ 0]

the current behavior is moving from offset 3 in node "FFZ LIG" moves cursor to offset 3 (cursor stays the same position).
It actually should move cursor to offset 0 in node "ABD DSU".

When the structure is complicated (with embedding elements and bidirectional text) the cursor start and end node might be different.

[Ryosuke Niwa]

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

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

> LayoutTests/editing/selection/move-by-word-visually.html:-374
> -<!-- The content in title means "startOffsetInCurrentNode movingDirectionByWord endingNode endingOffsetInEndingNode" -->
> -<div id="div_100" contenteditable>××× <span id="span_100" class="specific_test" title="1 left div_100 0">××</span></div>

Why are we removing this test?

[Xiaomei Ji]

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

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

>> LayoutTests/editing/selection/move-by-word-visually.html:-374
>> -<div id="div_100" contenteditable>××× <span id="span_100" class="specific_test" title="1 left div_100 0">××</span></div>
> 
> Why are we removing this test?

It was added to address comment #18 in bug 57806 https://bugs.webkit.org/show_bug.cgi?id=57806#c18.
It is a very specific test just to test a specific path works (at that time, the whole patch is not completed implemented yet).

Now since we have the whole patch ready, and I added test case to test word break stops at right position on *every* visible position. So this specific test is covered, and we are no longer need it anymore.

[Ryosuke Niwa]

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

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

>>> LayoutTests/editing/selection/move-by-word-visually.html:-374
>>> -<div id="div_100" contenteditable>××× <span id="span_100" class="specific_test" title="1 left div_100 0">××</span></div>
>> 
>> Why are we removing this test?
> 
> It was added to address comment #18 in bug 57806 https://bugs.webkit.org/show_bug.cgi?id=57806#c18.
> It is a very specific test just to test a specific path works (at that time, the whole patch is not completed implemented yet).
> 
> Now since we have the whole patch ready, and I added test case to test word break stops at right position on *every* visible position. So this specific test is covered, and we are no longer need it anymore.

Okay, thanks for the clarification.

[Xiaomei Ji]

Committed r84710: <http://trac.webkit.org/changeset/84710>

[WebKit Review Bot]

http://trac.webkit.org/changeset/84710 might have broken Qt Linux Release
The following tests are not passing:
editing/selection/move-by-word-visually.html

[Xiaomei Ji]

(In reply to comment #9)
> http://trac.webkit.org/changeset/84710 might have broken Qt Linux Release
> The following tests are not passing:
> editing/selection/move-by-word-visually.html

hm.. the result in QT is weird. Maybe visible position in QT is different, which  is not likely.
Or maybe the nextWordPosition and previousWordPosition in QT has bug.

temporarily put the test in QT's skip list while investigating.

Filed bug:
https://bugs.webkit.org/show_bug.cgi?id=59265