[Webkit-unassigned] [Bug 60529] Programmatically set selection should not have direction on Mac

Mon Jul 11 12:36:30 PDT 2011

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

--- Comment #4 from Ryosuke Niwa <rniwa at webkit.org>  2011-07-11 12:36:30 PST ---
(From update of attachment 100347)
View in context: https://bugs.webkit.org/attachment.cgi?id=100347&action=review

It's nice that the code change is exactly what I had anticipated but I'd like to see tests for undo/redo.  When a user editing action or execCommand is undone or redone, selection restored by WebKit should be directionless on Mac.  Similarly, you should run the test cases you have on input/textarea.

> LayoutTests/editing/selection/correct-selection-direction-for-platform-expected.txt:1
> +This test ensures that the direction of a programmatically set selection is correct depending on the platform. On mac, a programmatic selection should have no default direction, while on all other platforms, the direction should be forward. The expected results for this test are platform independent, due to the use of SetEditingBehaviour.

"correct" seems too vague.  You should say the programming set selection is directionless on and only on Mac.  Also, "direction" is a over-loaded word in WebKit because it usually refers to the text direction (ltr/rtl) but can also refer to writing mode such as vertical-lr.

Saying that the expected results for this platform are independent is unnecessary and should be removed.

> LayoutTests/editing/selection/correct-selection-direction-for-platform-expected.txt:6
> +PASS selection is ' second', with base: 30, and extent: 23.

This output isn't so useful because it doesn't tell me what action/function call made this selection.

> LayoutTests/editing/selection/correct-selection-direction-for-platform.html:1
> +<!doctype html>

While HTML5 spec says it's case-insensitive, I'd prefer to have <!DOCTYPE html>

> LayoutTests/editing/selection/correct-selection-direction-for-platform.html:15
> +    s = window.getSelection();

Please avoid using one-letter variable.

> LayoutTests/editing/selection/correct-selection-direction-for-platform.html:22
> +function setup(testContainer, start, end, result) {

Why does this function take "result" and never use it?

> LayoutTests/editing/selection/correct-selection-direction-for-platform.html:46
> +    setup(testContainer, 24, 30, 'second');
> +    test('extend', 'left', 'character', 30, 23, ' second');

I would have combined setup and test.

A general pattern I used in these tests is that I'll have an "actor" closure that sets up selection & calls Selection.modify.  It then returns the string explaining what it did.  The test function then compares the current selection to the expected results and prints out the pass/fail along with the string returned by the "actor".

> LayoutTests/editing/selection/correct-selection-direction-for-platform.html:87
> +        setup(testContainer, 24, 30, 'second');
> +        test('extend', 'left', 'character', 24, 29, 'secon');
> +        test('extend', 'right', 'character', 24, 30, 'second');
> +        setup(testContainer, 24, 30, 'second');
> +        test('extend', 'left', 'word', 24, 24, '');
> +        test('extend', 'right', 'word', 24, 30, 'second');
> +        setup(testContainer, 24, 30, 'second');
> +        test('extend', 'left', 'sentence', 24, 20, 'The ');
> +        test('extend', 'right', 'sentence', 24, 41, 'second sentence. ');
> +    
> +        setup(testContainer, 24, 30, 'second');
> +        test('extend', 'right', 'character', 24, 31, 'second ');
> +        test('extend', 'left', 'character', 24, 30, 'second');
> +        setup(testContainer, 24, 30, 'second');
> +        test('extend', 'right', 'word', 24, 39, 'second sentence');
> +        test('extend', 'left', 'word', 24, 31, 'second ');
> +        setup(testContainer, 24, 30, 'second');
> +        test('extend', 'right', 'sentence', 24, 41, 'second sentence. ');
> +        test('extend', 'left', 'sentence', 24, 20, 'The ');

I would have made this a function, and just called it twice after calling setEditingBehavior as in:
setEditingBehavior('win');
runTest();
setEditingBehavior('linux');
runTest();

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