Bug 61344 - --webkit-visual-word does not work in multi-line
Summary: --webkit-visual-word does not work in multi-line
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Blocker
Assignee: Nobody
URL:
Keywords:
Depends on: 65241
Blocks: 25298
  Show dependency treegraph
 
Reported: 2011-05-23 22:31 PDT by Xiaomei Ji
Modified: 2011-10-25 21:56 PDT (History)
8 users (show)

See Also:


Attachments
work in progress (5.16 KB, patch)
2011-05-25 18:04 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
work in progress (11.25 KB, patch)
2011-05-27 18:27 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (27.80 KB, patch)
2011-06-01 18:06 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (29.78 KB, patch)
2011-06-07 17:36 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (29.38 KB, patch)
2011-07-12 16:48 PDT, Xiaomei Ji
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (5.78 MB, application/zip)
2011-07-12 17:10 PDT, WebKit Review Bot
no flags Details
patch w/ layout test (30.40 KB, patch)
2011-07-13 15:35 PDT, Xiaomei Ji
rniwa: review-
Details | Formatted Diff | Diff
patch w/ layout test (41.84 KB, patch)
2011-07-19 12:06 PDT, Xiaomei Ji
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
patch w/ layout test (41.84 KB, patch)
2011-07-19 13:11 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
for EWS (41.61 KB, patch)
2011-07-26 12:57 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (29.48 KB, patch)
2011-09-16 15:47 PDT, Xiaomei Ji
rniwa: review-
Details | Formatted Diff | Diff
WIP (34.22 KB, patch)
2011-10-12 14:45 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
WIP (45.29 KB, patch)
2011-10-14 15:18 PDT, Xiaomei Ji
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
WIP (45.32 KB, patch)
2011-10-14 16:02 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (29.40 KB, patch)
2011-10-21 15:38 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-05-23 22:31:15 PDT
it only works within one line and could not go beyond line boundary.
Comment 1 Xiaomei Ji 2011-05-25 18:04:06 PDT
Created attachment 94896 [details]
work in progress
Comment 2 Xiaomei Ji 2011-05-27 18:27:49 PDT
Created attachment 95241 [details]
work in progress

needs layout test.
Comment 3 Xiaomei Ji 2011-06-01 18:06:08 PDT
Created attachment 95693 [details]
patch w/ layout test
Comment 4 Ryosuke Niwa 2011-06-01 18:39:17 PDT
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?
Comment 5 Xiaomei Ji 2011-06-01 18:52:28 PDT
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".
Comment 6 Ryosuke Niwa 2011-06-01 19:54:15 PDT
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?
Comment 7 Xiaomei Ji 2011-06-07 17:36:07 PDT
Created attachment 96341 [details]
patch w/ layout test

Changed previousRootInlineBox/nextRootInlineBox to walk RenderTree instead of DOM node.
I will add more tests.
Comment 8 Ryosuke Niwa 2011-06-20 18:56:06 PDT
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.
Comment 9 Ryosuke Niwa 2011-06-20 18:56:53 PDT
mitz, hyatt, & eric,

Could you take a look at the code that walks render tree and see if there are any problems?
Comment 10 Dave Hyatt 2011-07-12 09:53:02 PDT
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.
Comment 11 Xiaomei Ji 2011-07-12 16:48:12 PDT
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.
Comment 12 WebKit Review Bot 2011-07-12 17:10:19 PDT
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
Comment 13 WebKit Review Bot 2011-07-12 17:10:25 PDT
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
Comment 14 Xiaomei Ji 2011-07-13 15:35:56 PDT
Created attachment 100723 [details]
patch w/ layout test

make layout test cross-platform
Comment 15 Ryosuke Niwa 2011-07-14 18:42:59 PDT
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?
Comment 16 Ryosuke Niwa 2011-07-14 18:58:45 PDT
(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.
Comment 17 Xiaomei Ji 2011-07-15 10:29:54 PDT
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!
Comment 18 Xiaomei Ji 2011-07-19 12:06:24 PDT
Created attachment 101358 [details]
patch w/ layout test

updated per comment #15
Comment 19 Gyuyoung Kim 2011-07-19 12:13:59 PDT
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
Comment 20 WebKit Review Bot 2011-07-19 12:23:22 PDT
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
Comment 21 Early Warning System Bot 2011-07-19 12:24:37 PDT
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
Comment 22 Xiaomei Ji 2011-07-19 13:11:23 PDT
Created attachment 101368 [details]
patch w/ layout test

fix  compilation error.
Comment 23 Ryosuke Niwa 2011-07-26 12:08:34 PDT
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?
Comment 24 Xiaomei Ji 2011-07-26 12:23:56 PDT
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.
Comment 25 Xiaomei Ji 2011-07-26 12:41:14 PDT
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.
Comment 26 Xiaomei Ji 2011-07-26 12:57:00 PDT
Created attachment 102043 [details]
for EWS
Comment 27 Xiaomei Ji 2011-07-26 14:51:59 PDT
Committed r91788: <http://trac.webkit.org/changeset/91788>
Comment 28 Xiaomei Ji 2011-09-16 15:15:42 PDT
Comment on attachment 101368 [details]
patch w/ layout test

clear flag on landing patch.
Comment 29 Xiaomei Ji 2011-09-16 15:17:05 PDT
the fix was rolled back in http://trac.webkit.org/changeset/93935
Comment 30 Xiaomei Ji 2011-09-16 15:47:13 PDT
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.
Comment 31 Ryosuke Niwa 2011-10-04 14:46:37 PDT
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.
Comment 32 Xiaomei Ji 2011-10-12 14:45:42 PDT
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.
Comment 33 WebKit Review Bot 2011-10-12 14:48:19 PDT
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.
Comment 34 Ryosuke Niwa 2011-10-12 14:50:36 PDT
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.
Comment 35 Xiaomei Ji 2011-10-12 14:50:47 PDT
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.
Comment 36 Xiaomei Ji 2011-10-12 14:56:28 PDT
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.
Comment 37 Ryosuke Niwa 2011-10-12 15:35:46 PDT
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.
Comment 38 Ryosuke Niwa 2011-10-12 15:37:08 PDT
(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.
Comment 39 Xiaomei Ji 2011-10-12 16:23:27 PDT
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.
Comment 40 Ryosuke Niwa 2011-10-12 16:50:39 PDT
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.
Comment 41 Xiaomei Ji 2011-10-12 18:16:47 PDT
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.
Comment 42 Xiaomei Ji 2011-10-14 15:18:18 PDT
Created attachment 111092 [details]
WIP
Comment 43 WebKit Review Bot 2011-10-14 15:38:40 PDT
Comment on attachment 111092 [details]
WIP

Attachment 111092 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10068479
Comment 44 Early Warning System Bot 2011-10-14 15:51:49 PDT
Comment on attachment 111092 [details]
WIP

Attachment 111092 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10056710
Comment 45 Xiaomei Ji 2011-10-14 16:02:30 PDT
Created attachment 111103 [details]
WIP

fix EWS error.
Comment 46 Adam Barth 2011-10-15 01:52:48 PDT
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.
Comment 47 Xiaomei Ji 2011-10-21 15:38:20 PDT
Created attachment 112036 [details]
patch w/ layout test

talked with rniwa offline and upload this patch which only fixes the nits in attachement 107738
Comment 48 Ryosuke Niwa 2011-10-21 18:14:10 PDT
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.
Comment 49 Xiaomei Ji 2011-10-25 21:56:59 PDT
Committed r98428: <http://trac.webkit.org/changeset/98428>