[Webkit-unassigned] [Bug 85017] enable ctrl-arrow move by word visually in other platforms (besides Windows)

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Ryosuke Niwa <rniwa at webkit.org>  2012-04-30 17:12:44 PST ---
(From update of attachment 139541)
View in context: https://bugs.webkit.org/attachment.cgi?id=139541&action=review

> Source/WebCore/ChangeLog:8
> +        Enable ctrl-arrow moves caret by word in visual order in other platforms (besides Windows) that use ICU word

What do you mean by "besides Windows"? It's probably better to say on non-Windows platforms.

> Source/WebCore/ChangeLog:9
> +        break iterator (it is not enabled for WINCE and QT where ICU is not used). For those platforms, ctrl-arrow

Nit: WinCE and Qt ports.

> Source/WebCore/ChangeLog:11
> +        break positions using ctrl-left-arrow from rightmost position are "|abc |def |hij" (same as Windows platform).

Nit: same as the convention on Windows? What are you referring to on Windows?

> Source/WebCore/editing/EditingBehavior.h:71
> +    // Based on native behavior, when using ctrl(alt)+arrow to move caret by word, ctrl(alt)+left_arrow moves to the

Why do we need an underscore between left and arrow?

> Source/WebCore/editing/EditingBehavior.h:72
> +    // beginning of word in all platforms, for example, the word break positions are: "|abc |def |hij |opq".

It's probably better to say "immediately before the word" instead of "beginning".

> Source/WebCore/editing/EditingBehavior.h:73
> +    // But ctrl+right_arrow moves to the beginning of word in Windows in a way that caret should be moved to after the

Ditto about _ between right and arrow.

> Source/WebCore/editing/EditingBehavior.h:77
> +    // white space. For example, the word break positions are: "abc |def |hij |opq".
> +    // while ctrl(alt)+right_arrow moves to the end of word in Mac and Linux in a way that caret should be moved to
> +    // before the white space. For example, the word break positions are "abc| def| hij| opq|".
> +    bool shouldSkipSpaceBeforeWord() const { return m_type == EditingWindowsBehavior; }

I'm sorry but I get more confused by the comment. I would just do:
// "abc |def |hij |opq" on Windows and "abc| def| hij| opq|" on Mac and Linux when moving forward between word boundaries.
shouldSkipSpaceAfterWordOnForwardWordBoundaryMovement()
if I were you (note more verbose function name).

> LayoutTests/editing/selection/move-by-word-visually-mac-expected.txt:90
> +" abc def AAA AAA hij AAA AAA uvw xyz "[1, 5, 8, 12, 16, 20, 24, 28, 32, 36], <DIV>[0], "AAA kj AAA mn opq AAA AAA"[3, 6, 10, 13, 17, 21, 25]    FAIL expected: [" abc def AAA AAA hij AAA AAA uvw xyz "[ 1,  5,  8,  12,  16,  20,  24,  28,  32,  36, ]"AAA kj AAA mn opq AAA AAA"[ 3,  6,  10,  13,  17,  21,  25]

Why is this case failing? Offsets do match here.

> LayoutTests/editing/selection/move-by-word-visually-mac-expected.txt:103
> +"    abc    def    hij    opq    "[28, 22, 15, 8, 4]

Why is the second position 22? Shouldn't it be 25? And then 18, 11, then 4?
It seems odd to use a position between whitespaces like this.

> LayoutTests/editing/selection/move-by-word-visually-mac.html:-26
> -        var tests = document.getElementsByClassName("test_move_by_word");
> -        var sel = getSelection();
> -        for (var i = 0; i < tests.length; ++i) {
> -            if (tests[i].getAttribute("dir") == 'ltr')
> -                log("Test " + (i + 1) + ", LTR:\n");
> -            else
> -                log("Test " + (i + 1) + ", RTL:\n");
> -            sel.setPosition(tests[i], 0);
> -            moveByWord(sel, tests[i], "right");
> -            moveByWord(sel, tests[i], "left");
> -        }
> -        if (document.getElementById("testMoveByWord"))
> -            document.getElementById("testMoveByWord").style.display = "none";

Please explain why you're removing this chunk of code in the change log.

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