Bug 23607 - REGRESSION: Clicking near an contentEditable div will focus the div, even though it shouldn't!
Summary: REGRESSION: Clicking near an contentEditable div will focus the div, even tho...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 23605 (view as bug list)
Depends on:
Blocks: 23605
  Show dependency treegraph
 
Reported: 2009-01-28 16:15 PST by Eric Seidel (no email)
Modified: 2009-02-24 14:28 PST (History)
3 users (show)

See Also:


Attachments
js portion of test case (1.41 KB, application/x-javascript)
2009-01-28 16:16 PST, Eric Seidel (no email)
no flags Details
First pass testcases for bug 23605 and bug 23607 (9.83 KB, patch)
2009-01-30 14:50 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
First pass fix, Position::cannonicalPosition still seems to break us (9.23 KB, patch)
2009-01-30 14:50 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix bugs 23605 and 23607 (37.54 KB, patch)
2009-01-30 17:54 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix bugs 23605 and 23607 (37.58 KB, patch)
2009-01-30 18:26 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix bugs 23605 and 23607 (37.58 KB, patch)
2009-01-30 22:12 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix bugs 23605 and 23607 (39.88 KB, patch)
2009-01-30 22:14 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
A full git patch for easier applying by ojan (with git am) (57.28 KB, patch)
2009-02-02 17:58 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix positionForCoordinates now with an added fix for clicking in the margins of the body. (41.54 KB, patch)
2009-02-05 17:42 PST, Ojan Vafai
hyatt: review-
Details | Formatted Diff | Diff
Fix for clicking below the last root line box with multiple line boxes. (45.83 KB, patch)
2009-02-12 16:02 PST, Ojan Vafai
hyatt: review+
Details | Formatted Diff | Diff
Same patch, but synced to trunk for easier landing by eseidel. (45.72 KB, patch)
2009-02-24 13:25 PST, Ojan Vafai
no flags Details | Formatted Diff | Diff
patch made with git format-patch for even easier patching by eseidel. :) (48.99 KB, text/plain)
2009-02-24 13:32 PST, Ojan Vafai
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-01-28 16:15:13 PST
Clicking near an contentEditable div will focus the div, even though it shouldn't!

This is especially noticeable if the div is the only content in the page (as any click on the body will focus the div).  FF and IE get this right.

This blocks conversion of text areas away from shadow nodes.
Comment 1 Eric Seidel (no email) 2009-01-28 16:16:01 PST
Created attachment 27133 [details]
js portion of test case
Comment 2 Eric Seidel (no email) 2009-01-30 14:50:08 PST
Created attachment 27196 [details]
First pass testcases for bug 23605 and bug 23607

 ...ick-in-margins-inside-editable-div-expected.txt |    9 ++
 .../click-in-margins-inside-editable-div.html      |   13 ++
 .../click-outside-editable-div-expected.txt        |   15 ++
 .../selection/click-outside-editable-div.html      |   13 ++
 .../editing/selection/resources/TEMPLATE.html      |   13 ++
 .../click-in-margins-inside-editable-div.js        |  135 ++++++++++++++++++++
 .../resources/click-outside-editable-div.js        |   50 +++++++
 7 files changed, 248 insertions(+), 0 deletions(-)
Comment 3 Eric Seidel (no email) 2009-01-30 14:50:09 PST
Created attachment 27197 [details]
First pass fix, Position::cannonicalPosition still seems to break us

 WebCore/rendering/RenderBlock.cpp     |  112 +++++++++++++++------------------
 WebCore/rendering/RenderContainer.cpp |    3 +-
 WebCore/rendering/RenderInline.cpp    |    1 +
 3 files changed, 54 insertions(+), 62 deletions(-)
