[Webkit-unassigned] [Bug 91912] Adding "all" to -webkit-user-select
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Oct 31 23:29:14 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=91912
--- Comment #91 from Enrica Casucci <enrica at apple.com> 2012-10-31 23:30:33 PST ---
(In reply to comment #90)
> (In reply to comment #88)
> > (In reply to comment #87)
> > > (In reply to comment #85)
> > > > (From update of attachment 171744 [details] [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.
You are correct about this.
> >
> > >>> 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