[Webkit-unassigned] [Bug 57336] experiment with moving caret by word in visual order

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from Ryosuke Niwa <rniwa at webkit.org>  2011-03-30 07:22:24 PST ---
(From update of attachment 87522)
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
> +2011-03-30  Xiaomei Ji  <xji at chromium.org>
> +
> +        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?

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