Bug 61344 – --webkit-visual-word does not work in multi-line

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

[Xiaomei Ji]

it only works within one line and could not go beyond line boundary.

[Xiaomei Ji]

Created attachment 94896 [details]
work in progress

[Xiaomei Ji]

Created attachment 95241 [details]
work in progress

needs layout test.

[Xiaomei Ji]

Created attachment 95693 [details]
patch w/ layout test

[Ryosuke Niwa]

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

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

> Source/WebCore/editing/visible_units.cpp:1469
> +static const RootInlineBox* previousRootInlineBox(const InlineBox* box)

This seems like a very expensive function.  Why do we need to do this?

[Xiaomei Ji]

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

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

>> Source/WebCore/editing/visible_units.cpp:1469
>> +static const RootInlineBox* previousRootInlineBox(const InlineBox* box)
> 
> This seems like a very expensive function.  Why do we need to do this?

RootInlineBox->prevLineBox() and nextLineBox() only catches the multiple lines caused by wrapping or by <br> in <div>abc def<br>hij opq</div>.
For the <br> in <div>abc def <div><br></div>hij opq</div> (or the newline entered by user in contenteditable div), we need to find the previousRootInlineBox in order to go from "hij opq" to "abc def".

[Ryosuke Niwa]

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

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

>>> Source/WebCore/editing/visible_units.cpp:1469
>>> +static const RootInlineBox* previousRootInlineBox(const InlineBox* box)
>> 
>> This seems like a very expensive function.  Why do we need to do this?
> 
> RootInlineBox->prevLineBox() and nextLineBox() only catches the multiple lines caused by wrapping or by <br> in <div>abc def<br>hij opq</div>.
> For the <br> in <div>abc def <div><br></div>hij opq</div> (or the newline entered by user in contenteditable div), we need to find the previousRootInlineBox in order to go from "hij opq" to "abc def".

Can't we just walk on render tree?

[Xiaomei Ji]

Created attachment 96341 [details]
patch w/ layout test

Changed previousRootInlineBox/nextRootInlineBox to walk RenderTree instead of DOM node.
I will add more tests.

[Ryosuke Niwa]

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

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

Great!  New patch looks much better.  But it'll be nice if rendering experts like mitz or dhyatt could give us some feedback.

> Source/WebCore/editing/htmlediting.h:144
> +inline Position createPosition(Node* node, int offset)

This function name sounds too general.  How about createPosiitonAvoidingIgnoredNode?

> Source/WebCore/editing/visible_units.cpp:1506
> +        RenderObject* previousBlockRenderer = renderer->previousSibling();
> +        while (previousBlockRenderer) {
> +            const RenderBlock* blockWithLineBoxes = lastChildBlockWithLineBoxes(toRenderBlock(previousBlockRenderer));
> +            if (blockWithLineBoxes)
> +                return blockWithLineBoxes->lastRootBox();
> +
> +            previousBlockRenderer = previousBlockRenderer->previousSibling();
> +        }

It seems like this loop is almost identical to the one in lastChildBlockWithLineBoxes. Can we combine these two loops?

> Source/WebCore/editing/visible_units.cpp:1564
> +    const InlineFlowBox* leftLineBox = (blockDirection == LTR) ? rootBox->prevLineBox() :
> +                                                                 rootBox->nextLineBox();

I wonder it makes a sense to add a helper function on InlineTextBox.

[Ryosuke Niwa]

mitz, hyatt, & eric,

Could you take a look at the code that walks render tree and see if there are any problems?

[Dave Hyatt]

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

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

>> Source/WebCore/editing/visible_units.cpp:1564
>> +                                                                 rootBox->nextLineBox();
> 
> I wonder it makes a sense to add a helper function on InlineTextBox.

Responding to rniwa, I think we should just leave the code here.

[Xiaomei Ji]

Created attachment 100589 [details]
patch w/ layout test

Thanks for the review!

