Bug 58504 - move caret by word in visual order returns wrong result when caret itself is at word boundary
: move caret by word in visual order returns wrong result when caret itself is ...
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: PC Windows 7
: P2 Normal
Assigned To:
:
:
:
: 25298
  Show dependency treegraph
 
Reported: 2011-04-13 18:51 PST by
Modified: 2011-04-22 18:13 PST (History)


Attachments
patch w/ layout test (23.68 KB, patch)
2011-04-15 17:15 PST, Xiaomei Ji
no flags Review Patch | Details | Formatted Diff | Diff
patch w/ layout test (23.60 KB, patch)
2011-04-20 16:20 PST, Xiaomei Ji
rniwa: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-04-13 18:51:30 PST
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().
------- Comment #1 From 2011-04-15 17:15:53 PST -------
Created an attachment (id=89889) [details]
patch w/ layout test
------- Comment #2 From 2011-04-20 16:20:28 PST -------
Created an attachment (id=90437) [details]
patch w/ layout test
------- Comment #3 From 2011-04-20 17:24:01 PST -------
(From update of attachment 90437 [details])
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.
------- Comment #4 From 2011-04-20 18:10:56 PST -------
(From update of attachment 90437 [details])
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.
------- Comment #5 From 2011-04-22 13:29:42 PST -------
(From update of attachment 90437 [details])
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?
------- Comment #6 From 2011-04-22 14:19:31 PST -------
(From update of attachment 90437 [details])
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.
------- Comment #7 From 2011-04-22 14:31:41 PST -------
(From update of attachment 90437 [details])
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.
------- Comment #8 From 2011-04-22 17:12:41 PST -------
Committed r84710: <http://trac.webkit.org/changeset/84710>
------- Comment #9 From 2011-04-22 17:57:01 PST -------
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
------- Comment #10 From 2011-04-22 18:13:56 PST -------
(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