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
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: PC Windows 7
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 25298
  Show dependency treegraph
 
Reported: 2011-04-13 18:51 PDT by Xiaomei Ji
Modified: 2011-04-22 18:13 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaomei Ji 2011-04-13 18:51:30 PDT
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 Xiaomei Ji 2011-04-15 17:15:53 PDT
Created attachment 89889 [details]
patch w/ layout test
Comment 2 Xiaomei Ji 2011-04-20 16:20:28 PDT
Created attachment 90437 [details]
patch w/ layout test
Comment 3 Ryosuke Niwa 2011-04-20 17:24:01 PDT
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.
Comment 4 Xiaomei Ji 2011-04-20 18:10:56 PDT
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.
Comment 5 Ryosuke Niwa 2011-04-22 13:29:42 PDT
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?
Comment 6 Xiaomei Ji 2011-04-22 14:19:31 PDT
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.
Comment 7 Ryosuke Niwa 2011-04-22 14:31:41 PDT
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.
Comment 8 Xiaomei Ji 2011-04-22 17:12:41 PDT
Committed r84710: <http://trac.webkit.org/changeset/84710>
Comment 9 WebKit Review Bot 2011-04-22 17:57:01 PDT
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 Xiaomei Ji 2011-04-22 18:13:56 PDT
(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