[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 #88 from Ryosuke Niwa <rniwa at webkit.org>  2012-10-31 22:41:11 PST ---
(In reply to comment #87)
> (In reply to comment #85)
> > (From update of attachment 171744 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=171744&action=review
> >
> > > 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.

Because nodeIsUserSelectAll checks that:
node && node->renderer() && node->renderer()->style()->userSelect() == SELECT_ALL
but neither br nor its parent div has renderer this in this case. So nodeIsUserSelectAll() will return false for those two nodes and stops the loop.

I think we need a loop like:
while (node && !node->renderer())
    node = node->parentNode();
at the very beginning to skip all non-visible nodes first. Once we hit some node with a render object, we can start looking at its style.

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

But we call downstream whenever we call positionAfterNode and we call upstream whenever we call positionBeforeNode in both of these functions.

>>> Source/WebCore/page/EventHandler.cpp:859
>>> +        newSelection.setExtent(positionAfterNode(rootUserSelectAllForMousePressNode).downstream(CanCrossEditingBoundary));
>> 
>> Ditto about re-using adjustForwardPositionForUserSelectAll. Maybe adjustForwardPositionForUserSelectAll should take root user select node as an argument?
> 
> 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.

If we merged the two functions and just had adjustToPositionAfterNode as the argument, we can call it twice here as in:
newSelection.setBase(adjustPositionForUserSelectAll(rootUserSelectAllForMousePressNode, AdjustToPositionBeforeNode));
newSelection.setExtent(adjustPositionForUserSelectAll(rootUserSelectAllForMousePressNode, AdjustToPositionAfterNode));

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