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

[bugzilla-daemon at webkit.org]

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





--- Comment #57 from Alice Cheng <alice_cheng at apple.com>  2012-08-20 18:31:07 PST ---
(From update of attachment 159516)
View in context: https://bugs.webkit.org/attachment.cgi?id=159516&action=review

Thanks for your review =)

>> LayoutTests/editing/selection/user-select-all-selection.html:23
>> +                               " focusNode: " + sel.focusNode + " focusOffset: " + sel.focusOffset);
> 
> Wrong style. + should appear at the beginning of next line, and the text should be indented by exactly 4 spaces to the right of testFailed.

fixed

>> LayoutTests/editing/selection/user-select-all-selection.html:32
>> +                    testSelectionAt(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, "right before user-select-all element");
> 
> userSelectAllElement.previousSibling has a child, then textContent's length is different from that of child node count.
> On the other hand, it is a text node, then calling firstChild doesn't make sense so something is awfully wrong here.

fixed: should be userSelectAllElement.previousSibling.firstChild.textContent.length. userSelectAllElement.previousSibling is a <font> container, whose firstChild is a text node.

>> LayoutTests/editing/selection/user-select-all-selection.html:38
>> +                // test extend forward
> 
> These one line comment just repeats what the code tells us. Please remove.

fixed

>> LayoutTests/editing/selection/user-select-all-selection.html:39
>> +                execSetSelectionCommand(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length, userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.textContent.length);
> 
> !? The offsets should be userSelectAllElement.previousSibling.firstChild.childNodes.length, right? no?
> Also, you can just do:
> getSelection().collapse(userSelectAllElement.previousSibling.firstChild, userSelectAllElement.previousSibling.firstChild.childNodes.length);

fixed. offset is  userSelectAllElement.previousSibling.firstChild.textContent.length

>> LayoutTests/editing/selection/user-select-all-selection.html:40
>> +                execExtendSelectionForwardByCharacterCommand();
> 
> It would be better if this call was done inside evalAndLog so that the expected result contains the line showing this command is called before the assertion.
> Ditto for other test cases.

fixed

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