[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 #87 from Enrica Casucci <enrica at apple.com>  2012-10-31 22:19:54 PST ---
(In reply to comment #85)
> (From update of attachment 171744 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171744&action=review
> It seems like this can fit in one line.
We normally mark this comments as nits.
> 
> > 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?
Yes, and why is this not the case? We are looking for the root element with user-select and in your example this code still finds div.

> 
> > 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”.
No, I don't think we need to do that. The difference is also in the use of downstream and upstream.

> 
> No need for parentheses around directionIsLTR.
Sure...
> 
> > Source/WebCore/page/EventHandler.cpp:424
> > +    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(targetNode);
> > +    if (rootUserSelectAll) {
> 
> We can declare rootUserSelectAll in the if statement.
Yes.
> 
> > 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.

No, the other functions take into account the text direction that we don't need to take into account here.
I don't believe there is any need to change this.

> 
> > 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.
I don't understand this comment at all.
> 
> > 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.
Again, I disagree for the reason above.

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