[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