[Webkit-unassigned] [Bug 48037] Triple click does not select whole line for mixed contenteditable regions

[bugzilla-daemon at webkit.org]

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





--- Comment #20 from Ryosuke Niwa <rniwa at webkit.org>  2010-10-26 00:01:42 PST ---
(From update of attachment 71839)
View in context: https://bugs.webkit.org/attachment.cgi?id=71839&action=review

The patch looks much better now.  We probably want some reviewers come in & take a look at your patch once the problems I have addressed here have been fixed.

>>> LayoutTests/editing/selection/triple-click-across-editability-boundaries.html:58
>>> +    eventSender.mouseUp();
>> 
>> This might be too fast.  You may need to add some delays between each downs & ups.
> 
> Yeah I copied it from the other triple click test, seems to work?

Mn... could you verify that the other test hasn't been flaky?

>>> WebCore/editing/visible_units.cpp:817
>>> +        // We also do this in endOfParagraph, and might want to do it in other cases too.
>> 
>> Why FIXME?  Isn't new code correct?  I don't think we should add FIXME to indicate that we should fix other places.  And I note that there are other places in editing where we skip empty text nodes.  If you've found other code that may require skipping empty text nodes, then you should add FIXME to those places and not here.
> 
> Just copied the existing FIXME -- I don't know why that FIXME was added (http://trac.webkit.org/changeset/8786).
> I took the "such as startOfParagraph" to mean that there might be other cases too besides startOfParagraph, but I will delete it if you want.  I'll definitely delete the one here though.

That's very mysterious.  Darin might be able to elaborate on this issue since he reviewed the patch (r8786).

>>> WebCore/editing/visible_units.h:37
>>> +enum EditingBoundaryCrossingRule { CanCrossEditingBoundary, CannotCrossEditingBoundary };
>> 
>> Mn... you probably don't want to redefine enum here.
> 
> Yeah I was wondering about that.  So Position::EditingBoundaryCrossingRule etc everywhere?  Or define a new similarly-named enum?

You probably want to do Positon::EditingBoundaryCrossingRule everywhere or promote EditingBoundaryCrossingRule into WebCore namespace in Position.h.  Regardless, We shouldn't be defining two identical enums in Position and VisiblePosition.

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