Bug 57336 – experiment with moving caret by word in visual order

• Component: HTML Editing
• Status: RESOLVED FIXED

[Xiaomei Ji]

Implementing move caret by word in visual order  will; be done incrementally.
This will be the 1st version. And It only works for the following condition.
1. For a LTR box in a LTR block or a RTL box in RTL block, when caret is at the left boundary of the box and we are looking for the word boundary in right.
2. For a LTR or RTL box in a LTR block, when caret is at the left boundary of the box and we are looking for the word boundary in left and previous box is a LTR box.
3. For a LTR or RTL box in a RTL block, when the caret is at the right boundary of the box and we are looking for the word boundary in right and next box is RTL box.

[Xiaomei Ji]

Created attachment 87322 [details]
patch w/ layout test

[Ryosuke Niwa]

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

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

Great! This patch is much more reviewable.

> Source/WebCore/editing/TextGranularity.h:44
> +    // caret by wordGranularity. After all patches in, it should be removed.

Nit: Once all patches are landed

> Source/WebCore/editing/visible_units.cpp:1237
> +    bool first = previousWordBreak.isNull(); // Whether this is to collect the first word break in the box.

Instead of adding a comment here, let us instead have:
bool hasSeenWordBreakInThisBox = previousWordBreak.isNotNull();

> Source/WebCore/editing/visible_units.cpp:1238
> +    InlineBox* boxOfWordBreak;

Nit: should this be named as boxContainingPreviousWordBreak?

