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

Fri Aug 24 15:36:03 PDT 2012

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

--- Comment #66 from Alice Cheng <alice_cheng at apple.com>  2012-08-24 15:36:00 PST ---
(From update of attachment 159961)
View in context: https://bugs.webkit.org/attachment.cgi?id=159961&action=review

Thanks Ryosuke for your review!

>> 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.

fixed

>> LayoutTests/editing/selection/user-select-all-selection-expected.txt:8
>> +PASS Selection is the entire user-select-all element
> 
> This looks much better!

Thanks! hope this coming version becomes better too.

>> LayoutTests/editing/selection/user-select-all-selection.html:48
>> +                
> 
> Please remove leading whitespace and ditto for other lines.

fixed

>> 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')");

fixed

>> 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'));");

fixed, not exactly the same, but used evalAndLog

>> LayoutTests/editing/selection/user-select-all-selection.html:95
>> +                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)");

fixed, not exactly the same, but logged, and called the same function.

>> 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 =.

fixed

>> LayoutTests/editing/selection/user-select-all-selection.html:114
>> +        </script>
> 
> Why do we need to put this in a separate script element?

tried to move it to <head>, but did not work, because it needs things in <body>?

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