(In reply to comment #8)
> (From update of attachment 96341 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96341&action=review
> 
> Great!  New patch looks much better.  But it'll be nice if rendering experts like mitz or dhyatt could give us some feedback.
> 
> > Source/WebCore/editing/htmlediting.h:144
> > +inline Position createPosition(Node* node, int offset)
> 
> This function name sounds too general.  How about createPosiitonAvoidingIgnoredNode?

done.

> 
> > Source/WebCore/editing/visible_units.cpp:1506
> > +        RenderObject* previousBlockRenderer = renderer->previousSibling();
> > +        while (previousBlockRenderer) {
> > +            const RenderBlock* blockWithLineBoxes = lastChildBlockWithLineBoxes(toRenderBlock(previousBlockRenderer));
> > +            if (blockWithLineBoxes)
> > +                return blockWithLineBoxes->lastRootBox();
> > +
> > +            previousBlockRenderer = previousBlockRenderer->previousSibling();
> > +        }
> 
> It seems like this loop is almost identical to the one in lastChildBlockWithLineBoxes. Can we combine these two loops?

Thanks for the code snippet. Done.

> 
> > Source/WebCore/editing/visible_units.cpp:1564
> > +    const InlineFlowBox* leftLineBox = (blockDirection == LTR) ? rootBox->prevLineBox() :
> > +                                                                 rootBox->nextLineBox();
> 
> I wonder it makes a sense to add a helper function on InlineTextBox.

no change per hyatt's feedback.

[WebKit Review Bot]

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

Attachment 100589 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9017313

New failing tests:
editing/selection/move-by-word-visually-others.html

[WebKit Review Bot]

Created attachment 100591 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Xiaomei Ji]

Created attachment 100723 [details]
patch w/ layout test

make layout test cross-platform

[Ryosuke Niwa]

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

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

I've got a feeling that previousBlockWithLineBoxes/nextBlockWithLineBoxes are broken for flex boxes.  Please look into it.

> Source/WebCore/ChangeLog:15
> +        Reviewed by NOBODY (OOPS!).

This should appear before the long description.

> Source/WebCore/editing/htmlediting.h:150
> +    return Position(node, offset, Position::PositionIsOffsetInAnchor);

Can this node be non-text?  If so, we'll be introducing new blocker for https://bugs.webkit.org/show_bug.cgi?id=63040.  If not , you should use the one that doesn't take the anchor type. r- due to this change.

To give you some background, we're trying to avoid instantiating a position with (node, offset) pair when the node is not a text.  Instead, we use positions before/after node or positions before/after children.

> Source/WebCore/editing/visible_units.cpp:1325
> +                                createPositionAvoidingIgnoredNode(box->renderer()->node(), box->caretMinOffset());

Nit: should be intended by 4 spaces.

> Source/WebCore/editing/visible_units.cpp:1481
> +static const RenderBlock* previousBlockWithLineBoxes(const RenderBlock* root)

I'm not sure if root is a good name for this.  I'd call it startingBlock.

> Source/WebCore/editing/visible_units.cpp:1483
> +    for (const RenderBlock* previous = root; previous; previous = toRenderBlock(previous->previousSibling())) {

"previous" is a relative term.  I get confused by its use in this function.  Can we just call it block or currentBlock?

> Source/WebCore/editing/visible_units.cpp:1485
> +        // Renderer having inline children with contents has root line boxes. 
> +        if (previous->childrenInline() && previous->firstRootBox())

I don't think this comment adds any new information because you're indeed checking that the first root box isn't null.  Please remove it.

> Source/WebCore/editing/visible_units.cpp:1509
> +    while (renderer) {
> +        while (renderer && !renderer->isRenderBlock())
> +            renderer = renderer->parent();
> +
> +        if (!renderer)
> +            return 0;
> +
> +        if (const RenderBlock* blockWithLineBoxes = previousBlockWithLineBoxes(toRenderBlock(renderer->previousSibling())))
> +            return blockWithLineBoxes->lastRootBox();
> +
> +        renderer = renderer->parent();
> +    }

I started to think that these nested loops can be flattened as follows:

for (RenderObject* renderer = node->renderer(); renderer; renderer = renderer->parent()) {
    if (renderer->isRenderBlock()) {
        if (const RenderBlock* blockWithLineBoxes = previousBlockWithLineBoxes(toRenderBlock(renderer->previousSibling()))
            return blockWithLineBoxes->lastRootBox();
    }
}

no?

> Source/WebCore/editing/visible_units.cpp:1516
> +        // Renderer having inline children with contents has root line boxes. 

Ditto about the comment.

> Source/WebCore/editing/visible_units.cpp:1532
> +    while (renderer) {
> +        while (renderer && !renderer->isRenderBlock())
> +            renderer = renderer->parent();

Ditto about flattening the nested loop.

> Source/WebCore/editing/visible_units.cpp:1552
> +                                                                 rootBox->nextLineBox();

Nit: indentation.

> Source/WebCore/editing/visible_units.cpp:1564
> +    if (leftRootInlineBox)
> +        return leftRootInlineBox->lastLeafChild();
> +    return 0;

I'd use tertiary here.

> Source/WebCore/editing/visible_units.cpp:1574
> +                                                                  rootBox->prevLineBox();

Nit: indentation.

> Source/WebCore/editing/visible_units.cpp:1586
> +    if (rightRootInlineBox)
> +        return rightRootInlineBox->firstLeafChild();
> +    return 0;

Nit: tertiary.

> Source/WebCore/editing/visible_units.cpp:1704
> +    VisiblePosition rightWordBreak = rightWordPositionAcrossBoundary(visiblePosition);

You should check the nullity of visiblePosition here.

> LayoutTests/editing/selection/move-by-word-visually-others.html:94
> +<div contenteditable dir=ltr id="multi_line_1" class="test_move_by_word fix_width" style="width:50px" title="[multi_line_1, 0][multi_line_1, 4][multi_line_1, 8][multi_line_1, 12][multi_line_1, 16][multi_line_1, 0, 5][multi_line_1, 4, 5][multi_line_1, 8, 5][multi_line_1, 12, 5]|[multi_line_1, 15, 5][multi_line_1, 12, 5][multi_line_1, 8, 5][multi_line_1, 4, 5][multi_line_1, 0, 5][multi_line_1, 16][multi_line_1, 12][multi_line_1, 8][multi_line_1, 4][multi_line_1, 0]">abc def ghi jkl mn <br/><br/><br/>opq rst uvw xyz</div>

Can we give a shorter id to make the expected result shorter?

[Ryosuke Niwa]

(In reply to comment #15)
> (From update of attachment 100723 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100723&action=review
> 
> I've got a feeling that previousBlockWithLineBoxes/nextBlockWithLineBoxes are broken for flex boxes.  Please look into it.

I talked with Tony, who is implementing new flex box, and confirmed that these function work as expected with flex boxes.

[Xiaomei Ji]

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

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

>> Source/WebCore/editing/visible_units.cpp:1509
>> +    }
> 
> I started to think that these nested loops can be flattened as follows:
> 
> for (RenderObject* renderer = node->renderer(); renderer; renderer = renderer->parent()) {
>     if (renderer->isRenderBlock()) {
>         if (const RenderBlock* blockWithLineBoxes = previousBlockWithLineBoxes(toRenderBlock(renderer->previousSibling()))
>             return blockWithLineBoxes->lastRootBox();
>     }
> }
> 
> no?

Yes, and Thanks!

[Xiaomei Ji]

Created attachment 101358 [details]
patch w/ layout test

updated per comment #15

[Gyuyoung Kim]

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

Attachment 101358 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9158214

[WebKit Review Bot]

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

Attachment 101358 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9159225

[Early Warning System Bot]

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

Attachment 101358 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9158219

[Xiaomei Ji]

Created attachment 101368 [details]
patch w/ layout test

fix  compilation error.

[Ryosuke Niwa]

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

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

> Source/WebCore/editing/htmlediting.cpp:1144
> +    if (editingIgnoresContent(node) || !node->isTextNode()) {

It appears that all we need to check is !node->isTextNode().  Since editingIgnoresContent calls a virtual function, we should get rid of this function call if possible.

> Source/WebCore/editing/visible_units.cpp:1539
> +

Nit: Please get rid of this blank line.

> Source/WebCore/editing/visible_units.cpp:1541
> +    const InlineFlowBox* leftLineBox = (blockDirection == LTR) ? rootBox->prevLineBox() :
> +        rootBox->nextLineBox();

Nit: we can fit this in one line if we had added a local variable like "const bool isBlockLTR = blockDirection == LTR;"

> Source/WebCore/editing/visible_units.cpp:1550
> +    const RootInlineBox* leftRootInlineBox;
> +    if (blockDirection == LTR)
> +        leftRootInlineBox = previousRootInlineBox(box);
> +    else
> +        leftRootInlineBox = nextRootInlineBox(box);

Nit: I would have used tertiary.

> Source/WebCore/editing/visible_units.cpp:1563
> +
> +    const InlineFlowBox* rightLineBox = (blockDirection == LTR) ? rootBox->nextLineBox() :
> +        rootBox->prevLineBox();

Ditto.

> Source/WebCore/editing/visible_units.cpp:1572
> +    const RootInlineBox* rightRootInlineBox;
> +    if (blockDirection == LTR)
> +        rightRootInlineBox = nextRootInlineBox(box);
> +    else
> +        rightRootInlineBox = previousRootInlineBox(box);

Ditto.

> LayoutTests/editing/selection/move-by-word-visually-others.html:93
> +<div contenteditable dir=ltr id="ml_1" class="test_move_by_word fix_width" style="width:50px" title="[ml_1, 0][ml_1, 4][ml_1, 8][ml_1, 12][ml_1, 16][ml_1, 0, 5][ml_1, 4, 5][ml_1, 8, 5][ml_1, 12, 5]|[ml_1, 15, 5][ml_1, 12, 5][ml_1, 8, 5][ml_1, 4, 5][ml_1, 0, 5][ml_1, 16][ml_1, 12][ml_1, 8][ml_1, 4][ml_1, 0]">abc def ghi jkl mn <br/><br/><br/>opq rst uvw xyz</div>

I'm afraid that specifying "50px" will make this test platform-dependent. I'd at least use "ex" or "em" and only use "x" or some other letter since mixing letters will make the text width more dependent on font metrics.

> LayoutTests/editing/selection/move-by-word-visually-others.html:108
> +<div contenteditable dir=rtl id="ml_7" class="test_move_by_word fix_width" style="width:50px" title="[ml_7, 15, 5][ml_7, 11, 5][ml_7, 8, 5][ml_7, 3, 5][ml_7, 0, 5][ml_7, 16][ml_7, 11][ml_7, 8][ml_7, 3][ml_7, 0]|[ml_7, 0][ml_7, 3][ml_7, 8][ml_7, 11][ml_7, 16][ml_7, 0, 5][ml_7, 3, 5][ml_7, 8, 5][ml_7, 11, 5]">abc def ghi jkl mn <br/><br/><br/>opq rst uvw xyz</div>
> +
> +<div contenteditable dir=rtl id="ml_8" class="test_move_by_word fix_width" style="width:50px" title="[ml_8, 15, 5][ml_8, 11, 5][ml_8, 8, 5][ml_8, 3, 5][ml_8, 0, 5][ml_8, 18][ml_8, 16][ml_8, 11][ml_8, 8][ml_8, 3][ml_8, 0]|[ml_8, 0][ml_8, 3][ml_8, 8][ml_8, 11][ml_8, 16][ml_8, 18][ml_8, 0, 5][ml_8, 3, 5][ml_8, 8, 5][ml_8, 11, 5]">abc def ghi jkl mn <div><br/></div><div><br/></div><div><br/></div>opq rst uvw xyz</div>

Ditto about width.

> LayoutTests/editing/selection/move-by-word-visually-single-space-sigle-line.html:68
> -<div dir=ltr id="div_1" class="test_move_by_word" title="[div_1, 0][div_1, 3]|[span_1, 2][div_1, 3][div_1,0]" contenteditable>××× <span id="span_1">××</span></div>
> -<div dir=rtl id="div_2" class="test_move_by_word" title="[span_2, 2][div_2, 4][div_2, 0]|[div_2, 0][div_2, 4]" contenteditable>××× <span id="span_2">××</span></div>
> +<div dir=ltr id="d_1" class="test_move_by_word" title="[d_1, 0][d_1, 3]|[s_1, 2][d_1, 3][d_1,0]" contenteditable>××× <span id="s_1">××</span></div>
> +<div dir=rtl id="d_2" class="test_move_by_word" title="[s_2, 2][d_2, 4][d_2, 0]|[d_2, 0][d_2, 4]" contenteditable>××× <span id="s_2">××</span></div>

Why don't I see these changes in expected.txt?

> LayoutTests/editing/selection/resources/move-by-word-visually.js:247
> +        var span = document.getElementById("span_size");
> +        var length = span.offsetWidth;
> +        test.style.width = length + "px";

Mn... given this code maybe you didn't need to set width property in above test cases anyways?

[Xiaomei Ji]

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

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

>> LayoutTests/editing/selection/move-by-word-visually-others.html:93
>> +<div contenteditable dir=ltr id="ml_1" class="test_move_by_word fix_width" style="width:50px" title="[ml_1, 0][ml_1, 4][ml_1, 8][ml_1, 12][ml_1, 16][ml_1, 0, 5][ml_1, 4, 5][ml_1, 8, 5][ml_1, 12, 5]|[ml_1, 15, 5][ml_1, 12, 5][ml_1, 8, 5][ml_1, 4, 5][ml_1, 0, 5][ml_1, 16][ml_1, 12][ml_1, 8][ml_1, 4][ml_1, 0]">abc def ghi jkl mn <br/><br/><br/>opq rst uvw xyz</div>
> 
> I'm afraid that specifying "50px" will make this test platform-dependent. I'd at least use "ex" or "em" and only use "x" or some other letter since mixing letters will make the text width more dependent on font metrics.

Looks like for pure ascii, the result is cross-platform if  font-size and font-family are fixed. But you are probably right that I do not need to set width here since I am setting width in JS. BTW, if width is set here and by JS, which one take precedence?

>> LayoutTests/editing/selection/move-by-word-visually-single-space-sigle-line.html:68
>> +<div dir=rtl id="d_2" class="test_move_by_word" title="[s_2, 2][d_2, 4][d_2, 0]|[d_2, 0][d_2, 4]" contenteditable>××× <span id="s_2">××</span></div>
> 
> Why don't I see these changes in expected.txt?

id does not appear in expected file. instead of element id, the real content appears in expected file, which should make the expected file easier to read.

[Xiaomei Ji]

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

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

>>> LayoutTests/editing/selection/move-by-word-visually-others.html:93
>>> +<div contenteditable dir=ltr id="ml_1" class="test_move_by_word fix_width" style="width:50px" title="[ml_1, 0][ml_1, 4][ml_1, 8][ml_1, 12][ml_1, 16][ml_1, 0, 5][ml_1, 4, 5][ml_1, 8, 5][ml_1, 12, 5]|[ml_1, 15, 5][ml_1, 12, 5][ml_1, 8, 5][ml_1, 4, 5][ml_1, 0, 5][ml_1, 16][ml_1, 12][ml_1, 8][ml_1, 4][ml_1, 0]">abc def ghi jkl mn <br/><br/><br/>opq rst uvw xyz</div>
>> 
>> I'm afraid that specifying "50px" will make this test platform-dependent. I'd at least use "ex" or "em" and only use "x" or some other letter since mixing letters will make the text width more dependent on font metrics.
> 
> Looks like for pure ascii, the result is cross-platform if  font-size and font-family are fixed. But you are probably right that I do not need to set width here since I am setting width in JS. BTW, if width is set here and by JS, which one take precedence?

will remove the width style since it is reset in JS.

[Xiaomei Ji]

Created attachment 102043 [details]
for EWS

[Xiaomei Ji]

Committed r91788: <http://trac.webkit.org/changeset/91788>

[Xiaomei Ji]

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

clear flag on landing patch.

[Xiaomei Ji]

the fix was rolled back in http://trac.webkit.org/changeset/93935

[Xiaomei Ji]

Created attachment 107738 [details]
patch w/ layout test

These functions are only used in moving cursor by word, more specifically, the 2 newly added functions only triggered when finding word break on next or previous line.
Considering the speed of key press in  real use case, where user press ctrl (alt) + arrow to move cursor by word,  and considering the functions are only triggered when searching words across line boundary ("\n" in text field, not wrapped lines), I do not think performance is a concern here. Traversing DOM node is a much cleaner/safer solution.

[Ryosuke Niwa]

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

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

> Source/WebCore/ChangeLog:10
> +        DOM node.

Nit: it seems like DOM node fits in the previous line.

> Source/WebCore/editing/visible_units.cpp:1485
> +        Position pos;
> +        pos = createLegacyEditingPosition(previousNode, caretMaxOffset(previousNode));

Nit: this should be don in one line.

> Source/WebCore/editing/visible_units.cpp:1487
> +        if (pos.isCandidate()) {

Why do we need to check that this is a candidate?

> Source/WebCore/editing/visible_units.cpp:1493
> +            InlineBox* boxInPreviousNode;
> +            int ignoredCaretOffset;
> +            pos.getInlineBoxAndOffset(DOWNSTREAM, boxInPreviousNode, ignoredCaretOffset);
> +
> +            if (boxInPreviousNode)
> +                return boxInPreviousNode->root();

Please use RenderedPosition.

> Source/WebCore/editing/visible_units.cpp:1539
> +                                                          nextRootInlineBox(box); 

Nit: wrong indentation.

[Xiaomei Ji]

Created attachment 110748 [details]
WIP

Comparing with previously reviewed patch (107738), this WIP extracts the getting next root inline box functionality from nextLinePosition() and shares it with nextRootInlineBox().
I have not extract the getting previous root inline box functionality from previousLinePosition() and share with previousRootInlineBox() yet, want to get the idea that this is in the right track.

[WebKit Review Bot]

Attachment 110748 [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/WebCore/editing/visible_units.cpp:641:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 9 files


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

[Ryosuke Niwa]

Comment on attachment 110748 [details]
WIP

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

> Source/WebCore/editing/visible_units.cpp:606
> +struct RootInlineBoxAndPositionInfo {
> +    RootInlineBoxAndPositionInfo(EditingBoundaryCrossingRule rule, int offset, Node* root, bool returnPos)
> +        : rule(rule)
> +        , offset(offset)
> +        , highestRoot(root)
> +        , returnCandidatePosition(returnPos)
> +    {
> +    }

Why do we need this struct? Can't we just pass arguments around?

> Source/WebCore/editing/visible_units.cpp:629
> +static void nextRootInlineBoxOrCandidatePos(Node* node, const RootInlineBoxAndPositionInfo& info,
> +    RootInlineBox*& rootInlineBox, Position& candidatePos) {

Maybe this needs to moved to RenderedPosition.

> Source/WebCore/editing/visible_units.cpp:694
> +                                          highestEditableRoot(p), true);

Nit: wrong indentation.

> Source/WebCore/editing/visible_units.cpp:1523
> +static const RootInlineBox* previousRootInlineBox(const InlineBox* box)

Why is this function different from nextRootInlineBoxOrCandidatePos? These two functions should be symmetric to each other.

[Xiaomei Ji]

Comment on attachment 110748 [details]
WIP

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

> Source/WebCore/editing/visible_units.cpp:656
> +            // FIXME: how to handle it if we only want to return next root inline box.

I am not sure how to handle the case if we'd like to get next root inline box only which is the case in nextRootInlineBox(). Ignoring this position might miss some visible position candidates, but I have not thought of a test case yet. I am thinking maybe we could handle this iteratively.

[Xiaomei Ji]

Comment on attachment 110748 [details]
WIP

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

>> Source/WebCore/editing/visible_units.cpp:606
>> +    }
> 
> Why do we need this struct? Can't we just pass arguments around?

yes you are right that we do not need this struct. instead, we can just pass arguments around. then, instead of a struct, we will need to pass 4 arguments around. Also, the |offset| and |root| are only needed if the |rule| is CanNotCrossEditingBoundary.

>> Source/WebCore/editing/visible_units.cpp:629
>> +    RootInlineBox*& rootInlineBox, Position& candidatePos) {
> 
> Maybe this needs to moved to RenderedPosition.

what is the rule-of-thumb here?

>> Source/WebCore/editing/visible_units.cpp:1523
>> +static const RootInlineBox* previousRootInlineBox(const InlineBox* box)
> 
> Why is this function different from nextRootInlineBoxOrCandidatePos? These two functions should be symmetric to each other.

Yes, you are right. I have not make that change yet. would like to check with you that this is in the right track first.

[Ryosuke Niwa]

The general approach of extracting code from the existing visible_units functions is good but I'm concerned about new struct and the fact next/previous functions seem to have completely different interface and behavior.

[Ryosuke Niwa]

(In reply to comment #36)
> (From update of attachment 110748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110748&action=review
> 
> >> Source/WebCore/editing/visible_units.cpp:606
> >> +    }
> > 
> > Why do we need this struct? Can't we just pass arguments around?
> 
> yes you are right that we do not need this struct. instead, we can just pass arguments around. then, instead of a struct, we will need to pass 4 arguments around. Also, the |offset| and |root| are only needed if the |rule| is CanNotCrossEditingBoundary.

Could you elaborate on why we need those 4 arguments?

> >> Source/WebCore/editing/visible_units.cpp:629
> >> +    RootInlineBox*& rootInlineBox, Position& candidatePos) {
> > 
> > Maybe this needs to moved to RenderedPosition.
> 
> what is the rule-of-thumb here?

RenderedPosition is meant to provide abstraction around renderers and inline boxes in editing code. If the code touches inline boxes and/or talks on render tree, then it's a strong indication that it should belong in RenderedPosition.

[Xiaomei Ji]

Comment on attachment 110748 [details]
WIP

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

>>>> Source/WebCore/editing/visible_units.cpp:606
>>>> +    }
>>> 
>>> Why do we need this struct? Can't we just pass arguments around?
>> 
>> yes you are right that we do not need this struct. instead, we can just pass arguments around. then, instead of a struct, we will need to pass 4 arguments around. Also, the |offset| and |root| are only needed if the |rule| is CanNotCrossEditingBoundary.
> 
> Could you elaborate on why we need those 4 arguments?

this function is extracted to be shared by nextLinePosition() and nextRootInlineBox().
nextLinePosition() honor editing boundary while nextRootInlineBox() does not. This is from the 1st data member.

When it is honor editing boundary, in original code inside nextLinePosition(), the first next leaf node it gets is through nextLeafWithSameEditability(), and it accepts 2 parameters -- the node, and the original visible position's Position's offset which is the 2nd data member here.

When it is honor editing boundary, the code exits earlier when the next node's highest editable root is not the same as original node's highest editable root. original node's highest editable root is the 3rd data member.

In original code inside nextLinePosition(), when position is a candidate but without root inline box, this position is returned as next line position. I am not clear on what to do when we only want to get next root inline box in this case (pls. refer comment at line 656), and I ignored such case for nextRootInlineBox(), hence the 4th parameter to differentiate.

[Ryosuke Niwa]

Comment on attachment 110748 [details]
WIP

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

>>>>> Source/WebCore/editing/visible_units.cpp:606
>>>>> +    }
>>>> 
>>>> Why do we need this struct? Can't we just pass arguments around?
>>> 
>>> yes you are right that we do not need this struct. instead, we can just pass arguments around. then, instead of a struct, we will need to pass 4 arguments around. Also, the |offset| and |root| are only needed if the |rule| is CanNotCrossEditingBoundary.
>> 
>> Could you elaborate on why we need those 4 arguments?
> 
> this function is extracted to be shared by nextLinePosition() and nextRootInlineBox().
> nextLinePosition() honor editing boundary while nextRootInlineBox() does not. This is from the 1st data member.
> 
> When it is honor editing boundary, in original code inside nextLinePosition(), the first next leaf node it gets is through nextLeafWithSameEditability(), and it accepts 2 parameters -- the node, and the original visible position's Position's offset which is the 2nd data member here.
> 
> When it is honor editing boundary, the code exits earlier when the next node's highest editable root is not the same as original node's highest editable root. original node's highest editable root is the 3rd data member.
> 
> In original code inside nextLinePosition(), when position is a candidate but without root inline box, this position is returned as next line position. I am not clear on what to do when we only want to get next root inline box in this case (pls. refer comment at line 656), and I ignored such case for nextRootInlineBox(), hence the 4th parameter to differentiate.

It seems to me we need to decide to do either and refactor the code first accordingly. It's not great to support both behaviors.

[Xiaomei Ji]

Comment on attachment 110748 [details]
WIP

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

>>>>>> Source/WebCore/editing/visible_units.cpp:606
>>>>>> +    }
>>>>> 
>>>>> Why do we need this struct? Can't we just pass arguments around?
>>>> 
>>>> yes you are right that we do not need this struct. instead, we can just pass arguments around. then, instead of a struct, we will need to pass 4 arguments around. Also, the |offset| and |root| are only needed if the |rule| is CanNotCrossEditingBoundary.
>>> 
>>> Could you elaborate on why we need those 4 arguments?
>> 
>> this function is extracted to be shared by nextLinePosition() and nextRootInlineBox().
>> nextLinePosition() honor editing boundary while nextRootInlineBox() does not. This is from the 1st data member.
>> 
>> When it is honor editing boundary, in original code inside nextLinePosition(), the first next leaf node it gets is through nextLeafWithSameEditability(), and it accepts 2 parameters -- the node, and the original visible position's Position's offset which is the 2nd data member here.
>> 
>> When it is honor editing boundary, the code exits earlier when the next node's highest editable root is not the same as original node's highest editable root. original node's highest editable root is the 3rd data member.
>> 
>> In original code inside nextLinePosition(), when position is a candidate but without root inline box, this position is returned as next line position. I am not clear on what to do when we only want to get next root inline box in this case (pls. refer comment at line 656), and I ignored such case for nextRootInlineBox(), hence the 4th parameter to differentiate.
> 
> It seems to me we need to decide to do either and refactor the code first accordingly. It's not great to support both behaviors.

nextRootInlinBox() can do honor editing boundary. It is just that honor editing boundary is the last step in those editing operations, so that we do that as the last step in rightWordPosition()/leftWordPosition. If we honor editing boundary in nextRootInlineBox, the functionality should be the same, but the code will exit earlier.

[Xiaomei Ji]

Created attachment 111092 [details]
WIP

[WebKit Review Bot]

Comment on attachment 111092 [details]
WIP

Attachment 111092 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10068479

[Early Warning System Bot]

Comment on attachment 111092 [details]
WIP

Attachment 111092 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10056710

[Xiaomei Ji]

Created attachment 111103 [details]
WIP

fix EWS error.

[Adam Barth]

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

Cleared Ryosuke Niwa's review+ from obsolete attachment 101368 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Xiaomei Ji]

Created attachment 112036 [details]
patch w/ layout test

talked with rniwa offline and upload this patch which only fixes the nits in attachement 107738

[Ryosuke Niwa]

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

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

> Source/WebCore/editing/visible_units.cpp:1492
> +            InlineBox* boxInPreviousNode;
> +            int ignoredCaretOffset;
> +            pos.getInlineBoxAndOffset(DOWNSTREAM, boxInPreviousNode, ignoredCaretOffset);
> +
> +            if (boxInPreviousNode)
> +                return boxInPreviousNode->root();

Please use RenderedPosition to obtain the root inline box.

> Source/WebCore/editing/visible_units.cpp:1518
> +            InlineBox* boxInNextNode;
> +            int ignoredCaretOffset;
> +            pos.getInlineBoxAndOffset(DOWNSTREAM, boxInNextNode, ignoredCaretOffset);
> +
> +            if (boxInNextNode)
> +                return boxInNextNode->root();

Ditto.

[Xiaomei Ji]

Committed r98428: <http://trac.webkit.org/changeset/98428>