[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 #52 from Ryosuke Niwa <rniwa at webkit.org>  2012-08-20 14:27:42 PST ---
(From update of attachment 159516)
View in context: https://bugs.webkit.org/attachment.cgi?id=159516&action=review

The code change looks great to me. I've pointed out some nits. I apply the patch locally and play around to make sure it works as intended.

> Source/WebCore/dom/Position.cpp:874
> +    return nodeIsUserSelectAll(node) && node->parentNode() && !nodeIsUserSelectAll(node->parentNode());

You should probably check that
nodeIsUserSelectAll(node) && (!node->parentNode() || !nodeIsUserSelectAll(node->parentNode()));

> Source/WebCore/dom/Position.cpp:883
> +    Node* parent = node->parentNode();
> +    if (!parent)
> +        return node;

Then you don't need to special case this.

> Source/WebCore/page/EventHandler.cpp:831
> +        // reset base for user select all when necessary
> +        if (rootUserSelectAllForMousePressNode && comparePositions(target->renderer()->positionForPoint(hitTestResult.localPoint()), m_mousePressNode->renderer()->positionForPoint(m_dragStartPos)) < 0)

Instead of adding a comment like this, can we extract this long condition as a helper function with a descriptive name?
e.g. shouldResetBaseForUserSelectAll.
By the way, a more useful comment is to describe what "when necessary" exactly is.

> LayoutTests/editing/selection/user-select-all-selection.html:23
> +                    testFailed("Selection should be " + str +
> +                               " at anchorNode: " + sel.anchorNode + " anchorOffset: " + sel.anchorOffset +
> +                               " 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.

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

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

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

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

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

Ditto about calling this in evalAndLog.

> LayoutTests/editing/selection/user-select-all-selection.html:83
> +                eventSender.mouseDown();
> +                eventSender.mouseMoveTo(leftTarget.offsetLeft, leftTarget.offsetTop + 10);
> +                eventSender.mouseUp();

Ditto. You may consider adding a helper function like moveFromTo that takes two coordinate points.

> LayoutTests/editing/selection/user-select-all-selection.html:92
> +                eventSender.mouseMoveTo(clickTarget.offsetLeft + 10, clickTarget.offsetTop + 10);
> +                eventSender.mouseDown();
> +                eventSender.mouseMoveTo(userSelectAllElement.offsetLeft + userSelectAllElement.offsetWidth +rightTarget.offsetWidth, rightTarget.offsetTop + 10);
> +                eventSender.mouseUp();

Ditto.

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