Comment 4 Eric Seidel (no email) 2009-01-30 14:53:26 PST
I've attached our first stab.  It's broken.  We're now correctly returning a good VisualPosition right before the editable div, but canonicalPosition is turning our returned position into the next div.  I'm not sure what changes we need to make to canonicalPosition to make this work.  Should our placement be a "candidate position"?  Should we special case to check if next's editibitility is not the same as position's?  (That's probably the fix.)

Here's the function for reference:  In our case, we fall out the last return where it does return next;

Position VisiblePosition::canonicalPosition(const Position& position)
{
    // FIXME (9535):  Canonicalizing to the leftmost candidate means that if we're at a line wrap, we will 
    // ask renderers to paint downstream carets for other renderers.
    // To fix this, we need to either a) add code to all paintCarets to pass the responsibility off to
    // the appropriate renderer for VisiblePosition's like these, or b) canonicalize to the rightmost candidate
    // unless the affinity is upstream.
    Node* node = position.node();
    if (!node)
        return Position();

    node->document()->updateLayoutIgnorePendingStylesheets();

    Position candidate = position.upstream();
    if (candidate.isCandidate())
        return candidate;
    candidate = position.downstream();
    if (candidate.isCandidate())
        return candidate;

    // When neither upstream or downstream gets us to a candidate (upstream/downstream won't leave 
    // blocks or enter new ones), we search forward and backward until we find one.
    Position next = canonicalizeCandidate(nextCandidate(position));
    Position prev = canonicalizeCandidate(previousCandidate(position));
    Node* nextNode = next.node();
    Node* prevNode = prev.node();

    // 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())
        return next.isNotNull() ? next : prev;

    Node* editingRoot = editableRootForPosition(position);
        
    // If the html element is editable, descending into its body will look like a descent 
    // from non-editable to editable content since rootEditableElement() always stops at the body.
    if (editingRoot && editingRoot->hasTagName(htmlTag) || position.node()->isDocumentNode())
        return next.isNotNull() ? next : prev;
        
    bool prevIsInSameEditableElement = prevNode && editableRootForPosition(prev) == editingRoot;
    bool nextIsInSameEditableElement = nextNode && editableRootForPosition(next) == editingRoot;
    if (prevIsInSameEditableElement && !nextIsInSameEditableElement)
        return prev;
        
    if (nextIsInSameEditableElement && !prevIsInSameEditableElement)
        return next;
        
    if (!nextIsInSameEditableElement && !prevIsInSameEditableElement)
        return Position();

    // The new position should be in the same block flow element. Favor that.
    Node *originalBlock = node->enclosingBlockFlowElement();
    bool nextIsOutsideOriginalBlock = !nextNode->isDescendantOf(originalBlock) && nextNode != originalBlock;
    bool prevIsOutsideOriginalBlock = !prevNode->isDescendantOf(originalBlock) && prevNode != originalBlock;
    if (nextIsOutsideOriginalBlock && !prevIsOutsideOriginalBlock)
        return prev;
        
    return next;
}
Comment 5 Eric Seidel (no email) 2009-01-30 14:53:55 PST
I'm very interested in Justin's comment here.
Comment 6 Eric Seidel (no email) 2009-01-30 15:01:45 PST
I was wrong!  We hit this line:
    if (!nextIsInSameEditableElement && !prevIsInSameEditableElement)
        return Position();

We might just change that to return "position" instead.
Comment 7 Eric Seidel (no email) 2009-01-30 15:10:24 PST
Yay!  Now we're crashing at:
            if (previous.isNull()) {
                ASSERT_NOT_REACHED();

in Selection::adjustForEditableContent..  sigh.
Comment 8 Eric Seidel (no email) 2009-01-30 16:33:07 PST
Ha!  This is a regression:
http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/editing/selection/contenteditable-click-outside-expected.png

We already had a test which was failing!
Comment 9 Eric Seidel (no email) 2009-01-30 17:54:30 PST
Created attachment 27204 [details]
Fix bugs 23605 and 23607

 LayoutTests/ChangeLog                              |   33 +++++
 ...ick-in-margins-inside-editable-div-expected.txt |   23 ++++
 .../click-in-margins-inside-editable-div.html      |   13 ++
 .../click-outside-editable-div-expected.txt        |   17 +++
 .../selection/click-outside-editable-div.html      |   13 ++
 .../selection/contenteditable-click-outside.html   |   13 --
 .../editing/selection/resources/TEMPLATE.html      |   13 ++
 .../click-in-margins-inside-editable-div.js        |  138 ++++++++++++++++++++
 .../resources/click-outside-editable-div.js        |   50 +++++++
 ...contenteditable-click-outside-expected.checksum |    1 -
 .../contenteditable-click-outside-expected.png     |  Bin 21262 -> 0 bytes
 .../contenteditable-click-outside-expected.txt     |   16 ---
 .../selection/select-all-iframe-expected.txt       |    4 +-
 .../select-from-textfield-outwards-expected.txt    |    2 +-
 .../contenteditable-click-outside-expected.txt     |   12 --
 .../selection/select-all-iframe-expected.txt       |   56 --------
 WebCore/ChangeLog                                  |   28 ++++
 WebCore/editing/VisiblePosition.cpp                |    4 +-
 WebCore/rendering/RenderBlock.cpp                  |  115 +++++++---------
 WebCore/rendering/RenderContainer.cpp              |    3 +-
 WebCore/rendering/RenderInline.cpp                 |    1 +
 21 files changed, 385 insertions(+), 170 deletions(-)
Comment 10 Eric Seidel (no email) 2009-01-30 17:56:27 PST
The editing delegate changes are not well understood, but I think they're progressions.  Perhaps when Justing reviews this he can comment on if they look better to him too! :)
Comment 11 Eric Seidel (no email) 2009-01-30 18:26:21 PST
Created attachment 27207 [details]
Fix bugs 23605 and 23607

 LayoutTests/ChangeLog                              |   33 +++++
 ...ick-in-margins-inside-editable-div-expected.txt |   23 ++++
 .../click-in-margins-inside-editable-div.html      |   13 ++
 .../click-outside-editable-div-expected.txt        |   17 +++
 .../selection/click-outside-editable-div.html      |   13 ++
 .../selection/contenteditable-click-outside.html   |   13 --
 .../editing/selection/resources/TEMPLATE.html      |   13 ++
 .../click-in-margins-inside-editable-div.js        |  138 ++++++++++++++++++++
 .../resources/click-outside-editable-div.js        |   50 +++++++
 ...contenteditable-click-outside-expected.checksum |    1 -
 .../contenteditable-click-outside-expected.png     |  Bin 21262 -> 0 bytes
 .../contenteditable-click-outside-expected.txt     |   16 ---
 .../selection/select-all-iframe-expected.txt       |    4 +-
 .../select-from-textfield-outwards-expected.txt    |    2 +-
 .../contenteditable-click-outside-expected.txt     |   12 --
 .../selection/select-all-iframe-expected.txt       |   56 --------
 WebCore/ChangeLog                                  |   28 ++++
 WebCore/editing/VisiblePosition.cpp                |    4 +-
 WebCore/rendering/RenderBlock.cpp                  |  113 +++++++---------
 WebCore/rendering/RenderContainer.cpp              |    3 +-
 WebCore/rendering/RenderInline.cpp                 |    1 +
 21 files changed, 383 insertions(+), 170 deletions(-)
Comment 12 Eric Seidel (no email) 2009-01-30 22:12:58 PST
Created attachment 27210 [details]
Fix bugs 23605 and 23607

 LayoutTests/ChangeLog                              |   33 +++++
 ...ick-in-margins-inside-editable-div-expected.txt |   23 ++++
 .../click-in-margins-inside-editable-div.html      |   13 ++
 .../click-outside-editable-div-expected.txt        |   17 +++
 .../selection/click-outside-editable-div.html      |   13 ++
 .../selection/contenteditable-click-outside.html   |   13 --
 .../editing/selection/resources/TEMPLATE.html      |   13 ++
 .../click-in-margins-inside-editable-div.js        |  138 ++++++++++++++++++++
 .../resources/click-outside-editable-div.js        |   50 +++++++
 ...contenteditable-click-outside-expected.checksum |    1 -
 .../contenteditable-click-outside-expected.png     |  Bin 21262 -> 0 bytes
 .../contenteditable-click-outside-expected.txt     |   16 ---
 .../selection/select-all-iframe-expected.txt       |    4 +-
 .../select-from-textfield-outwards-expected.txt    |    2 +-
 .../contenteditable-click-outside-expected.txt     |   12 --
 .../selection/select-all-iframe-expected.txt       |   56 --------
 WebCore/ChangeLog                                  |   28 ++++
 WebCore/editing/VisiblePosition.cpp                |    4 +-
 WebCore/rendering/RenderBlock.cpp                  |  113 +++++++---------
 WebCore/rendering/RenderContainer.cpp              |    3 +-
 WebCore/rendering/RenderInline.cpp                 |    1 +
 21 files changed, 383 insertions(+), 170 deletions(-)
Comment 13 Eric Seidel (no email) 2009-01-30 22:14:59 PST
Created attachment 27211 [details]
Fix bugs 23605 and 23607

 LayoutTests/ChangeLog                              |   33 ++++
 ...ick-in-margins-inside-editable-div-expected.txt |   23 +++
 .../click-in-margins-inside-editable-div.html      |   13 ++
 .../click-outside-editable-div-expected.txt        |   17 ++
 .../selection/click-outside-editable-div.html      |   13 ++
 .../selection/contenteditable-click-outside.html   |   13 --
 .../editing/selection/resources/TEMPLATE.html      |   13 ++
 .../click-in-margins-inside-editable-div.js        |  138 ++++++++++++++++
 .../resources/click-outside-editable-div.js        |   50 ++++++
 ...contenteditable-click-outside-expected.checksum |    1 -
 .../contenteditable-click-outside-expected.png     |  Bin 21262 -> 0 bytes
 .../contenteditable-click-outside-expected.txt     |   16 --
 .../selection/select-all-iframe-expected.txt       |    4 +-
 .../select-from-textfield-outwards-expected.txt    |    2 +-
 .../contenteditable-click-outside-expected.txt     |   12 --
 .../selection/select-all-iframe-expected.txt       |   56 -------
 WebCore/ChangeLog                                  |   31 ++++
 WebCore/editing/VisiblePosition.cpp                |    4 +-
 WebCore/rendering/RenderBlock.cpp                  |  174 +++++++++-----------
 WebCore/rendering/RenderContainer.cpp              |    3 +-
 WebCore/rendering/RenderInline.cpp                 |    1 +
 21 files changed, 419 insertions(+), 198 deletions(-)
Comment 14 Justin Garcia 2009-02-02 17:35:08 PST
> The editing delegate changes are not well understood

The change in:

/LayoutTests/platform/mac/editing/selection/select-all-iframe-expected.txt

comes from your fix for 23605 (the x position of the caret that's created is close to where the mouseDown occurred now instead of after the line of text).  So that's fine.

But, the change in:

/LayoutTests/platform/mac/editing/selection/select-from-textfield-outwards-expected.txt

leads me to believe that the patch may have caused a regression in behavior when you click inside a text field and drag-select up.  It looks like for a particular position above the textfield, we create the wrong selection.  Check out line 25 of select-from-textfield-outwards:

eventSender.mouseMoveTo(middleX, middleY - field.offsetHeight);

It looks like that used to correspond to this:

EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of #text > DIV to 12 of #text > DIV toDOMRange:range from 0 of #text > DIV to 12 of #text > DIV affinity:NSSelectionAffinityDownstream stillSelecting:FALSE

which is being replaced by:

EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of #text > DIV to 12 of #text > DIV toDOMRange:range from 11 of #text > DIV to 17 of #text > DIV affinity:NSSelectionAffinityDownstream stillSelecting:FALSE

which doesn't seem right.

Also I think:

+// FIXME: This function should could go on RenderObject an instance method then
+// all cases in which positionForCoordinates recurses could call this instead to

should perhaps read "...should go in an RenderObject instance method, then...".
Comment 15 Eric Seidel (no email) 2009-02-02 17:58:26 PST
Created attachment 27270 [details]
A full git patch for easier applying by ojan (with git am)
Comment 16 Ojan Vafai 2009-02-05 17:42:18 PST
Created attachment 27371 [details]
Fix positionForCoordinates now with an added fix for clicking in the margins of the body.

 LayoutTests/ChangeLog                              |   31 ++++
 ...ick-in-margins-inside-editable-div-expected.txt |   23 +++
 .../click-in-margins-inside-editable-div.html      |   13 ++
 .../click-outside-editable-div-expected.txt        |   20 +++
 .../selection/click-outside-editable-div.html      |   13 ++
 .../selection/contenteditable-click-outside.html   |   13 --
 .../editing/selection/resources/TEMPLATE.html      |   13 ++
 .../click-in-margins-inside-editable-div.js        |  138 ++++++++++++++++
 .../resources/click-outside-editable-div.js        |   55 ++++++
 .../editing/selection/select-missing-image.html    |    2 +-
 ...contenteditable-click-outside-expected.checksum |    1 -
 .../contenteditable-click-outside-expected.txt     |   16 --
 .../selection/select-all-iframe-expected.txt       |    4 +-
 .../select-from-textfield-outwards-expected.txt    |    2 +-
 .../selection/select-missing-image-expected.txt    |    3 +-
 .../contenteditable-click-outside-expected.txt     |   12 --
 .../selection/select-all-iframe-expected.txt       |   56 -------
 WebCore/ChangeLog                                  |   29 ++++
 WebCore/editing/VisiblePosition.cpp                |    6 +-
 WebCore/rendering/RenderBlock.cpp                  |  174 +++++++++-----------
 WebCore/rendering/RenderInline.cpp                 |    1 +
 21 files changed, 426 insertions(+), 199 deletions(-)
Comment 17 Ojan Vafai 2009-02-05 17:50:43 PST
WRT LayoutTests/platform/mac/editing/selection/select-from-textfield-outwards-expected.txt, I think the new behavior is very similar to the old behavior. Specifically, they both have a bug where if you click from the middle of a textfield and drag upwards, at first it selects to the end of the textfield, then at some point selects to the beginning (from the middle). The only difference is where the switch happens. In the old code, it's somewhere in the middle of the margin. In the new code, it's at the top of the margin. This is a separate bug that should be fixed so that we always select from the middle to the beginning of the textfield, or that we do the IE behavior and ignore that you're above the field and continue selecting text as you drag in the x-axis.

WRT LayoutTests/editing/selection/select-missing-image.html. It relied on the old buggy behavior. A execCommand(SelectAll) on a selection that *contains* a contentEditable div should not select the contents of the div. So the new behavior is more correct. My change to the test is to give it an ID that matches the rest of the selection tests, which correctly puts focus inside the div before calling the execCommand. It still tests the original goal that a missing image properly shows it's selected.
Comment 18 Dave Hyatt 2009-02-10 17:48:10 PST
Comment on attachment 27371 [details]
Fix positionForCoordinates now with an added fix for clicking in the margins of the body.

> --- 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.
Comment 19 Ojan Vafai 2009-02-12 16:02:03 PST
Created attachment 27626 [details]
Fix for clicking below the last root line box with multiple line boxes.

 LayoutTests/ChangeLog                              |   35 ++++
 ...ick-in-margins-inside-editable-div-expected.txt |   23 +++
 .../click-in-margins-inside-editable-div.html      |   14 ++
 ...n-padding-with-multiple-line-boxes-expected.txt |   11 ++
 .../click-in-padding-with-multiple-line-boxes.html |   14 ++
 .../click-outside-editable-div-expected.txt        |   20 ++
 .../selection/click-outside-editable-div.html      |   14 ++
 .../selection/contenteditable-click-outside.html   |   13 --
 .../editing/selection/resources/TEMPLATE.html      |   14 ++
 .../click-in-margins-inside-editable-div.js        |   93 ++++++++++
 .../click-in-padding-with-multiple-line-boxes.js   |   62 +++++++
 .../resources/click-outside-editable-div.js        |   49 +++++
 .../resources/js-test-selection-shared.js          |   44 +++++
 .../editing/selection/select-missing-image.html    |    2 +-
 ...contenteditable-click-outside-expected.checksum |    1 -
 .../contenteditable-click-outside-expected.txt     |   16 --
 .../selection/select-all-iframe-expected.txt       |    4 +-
 .../select-from-textfield-outwards-expected.txt    |    2 +-
 .../selection/select-missing-image-expected.txt    |    3 +-
 .../contenteditable-click-outside-expected.txt     |   12 --
 .../selection/select-all-iframe-expected.txt       |   56 ------
 WebCore/ChangeLog                                  |   29 +++
 WebCore/editing/VisiblePosition.cpp                |   10 +-
 WebCore/rendering/RenderBlock.cpp                  |  184 ++++++++++----------
 24 files changed, 525 insertions(+), 200 deletions(-)
Comment 20 Ojan Vafai 2009-02-12 16:06:37 PST
All addressed.

Two issues:
1. If the body was null, I wasn't really sure what to do. returning Position() seems like the only option though. All the layout tests still pass, but I doubt we have any that test a null body.
2. I just went ahead and fixed the FIXME you wanted me to file a bug for. Added a layout test for that case as well. In the process, found a bug this code was about to introduce. :)
Comment 21 Eric Seidel (no email) 2009-02-13 11:27:02 PST
*** Bug 23605 has been marked as a duplicate of this bug. ***
Comment 22 Dave Hyatt 2009-02-17 14:48:22 PST
Comment on attachment 27626 [details]
Fix for clicking below the last root line box with multiple line boxes.

r=me
Comment 23 Ojan Vafai 2009-02-24 13:25:45 PST
Created attachment 27927 [details]
Same patch, but synced to trunk for easier landing by eseidel.

 LayoutTests/ChangeLog                              |   35 ++++
 ...ick-in-margins-inside-editable-div-expected.txt |   23 +++
 .../click-in-margins-inside-editable-div.html      |   14 ++
 ...n-padding-with-multiple-line-boxes-expected.txt |   11 ++
 .../click-in-padding-with-multiple-line-boxes.html |   14 ++
 .../click-outside-editable-div-expected.txt        |   20 ++
 .../selection/click-outside-editable-div.html      |   14 ++
 .../selection/contenteditable-click-outside.html   |   13 --
 .../editing/selection/resources/TEMPLATE.html      |   14 ++
 .../click-in-margins-inside-editable-div.js        |   93 ++++++++++
 .../click-in-padding-with-multiple-line-boxes.js   |   62 +++++++
 .../resources/click-outside-editable-div.js        |   49 +++++
 .../resources/js-test-selection-shared.js          |   44 +++++
 .../editing/selection/select-missing-image.html    |    2 +-
 ...contenteditable-click-outside-expected.checksum |    1 -
 .../contenteditable-click-outside-expected.txt     |   16 --
 .../selection/select-all-iframe-expected.txt       |    4 +-
 .../select-from-textfield-outwards-expected.txt    |    2 +-
 .../selection/select-missing-image-expected.txt    |    3 +-
 .../contenteditable-click-outside-expected.txt     |   12 --
 .../selection/select-all-iframe-expected.txt       |   56 ------
 WebCore/ChangeLog                                  |   29 +++
 WebCore/editing/VisiblePosition.cpp                |   10 +-
 WebCore/rendering/RenderBlock.cpp                  |  184 ++++++++++----------
 24 files changed, 525 insertions(+), 200 deletions(-)
Comment 24 Ojan Vafai 2009-02-24 13:32:51 PST
Created attachment 27929 [details]
patch made with git format-patch for even easier patching by eseidel. :)
Comment 25 Eric Seidel (no email) 2009-02-24 14:28:44 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	D	LayoutTests/editing/selection/contenteditable-click-outside.html
	D	LayoutTests/platform/mac/editing/selection/contenteditable-click-outside-expected.checksum
	D	LayoutTests/platform/mac/editing/selection/contenteditable-click-outside-expected.txt
	D	LayoutTests/platform/qt/editing/selection/contenteditable-click-outside-expected.txt
	D	LayoutTests/platform/qt/editing/selection/select-all-iframe-expected.txt
	M	LayoutTests/ChangeLog
	A	LayoutTests/editing/selection/click-in-margins-inside-editable-div-expected.txt
	A	LayoutTests/editing/selection/click-in-margins-inside-editable-div.html
	A	LayoutTests/editing/selection/click-in-padding-with-multiple-line-boxes-expected.txt
	A	LayoutTests/editing/selection/click-in-padding-with-multiple-line-boxes.html
	A	LayoutTests/editing/selection/click-outside-editable-div-expected.txt
	A	LayoutTests/editing/selection/click-outside-editable-div.html
	A	LayoutTests/editing/selection/resources/TEMPLATE.html
	A	LayoutTests/editing/selection/resources/click-in-margins-inside-editable-div.js
	A	LayoutTests/editing/selection/resources/click-in-padding-with-multiple-line-boxes.js
	A	LayoutTests/editing/selection/resources/click-outside-editable-div.js
	A	LayoutTests/editing/selection/resources/js-test-selection-shared.js
	M	LayoutTests/editing/selection/select-missing-image.html
	M	LayoutTests/platform/mac/editing/selection/select-all-iframe-expected.txt
	M	LayoutTests/platform/mac/editing/selection/select-from-textfield-outwards-expected.txt
	M	LayoutTests/platform/mac/editing/selection/select-missing-image-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/editing/VisiblePosition.cpp
	M	WebCore/rendering/RenderBlock.cpp
Committed r41191