[webkit-reviews] review denied: [Bug 23607] REGRESSION: Clicking near an contentEditable div will focus the div, even though it shouldn't! : [Attachment 27371] Fix positionForCoordinates now with an added fix for clicking in the margins of the body.

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 23607: REGRESSION: Clicking near an contentEditable div will focus the div,
even though it shouldn't!
https://bugs.webkit.org/show_bug.cgi?id=23607

Attachment 27371: Fix positionForCoordinates now with an added fix for clicking
in the margins of the body.
https://bugs.webkit.org/attachment.cgi?id=27371&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
> --- a/WebCore/editing/VisiblePosition.cpp
> +++ b/WebCore/editing/VisiblePosition.cpp
> @@ -478,7 +478,7 @@ Position VisiblePosition::canonicalPosition(const
Position& position)
>  
>      // The new position must be in the same editable element. Enforce that
first.
>      // Unless the descent is from a non-editable html element to an editable
body.
> -    if (node->hasTagName(htmlTag) && !node->isContentEditable())
> +    if (node->hasTagName(htmlTag) && !node->isContentEditable() &&
node->ownerDocument()->body()->isContentEditable())
>	   return next.isNotNull() ? next : prev;
>  

I think body() can be null in certain circumstances but am not positive.


> +// FIXME: This function should could go on RenderObject an instance method
then
> +// all cases in which positionForCoordinates recurses could call this
instead to
> +// prevent crossing editable boundaries.  This would require many tests!

Can you clean up the comment?  "should could" and did you mean "as an instance
method"?  I think "recurs" is easier to read
than "recurses".  Same for other places where you say "recurse" (I'd just say
"recur.")

> +static VisiblePosition
positionForPointRespectingEditingBoundaries(RenderBox* parent, RenderBox*
child, const IntPoint& parentCoords)
> +{
> +    int xInChildCoords = parentCoords.x() - child->x();
> +    int yInChildCoords = parentCoords.y() - child->y();
> +
> +    // If this is an anonymous renderer, we just recurse normally
> +    Node* childNode = child->element();
> +    if (!childNode)
> +	   return child->positionForCoordinates(xInChildCoords,
yInChildCoords);
> +
> +    // Otherwise, first make sure that the editability of the parent and
child agree!

I don't think the ! is necessary here. I mean, it's just editing code. It's not
*that* exciting.

> +    // If they don't agree, then we return a visible position just before or
after the child
> +    RenderObject* ancestor = parent;
> +    while (ancestor && !ancestor->element())
> +	   ancestor = ancestor->parent();
> +    
> +    // If we can't find an ancestor to check editability on, or editibility
is unchanged, we recurse like normal

Typo. "editibility" should be "editability"

> +VisiblePosition positionForPointWithInlineChildren(RenderBlock* block, const
IntPoint& pointInContents, int xInParentCoords)

Should be static.

> +{
> +    ASSERT(block->childrenInline());
> +
> +    if (!block->firstRootBox())
> +	   return VisiblePosition(block->element(), 0, DOWNSTREAM);
> +
> +    // look for the closest line box in the root box which is at the
passed-in y coordinate
> +    for (RootInlineBox* root = block->firstRootBox(); root; root =
root->nextRootBox()) {
> +	   int bottom;
> +	   // set the bottom based on whether there is a next root box
> +	   if (root->nextRootBox())
> +	       // FIXME: make the break point halfway between the bottom of the
previous root box and the top of the next root box
> +	       bottom = root->nextRootBox()->topOverflow();
> +	   else
> +	       bottom = root->bottomOverflow() + verticalLineClickFudgeFactor;
> +	   // check if this root line box is located at this y coordinate
> +	   if (pointInContents.y() < bottom && root->firstChild()) {
> +	       // FIXME: It seems like a bug that this function needs
xInParentCoords
> +	       InlineBox* closestBox =
root->closestLeafChildForXPos(xInParentCoords);

It does indeed seem like a bug at first glance.  Please make sure to split out
a followup bug on this issue.


> diff --git a/WebCore/rendering/RenderInline.cpp
b/WebCore/rendering/RenderInline.cpp
> index 8f09fc7..e8ee55d 100644
> --- a/WebCore/rendering/RenderInline.cpp
> +++ b/WebCore/rendering/RenderInline.cpp
> @@ -485,6 +485,7 @@ VisiblePosition RenderInline::positionForCoordinates(int
x, int y)
>      while (c) {
>	   RenderBox* contBlock = c;
>	   if (c->isInline() || c->firstChild())
> +	       // The continuations (anonymous blocks) will never be editable,
so we don't need to worry about crossing editable boundaries here.
>	       return c->positionForCoordinates(parentBlockX - contBlock->x(),
parentBlockY - contBlock->y());
>	   c = toRenderBlock(c)->inlineContinuation();
>      }

Just revert this change please.  I think the comment here is more confusing
than helpful.