[Webkit-unassigned] [Bug 61344] --webkit-visual-word does not work in multi-line

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61344


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #100723|review?                     |review-
               Flag|                            |




--- Comment #15 from Ryosuke Niwa <rniwa at webkit.org>  2011-07-14 18:42:59 PST ---
(From update of attachment 100723)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.