> Source/WebCore/editing/visible_units.cpp:1240
> +    if (box->direction() == blockDirection) {

This function should probably named as nextWordBreakInBox or previousWordBreakInBoxInVisualOrder.

It also seems like we should split this function into two versions the one for box->direction() == blockDirection and (box->direction() != blockDirection.
They can be named as:
previousWordBreakInBoxInsideBlockWithSameDirectionality
nextWordBreakInBoxInsideBlockWithOppositeDirectionality

> Source/WebCore/editing/visible_units.cpp:1243
> +        wordBreak = first ? VisiblePosition(Position(box->renderer()->node(), box->caretMaxOffset(), Position::PositionIsOffsetInAnchor), VP_DEFAULT_AFFINITY) : previousWordBreak;

You don't need to explicitly call VisiblePosition's constructor.  it'll automatically do it.  So you can do:
wordBreak = first ? Position(box->renderer()->node(), box->caretMaxOffset(), Position::PositionIsOffsetInAnchor) : previousWordBreak;

> Source/WebCore/editing/visible_units.cpp:1245
> +        if (first && box->nextLeafChild()) {

It seems that this condition should be dependent on the value of directionality.  It doesn't make sense that we check the same box for both LTR and RTL cases.

> Source/WebCore/editing/visible_units.cpp:1248
> +            // when cursor is at the right of "opq", its previous word boundary's offset returned from previousWordPosition() is 15, which is not in the same box as "opq",

Nit: immediately to the right of "opq".
Nit: offset from previousWordPosition() is immediately to the right of "hij ".

> Source/WebCore/editing/visible_units.cpp:1262
> +        last = true;
> +        return VisiblePosition();

For this version at least, it seems simpler to get rid of "last" argument and use the null visible position to mean the last.

> Source/WebCore/editing/visible_units.cpp:1264
> +    // FIXME in next version.

FIXME implies "next version".  We should explain what need to be implemented.  I guess:
FIXME: Implement the case when the box direction is not equal to the block direction
would work here.

> Source/WebCore/editing/visible_units.cpp:1277
> +// Get the leftmost or rightmost word bounary in a box.
> +// 'box' is the box to start search.
> +// 'offset' is the node offset of the current caret.
> +// When 'offset' is valid (not equal to -1), the word boundary returned should be different from the word boundary starts at 'offset'.
> +// 'boundaryDirection' indicates the visual direction (left or right) to search for
> +// word boundary.
> +// 'boundaryDirection' == DirectionRight means search rightmost word boundary in box.
> +// 'boundaryDirection' == DirectionLeft means search leftmost word boundary in box.
> +// 'blockDirection' indicates the directionality of the current block.

No need for these comments.

> Source/WebCore/editing/visible_units.cpp:1291
> +        VisiblePosition wordBreak = collectOneWordBreakInBox(box, blockDirection, VisiblePosition(), offsetOfWordBreak, last);
> +        while (!last) {
> +            if (offset == -1 || offsetOfWordBreak != offset)
> +                return wordBreak;
> +            wordBreak = collectOneWordBreakInBox(box, blockDirection, wordBreak, offsetOfWordBreak, last);
> +        }

This can be written as a do-while as in:
VisiblePosition wordBreak;
do {
    wordBreak = collectOneWordBreakInBox(box, blockDirection, wordBreak, offsetOfWordBreak, last);
    if (offset == -1 || offsetOfWordBreak != offset)
        return wordBreak;
} while (!last);

> Source/WebCore/editing/visible_units.cpp:1293
> +        if (!wordBreak.isNull() && (offset == -1 || offsetOfWordBreak != offset))
> +            return wordBreak;

This is dead code for this version.  Please remove.

> Source/WebCore/editing/visible_units.cpp:1299
> +// Search word boundary in visually adjacent (previous or next) boxes.

This repeats the function name. Please remove it.

> Source/WebCore/editing/visible_units.cpp:1301
> +// 'box' is the box to start search.
> +// 'offset' is the node offset of the current caret.

No need. Please get rid of it.

> Source/WebCore/editing/visible_units.cpp:1302
> +// When 'offset' is valid (not equal to -1), the word boundary returned should be different from the word boundary starts at 'offset'.

We should define a value as in #define INVALID_OFFSET -1 so that we don't need to add this comment.

> Source/WebCore/editing/visible_units.cpp:1308
> +// 'boundaryDirection' indicates the visual direction (left or right) to search for
> +// word boundary.
> +// 'boundaryDirection' == DirectionRight means search rightmostWordBoundaryInPreviousBoxes.
> +// 'boundaryDirection' == DirectionLeft means search leftmostWordBoundaryInNextBoxes.
> +// 'blockDirection' indicates the directionality of the current block.
> +// 'visiblePosition' is the visible position of current caret.

I don't think these comments are necessary.

> Source/WebCore/editing/visible_units.cpp:1309
> +static VisiblePosition wordBoundaryInAdjacentBoxes(const InlineBox* box, int offset, WebCore::SelectionDirection boundaryDirection, TextDirection blockDirection, const VisiblePosition& visiblePosition)

boundaryDirection might be a bit confusing?  Maybe directionToSearch?

> Source/WebCore/editing/visible_units.cpp:1322
> +static VisiblePosition rightmostWordBoundaryInPreviousBoxes(const InlineBox* box, int offset, TextDirection blockDirection, const VisiblePosition& visiblePosition)

This function name is confusing because I get an impression that we start looking for a word boundary from box->prevLeafChild().
maybe leftWordBoundary or rightmostWordBoundaryInBoxOrPreviousBoxes?

> Source/WebCore/editing/visible_units.cpp:1348
> +    // FIXME: If the box's directionality is the same as that of the enclosing block, when the offset is at the box boundary and the direction is towards inside the box,
> +    // do I still need to make it a special case? For example, a LTR box inside a LTR block, when offset is at box's caretMinOffset and the direction is DirectionRight,
> +    // should it be taken care as a general case?

These lines look too long.  Please make each line a little shorter (20-30%).

> Source/WebCore/editing/visible_units.cpp:1352
> +        if (wordDirection == WebCore::DirectionLeft)
> +            return rightmostWordBoundaryInPreviousBoxes(isLTRBox ? box->prevLeafChild() : box, isLTRBox ? -1 : offset, blockDirection, visiblePosition);
> +        return leftmostWordBoundaryInNextBoxes(isLTRBox ? box : box->nextLeafChild(), isLTRBox ? offset : -1, blockDirection, visiblePosition);

Maybe it's better to have these logic directly in leftWordPosition and rightWordPosition.

e.g.
VisiblePosition leftWordPosition(const VisiblePosition& visiblePosition) {
    InlineBox* box;
    int offset;
    visiblePosition.getInlineBoxAndOffset(box, offset);
    TextDirection blockDirection = directionOfEnclosingBlock(visiblePosition.deepEquivalent());

    if (offset == box->leftmostCaretOffset())
        return rightmostWordBoundaryInPreviousBoxes(box->prevLeafChild(), -1, blockDirection, visiblePosition);
    else if (offset == box->rightmostCaretoffset())
        return rightmostWordBoundaryInPreviousBoxes(box, offset, blockDirection, visiblePosition);

     // FIXME: Not implemented.
     return VisiblePosition();
}

> LayoutTests/editing/selection/move-by-word-visually-expected.txt:1
> +CONSOLE MESSAGE: line 180: TypeError: 'null' is not an object (evaluating 'document.getElementById("testGroup").style')

Do we expect to see this error?

> LayoutTests/editing/selection/move-by-word-visually-expected.txt:37
> +"abc def hij opq rst"[0, 0]
> +"abc def hij opq rst"[1, 1]
> +"abc def hij opq rst"[2, 2]
> +"abc def hij opq rst"[3, 3]
> +"abc def hij opq rst"[4, 4]
> +"abc def hij opq rst"[5, 5]
> +"abc def hij opq rst"[6, 6]
> +"abc def hij opq rst"[7, 7]
> +"abc def hij opq rst"[8, 8]
> +"abc def hij opq rst"[9, 9]
> +"abc def hij opq rst"[10, 10]
> +"abc def hij opq rst"[11, 11]
> +"abc def hij opq rst"[12, 12]
> +"abc def hij opq rst"[13, 13]
> +"abc def hij opq rst"[14, 14]
> +"abc def hij opq rst"[15, 15]
> +"abc def hij opq rst"[16, 16]
> +"abc def hij opq rst"[17, 17]
> +"abc def hij opq rst"[18, 18]
> +"abc def hij opq rst"[19, 19]

How do we know these results are correct?  it seems like we want to print either PASS or FAIL.  Otherwise, other contributors will have no idea whether this test is passing or failing.

> LayoutTests/editing/selection/move-by-word-visually.html:1
> +<head>

Missing DOCTYPE

> LayoutTests/editing/selection/move-by-word-visually.html:3
> +    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
> +    <style>

Let's not indent these elements.  Outdenting everything will make code simpler.

> LayoutTests/editing/selection/move-by-word-visually.html:163
> +            while (true) {
> +                sel.setPosition(prevNode, prevOffset);
> +                var positions = [];
> +                positions.push({ node: sel.anchorNode, offset: sel.anchorOffset, point: caretCoordinates() });
> +                sel.modify("move", direction, "-webkit-visual-word");
> +                positions.push({ node: sel.anchorNode, offset: sel.anchorOffset, point: caretCoordinates() });
> +                if (direction == "right")
> +                    checkCoordinatesMovingRightDown(positions);
> +                else
> +                    checkCoordinatesMovingLeftUp(positions);
> +                logPositions(positions);
> +                log("\n");
> +                sel.setPosition(prevNode, prevOffset);
> +                sel.modify("move", direction, "character");
> +                if (sel.anchorNode == prevNode && sel.anchorOffset == prevOffset) {
> +                    break;
> +                }
> +                prevNode = sel.anchorNode;
> +                prevOffset = sel.anchorOffset;
> +            }

This function seems to be a duplicate of the code above.  We should make a function and call it twice instead of duplicating the code.

[Xiaomei Ji]

Created attachment 87522 [details]
patch w/ layout test

[WebKit Review Bot]

Attachment 87522 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebKit/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/editing/visible_units.cpp:1273:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/editing/visible_units.cpp:1289:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

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

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

> Source/WebCore/editing/visible_units.cpp:1245
> +    if (!hasSeenWordBreakInThisBox && ((box->isLeftToRightDirection() && box->nextLeafChild()) || (!box->isLeftToRightDirection() && box->prevLeafChild()))) {

The condition ((box->isLeftToRightDirection() && box->nextLeafChild()) || (!box->isLeftToRightDirection() && box->prevLeafChild())) should be either a boolean variable or extracted as an inline function.

> Source/WebCore/editing/visible_units.cpp:1254
> +        // Add the rightmost(for LTR box)/leftmost(for RTL box) boundary as wordbreak to handle multi-spaces. 
> +        // For example "<div>abc    def hij    opq rst</div>",
> +        // when cursor is immediately to the right of "opq", its previous word boundary's offset returned from previousWordPosition() is 15,
> +        // which is immediately to the right of "hij ", and which is not in the same box as "opq",
> +        // so, it wont be added as a word break of box "opq rst". It has to be added here as a word break.
> +        // Or we need to provide a way not to canonicalize(upstream) VisiblePosition in its constructor, 
> +        // so previousWordPosition() of "opq" could return offset 18, which is immediately to the left of "opq", instead.
> +        wordBreak.getInlineBoxAndOffset(boxContainingPreviousWordBreak, offsetOfWordBreak);
> +        if (boxContainingPreviousWordBreak == box)

Mn... this logic is way too complicated for me to review.  mitz or someone else who knows rendering code well should review should this part.

> Source/WebCore/editing/visible_units.cpp:1255
> +            return wordBreak;

Instead of assigning the value of wordBreak, we should just return Position(box->renderer()->node(), box->caretMaxOffset(), Position::PositionIsOffsetInAnchor).

> Source/WebCore/editing/visible_units.cpp:1284
> +    } else // FIXME not implemented yet.
> +        return VisiblePosition();

Get rid of this else clause.

> Source/WebKit/ChangeLog:15
> [phone]  Xiaomei Ji  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Need a short description and bug URL (OOPS!)
> +
> +        * WebKit.xcodeproj/project.pbxproj:

double change logs!

> LayoutTests/editing/selection/move-by-word-visually-expected.txt:10
> +"abc def hij opq rst"[0, 0] FAIL expected: 4
> +"abc def hij opq rst"[4, 4] FAIL expected: 8
> +"abc def hij opq rst"[8, 8] FAIL expected: 12
> +"abc def hij opq rst"[12, 12] FAIL expected: 16
> +"abc def hij opq rst"[16, 16] FAIL expected: 19
> +"abc def hij opq rst"[19, 19]

These outputs are too verbose.  I think we can instead have something like:
"abc def hij opq rst" FAIL expected [4, 8, 12, 16, 19, 19] but got [0, 4, 8, 12, 16, 19]

> LayoutTests/editing/selection/move-by-word-visually.html:28
> +    var messages = [];
> +
> +    function log(message)
> +    {
> +        messages.push(message);
> +    }

We shouldn't indent all of this code.

> LayoutTests/editing/selection/move-by-word-visually.html:177
> +    <div style="direction:ltr" class="test_move_by_word" title="0 4 8 12 16 19|19 16 12 8 4 0" contenteditable>abc def hij opq rst</div>

Maybe we should use dir attribute so that it's shorter?

[Ryosuke Niwa]

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

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

> Source/WebCore/editing/visible_units.cpp:1268
> +    // In order to get the correct word boundary (the one before space or the one after space), 

This repeats what code does.  We need to explain "why" we need to do this in order to get the correct word boundary instead.

> Source/WebCore/editing/visible_units.cpp:1271
> +    if ((directionToSearch == WebCore::DirectionRight && blockDirection == LTR) 
> +        || (directionToSearch == WebCore::DirectionLeft && blockDirection == RTL)) {

I think this condition should be in wordBoundaryInAdjacentBoxes and we should split wordBoundaryInBox into two functions.
Maybe findWordBoundaryInBoxInBlockDirection?

[Build Bot]

Attachment 87522 [details] did not build on win:
Build output: http://queues.webkit.org/results/8285868

[Xiaomei Ji]

Created attachment 87618 [details]
patch w/ layout test

[Build Bot]

Attachment 87618 [details] did not build on win:
Build output: http://queues.webkit.org/results/8311007

[Xiaomei Ji]

Created attachment 87628 [details]
patch w/ layout test

fix win compilation error

[Ryosuke Niwa]

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

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

> Source/WebCore/editing/SelectionController.cpp:451
> +    // FIXME: remove later.

Since we've already added this comment to where we declared WebKitVisualWordGranularity, I don't think we need to repeat ourselves here.

> Source/WebCore/editing/SelectionController.cpp:493
> +    // FIXME: remove later.

Ditto.

> Source/WebCore/editing/SelectionController.cpp:578
> +    // FIXME: remove later.

Ditto.

> Source/WebCore/editing/SelectionController.cpp:621
> +    // FIXME: remove later.

Ditto.

> Source/WebCore/editing/SelectionController.cpp:668
> +    // FIXME: remove later.

Ditto.

> Source/WebCore/editing/SelectionController.cpp:746
> +    // FIXME: remove later.

Ditto.

> Source/WebCore/editing/VisibleSelection.cpp:383
> +        // FIXME: remove later.

Ditto.

> Source/WebCore/editing/visible_units.cpp:1160
> +    // For LTR text, previousWordPosition() returns the word break at the left boundary of a word, 
> +    //               nextWordPosition() returns the word break at the right boundary of a word. 
> +    // For RTL text, previousWordPosition() returns the word break at the right boundary of a word,
> +    //               nextWordPosition() returns the word break at the left boundary of a word.

I don't understand why we need this comment here.  It seems like we want these comments to be right above previousWordPosition and nextWordPosition instead.
Or, you should explain how these things affect what we do in this function.  Why explaining what nextWordPosition and previousWordPosition do isn't particularly helpful here
because this comment appears before any code where this information is relevant.

> Source/WebCore/editing/visible_units.cpp:1165
> +    // In LTR block, ctrl-arrow moves cursor to the left boundary of words,
> +    // so we should use previousWordPosition() for LTR text, in which offsets collected from visually right to visually left.
> +    // In RTL block, ctrl-arrow moves cursor to the right boundary of words,
> +    // so we should use previousWordPosiiton() for RTL text, in which, offsets collected from visually left to  visually right. 

This comment isn't intelligible for me. What does ctrl-arrow mean?  Which arrow key are we talking about?
You say we should use previousWordPosition() for LTR text but you don't really explain why we should.
And I can't make a sense of previousWordPosition() "collecting" offsets visually right to left.

> Source/WebCore/editing/visible_units.cpp:1185
> +        wordBreak.getInlineBoxAndOffset(boxContainingPreviousWordBreak, offsetOfWordBreak);

I'd declare boxContainingPreviousWordBreak and offsetOfWordBreak here instead of at the beginning of the function.  It's okay to have multiple local variables of the same name as long as they are in separate blocks.

> Source/WebCore/editing/visible_units.cpp:1198
> +        if (box->direction() == blockDirection)
> +            wordBreak = previousWordBreakInBoxInsideBlockWithSameDirectionality(box, wordBreak, offsetOfWordBreak);

I'm a bit confused here.  If we're looking for a word boundary in the text direction of block and box and block have the same directionality, shouldn't we be looking for the next word boundary instead of previous one?

> Source/WebCore/editing/visible_units.cpp:1204
> +    } while (1);        

Since you're putting 1 in while's condition, I'd use the regular while instead of do-while.

> Source/WebCore/editing/visible_units.cpp:1221
> +    const InlineBox* adjacentBox = box;
> +    while (adjacentBox) {
> +        VisiblePosition wordBreak;
> +        if ((directionToSearch == WebCore::DirectionRight && blockDirection == LTR) 
> +            || (directionToSearch == WebCore::DirectionLeft && blockDirection == RTL))
> +            wordBreak = findWordBoundaryInBoxInBlockDirection(adjacentBox, offset, blockDirection);
> +        // FIXME: "else" not implemented yet.
> +        if (!wordBreak.isNull())
> +            return wordBreak;
> +        adjacentBox = (directionToSearch == WebCore::DirectionLeft) ? adjacentBox->nextLeafChild() : adjacentBox->prevLeafChild();
> +        offset = invalidOffset;
> +    }

Since this code is fairly simple, I'd duplicate code in leftWordBoundary and rightWordBoundary rather than adding a function that takes directionToSearch.
You can then rewrite this while loop as a simple for loop (leftWordBoundary case):

static VisiblePosition leftWordBoundary(const InlineBox* box, int offset, TextDirection blockDirection, const VisiblePosition& visiblePosition)
{
    for (const InlineBox* adjacentBox = box; adjacentBox; adjacentBox = adjacentBox->prevLeafChild()) {
        if (blockDirection == RTL)
            wordBreak = findWordBoundaryInBoxInBlockDirection(adjacentBox, adjacentBox == box ? offset : invalidOffset, blockDirection);
        // FIXME: Implement "else" case.
        if (wordBreak.isNotNull())
            return wordBreak;
    }
}

> Source/WebCore/editing/visible_units.cpp:1222
> +    return visiblePosition;

I don't understand the point of returning visiblePosition here.  Is this visiblePosition used in the "else" case?  If not, I'd just return null here and take care of that in leftWordPosition and rightWordPosition.

> Source/WebCore/editing/visible_units.cpp:1238
> +    // In LTR block, ctrl-arrow moves cursor to the left boundary of words.
> +    // In RTL block, ctrl-arrow moves cursor to the right boundary of words.

I can't make sense of this comment.  I think you should remove it.

> LayoutTests/editing/selection/move-by-word-visually-expected.txt:7
> +"abc def hij opq rst"[0, 4, 8, 12, 16, 19]    FAIL expected: [4, 8, 12, 16, 19, 19]
> +Move left by one word
> +"abc def hij opq rst"[16, 16, 12, 8, 4, 0]    FAIL expected: [16, 12, 8, 4, 0, 0]

Very nice output!  I can make sense of outputs now. But the expected result for the second row doesn't look right.
Since we have "0 4 8 12 16 19|19 16 12 8 4 0" in the title, shouldn't we see FAIL expected [19, 16, 12, 8, 4, 0] instead?

[Xiaomei Ji]

Created attachment 87696 [details]
patch w/ layout test

[Xiaomei Ji]

Created attachment 87697 [details]
patch w/ layout test

[Ryosuke Niwa]

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

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

> Source/WebCore/editing/visible_units.cpp:1169
> +    // We need to take care of the word break is at the correct place (before space or after space).
> +    // Follow Firefox's convention in Windows, 
> +    // In LTR block, word break visually moves cursor to the left boundary of words,
> +    // In RTL block, word break visually moves cursor to the right boundary of words.
> +    //
> +    // Since we are using previousWordPosition() and nextWordPosition() to get the word boundary.
> +    // For LTR text, previousWordPosition() returns the word break at the left boundary of a word, 
> +    //               nextWordPosition() returns the word break at the right boundary of a word. 
> +    // For RTL text, previousWordPosition() returns the word break at the right boundary of a word,
> +    //               nextWordPosition() returns the word break at the left boundary of a word.
> +    // 
> +    // So, when the box's directionality is the same as the block's directionality,
> +    // we should use previousWordPosition() for LTR text or RTL text.
> +    // If continously collecting word breaks, for LTR box, the word breaks are collected from right to left.
> +    // for RTL box, word breaks are collected from left to right. 

This comment is too long.  Revise it something like:

// In a LTR block, the word break should be on the left boundary of a word.
// In a RTL block, the word break should be on the right boundary of a word.
// Because nextWordPosition() returns the word break on the right boundary of the word for LTR text,
// we need to use previousWordPosition() to traverse words within the inline boxes from right to left
// to find the previous word break (i.e. the first word break on the left). The same applies to RTL text.

> Source/WebCore/editing/visible_units.cpp:1174
> +    // FIXME: handle multi-spaces. 
> +    // In the case of multi-spaces, word break among the collapsed spaces might not added.
> +    // For example, "<div>abc    def hij    opq rst</div>", no word break is added between "hij" and "opq".

I can't make sense of this comment.  Please get help from Eric on this :)

> Source/WebCore/editing/visible_units.cpp:1219
> +            wordBreak = findWordBoundaryInBoxInBlockDirection(adjacentBox, adjacentBox == box ? offset : invalidOffset, blockDirection);

findWordBoundaryInBoxInBlockDirection isn't the right name for this function.  If we're in LTR context, and looking for a word boundary on the left, that means we're moving to the opposite direction.  Maybe should just call this function previousWordBreakInBox instead.

> Source/WebCore/editing/visible_units.cpp:1245
> +    // Follow Firefox's convention in Windows, 
> +    // In LTR block, word break visually moves cursor to the left boundary of words,
> +    // In RTL block, word break visually moves cursor to the right boundary of words.

This comment is irrelevant to the code below so please remove.

[Xiaomei Ji]

Created attachment 87718 [details]
patch w/ layout test

[Ryosuke Niwa]

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

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

> Source/WebCore/editing/visible_units.cpp:1163
> +    // FIXME: handle multi-spaces. Please refer bug https://bugs.webkit.org/show_bug.cgi?id=57543 for detail.

Just say "FIXME: handle multi-spaces (https://webkit.org/b/57543)".

> Source/WebCore/editing/visible_units.cpp:1166
> +    if (previousWordBreak != wordBreak) {

Let's do early exit here instead.

> Source/WebCore/editing/visible_units.cpp:1170
> +        if (boxContainingPreviousWordBreak == box)
> +            return wordBreak;

Ditto. do:
if (boxContainingPreviousWordBreak != box)
    return VisiblePosition();
return wordBreak;

> Source/WebCore/editing/visible_units.cpp:1197
> +        // FIXME: "else" not implemented yet.

FIXME: Implement the "else" case.

> Source/WebCore/editing/visible_units.cpp:1211
> +        // FIXME: "else" not implemented yet.

Ditto.

[Xiaomei Ji]

Committed r82588: <http://trac.webkit.org/changeset/82588>