[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 #85 from Ryosuke Niwa <rniwa at webkit.org>  2012-10-31 17:46:39 PST ---
(From update of attachment 171744)
View in context: https://bugs.webkit.org/attachment.cgi?id=171744&action=review

> Source/WebCore/dom/Node.cpp:753
> +            // Elements with user-select: all style are considered atomic
> +            // therefore non editable.

It seems like this can fit in one line.

> Source/WebCore/dom/Node.cpp:794
> +    return isContentEditable(Node::UserSelectAllIsAlwaysNonEditable);

We’re already in Node. We don’t need to qualify it with Node, right?

> Source/WebCore/dom/Node.cpp:2755
> +    return isContentEditable(Node::UserSelectAllIsAlwaysNonEditable) || hasEventListeners(eventNames().mouseupEvent) || hasEventListeners(eventNames().mousedownEvent) || hasEventListeners(eventNames().clickEvent) || hasEventListeners(eventNames().DOMActivateEvent);

Ditto about Node::.

> Source/WebCore/dom/Position.cpp:881
> +    

Whitespace here.

> Source/WebCore/dom/Position.cpp:883
> +    while (parent && nodeIsUserSelectAll(parent)) {

Don’t we need to skip the display:none nodes? e.g. <div style="-webkit-user-select: all;">hello<span style="display: none;"><br></span>world</div>.
If we started the search from br, we still want to detect the root node to be the div, right?

> Source/WebCore/editing/FrameSelection.cpp:560
> +static void adjustForwardPositionForUserSelectAll(VisiblePosition& pos, bool directionIsLTR)

Should these two functions be combined and take isForward?
We can then do directionIsLTR == isForward ? positionAfter… : positionBefore…;
Or, we can rename the argument to be something like “adjustToPositionAfterNode”.

> Source/WebCore/editing/FrameSelection.cpp:563
> +        pos = VisiblePosition((directionIsLTR) ? positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary) : positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));

No need for parentheses around directionIsLTR.

> Source/WebCore/editing/FrameSelection.cpp:570
> +        pos = VisiblePosition((directionIsLTR) ? positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary) : positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary));

Ditto.

> Source/WebCore/page/EventHandler.cpp:424
> +    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(targetNode);
> +    if (rootUserSelectAll) {

We can declare rootUserSelectAll in the if statement.

> Source/WebCore/page/EventHandler.cpp:425
> +        selection.setBase(positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));

Why don’t we call adjustForwardPositionForUserSelectAll here to share more code?

> Source/WebCore/page/EventHandler.cpp:426
> +        selection.setExtent(positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary));

Ditto.

> Source/WebCore/page/EventHandler.cpp:859
> +        newSelection.setBase(positionBeforeNode(rootUserSelectAllForMousePressNode).upstream(CanCrossEditingBoundary));
> +        newSelection.setExtent(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary));

Ditto about re-using adjustForwardPositionForUserSelectAll. Maybe adjustForwardPositionForUserSelectAll should take root user select node as an argument?

> Source/WebCore/page/EventHandler.cpp:862
> +        if (rootUserSelectAllForMousePressNode && comparePositions(target->renderer()->positionForPoint(hitTestResult.localPoint()), m_mousePressNode->renderer()->positionForPoint(m_dragStartPos)) < 0)

Is target guaranteed to have renderer?

> Source/WebCore/page/EventHandler.cpp:863
> +            newSelection.setBase(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary));

We should probably re-use adjustForwardPositionForUserSelectAll here as well.

> Source/WebCore/page/EventHandler.cpp:866
> +        if (rootUserSelectAllForTarget && m_mousePressNode->renderer() && comparePositions(target->renderer()->positionForPoint(hitTestResult.localPoint()), m_mousePressNode->renderer()->positionForPoint(m_dragStartPos)) < 0)

Ditto about target having renderer.

> Source/WebCore/page/EventHandler.cpp:869
> +            newSelection.setExtent(positionBeforeNode(rootUserSelectAllForTarget).upstream(CanCrossEditingBoundary));
> +        else if (rootUserSelectAllForTarget && m_mousePressNode->renderer())
> +            newSelection.setExtent(positionAfterNode(rootUserSelectAllForTarget).downstream(CanCrossEditingBoundary));

Ditto about re-using adjustForwardPositionForUserSelectAll.

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