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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 31 23:28:10 PDT 2012


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





--- Comment #90 from Enrica Casucci <enrica at apple.com>  2012-10-31 23:29:27 PST ---
(In reply to comment #88)
> (In reply to comment #87)
> > (In reply to comment #85)
> > > (From update of attachment 171744 [details] [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));
I will do that to stop all this. I don't believe this is necessary, but we have spent way too much time on this patch already.

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


More information about the webkit-unassigned mailing list