[Webkit-unassigned] [Bug 23607] REGRESSION: Clicking near an contentEditable div will focus the div, even though it shouldn't!

[bugzilla-daemon at webkit.org]

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


hyatt at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #27371|review?                     |review-
               Flag|                            |




------- Comment #18 from hyatt at apple.com  2009-02-10 17:48 PDT -------
(From update of attachment 27371)
> --- 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.


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