[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 #101368|review?                     |review+
               Flag|                            |




--- Comment #23 from Ryosuke Niwa <rniwa at webkit.org>  2011-07-26 12:08:34 PST ---
(From update of attachment 101368)
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?

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