[Webkit-unassigned] [Bug 91912] Adding "all" to -webkit-user-select

Thu Aug 23 23:43:47 PDT 2012

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

--- Comment #65 from Ryosuke Niwa (vacation: 8/26-9/10) <rniwa at webkit.org>  2012-08-23 23:43:44 PST ---
(From update of attachment 159961)
View in context: https://bugs.webkit.org/attachment.cgi?id=159961&action=review

Sorry more nits.

> Source/WebCore/page/EventHandler.cpp:830
> +        // reset base for user select all when base is inside user-select-all area and extent < base.

Nit: Capitalize R.

> LayoutTests/editing/selection/user-select-all-selection-expected.txt:8
> +window.getSelection().modify('extend', 'forward', 'character')
> +PASS Selection is the entire user-select-all element

This looks much better!

> LayoutTests/editing/selection/user-select-all-selection.html:48
> +                

Please remove leading whitespace and ditto for other lines.

> LayoutTests/editing/selection/user-select-all-selection.html:57
> +                execSetSelectionCommand(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length);

Maybe you can create a helper function like placeCaretInsideUserSelectAllElement and call it inside evalAndLog as in:
evalAndLog("placeCaretInsideUserSelectAllElement(); getSelection().modify('extend', 'forward', 'character')");

> LayoutTests/editing/selection/user-select-all-selection.html:61
> +                evalAndLog("window.getSelection().modify('extend', 'backward', 'character')");

Shouldn't we reset the selection using execSetSelectionCommand?

> LayoutTests/editing/selection/user-select-all-selection.html:83
> +                // test click

This comment is probably not that helpful.

> LayoutTests/editing/selection/user-select-all-selection.html:87
> +                clickAt(clickTarget.offsetLeft + 10 , clickTarget.offsetTop + 10);

Maybe we can make this function take an element instead of x & y so that you can do:
evalAndLog("click(document.getElementById('elementInsideUserSelectAll'));");

> LayoutTests/editing/selection/user-select-all-selection.html:88
> +                log("clicking on descendent of user-select-all element");

Then you wouldn't need this description.

> LayoutTests/editing/selection/user-select-all-selection.html:95
> +                mouseMoveFromTo(leftTarget.offsetLeft, leftTarget.offsetTop + 10, clickTarget.offsetLeft + 20 , clickTarget.offsetTop + 10);
> +                log("mouse extending from left to user-select-all element");

Here, you can pass in two x coordinates as in:
evalAndLog("moveMouseInElement(userSelectAllElement.offsetTop + 10, userSelectAllElement.previousSibling.offsetLeft, document.getElementById('elementInsideUserSelectAll').offsetLeft + 10)");

> LayoutTests/editing/selection/user-select-all-selection.html:102
> +                var rightTargetTextLength = rightTarget.firstChild.textContent.length;
> +                mouseMoveFromTo(userSelectAllElement.offsetLeft + userSelectAllElement.offsetWidth +rightTarget.offsetWidth, rightTarget.offsetTop + 10, clickTarget.offsetLeft + 10, clickTarget.offsetTop + 10);

Ditto.

> LayoutTests/editing/selection/user-select-all-selection.html:108
> +<body contentEditable="true" id = "body"><font>Test -webkit-user-select all</font><font class="userSelectAll" id ="allArea"><font id = "descendent">user select all area</font>user select all area</font><font>Test -webkit-user-select all</font>

Please be consistent about spaces around =. In most tests, we omit spaces around =.

> LayoutTests/editing/selection/user-select-all-selection.html:114
> +        <script>
> +            description(" Test -webkit-user-select all selection movements and extensions (left right forward backward) ");
> +            testKeyboard();
> +            testMouse();
> +        </script>

Why do we need to put this in a separate script element?

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