###!!! ASSERTION: point.x should not be outside of rect r: 'aR->x <= point.x && point.x <= aR->XMost()', file content/base/src/nsRange.cpp, line 2825

I think I see what might be happening here. It looks like caretPositionFromPoint is counting the last character, which is a newline, as part of the offset. Thus, it's getting "6" as the offset. But, the content rect doesn't seem to include the whitespace as part of it, causing it to expect that 2580 should be the furthest to the right that should occur. I think caretPositionFromPoint should be modified to return Length() - 1 as an offset, if it would normally return Length(), but the last character is whitespace of some kind.

I think there are two possible solutions to this problem: A) Change the assertion in nsRange so that it checks the content rect + the rect of any trailing whitespace at the end of a block, or B) Change caretPositionFromPoint to return offsets that are not within the trailing whitespace of a given content node. I think I'm leaning towards B, since the other behavior is probably more likely to be correct, given that it's been around longer. However, I'm going to double-check the spec to see if there is anything in it that makes it clear that offsets should include trailing whitespace.

This patch makes nsDOMCaretPosition adjust its offset so that trailing whitespace is ignored. The reason this is necessary is that nsLineLayout, when performing reflow, and thus positioning/sizing the frame, ignores trailing whitespace. If the offset for a given caret position is within the trailing whitespace of the node for which it is a part, then getClientRect() will be outside of the bounds of the frame for that node.

Comment on attachment 762124 [details] [diff] [review] b864595 This feels very hacky. These are just my first thoughts. At minimum don't we want to make sure the frames are subject to nsLineLayout::TrimTrailingWhiteSpace? The frames could not be under control of nsLineLayout at all, or maybe it could have whitespace: pre? I reserve the right to ask for larger changes that make any work on the above comment wasted effort. :)

Also, is it possible to construct a DOM range object that is only in trailing whitespace and ask for it's rect and trigger the same assertion (without any use of caretPositionFromPoint)?

Another testcase showing that this can be reproduced in javascript without the use of caretPositionFromPoint.

Comment on attachment 762124 [details] [diff] [review] b864595 Given that this isn't a bug in caret position but rather in range I don't think we should work around it in caret position.

If I'm typing in contenteditable text, and I hit the space bar, I want the caret to move to reflect that space even if the space would be trimmed as part of the layout algorithm (or maybe we prevent it from being trimmed as a function of the actual caret being at that point?). So maybe we actually want the CaretPosition API to do the same thing, and return a point that's actually outside of the element? Or does the range code needs to be able to answer slightly different questions for different callers?

The editor has some logic to handle trailing spaces by converting them to non-breaking spaces and then converting them back to regular spaces in some cases after you enter a non-whitespace letter at the end of the whitespace that would be collapsed otherwise. That being said, in layout, collapsed whitespaces do not have visible rendering, so why would we want to expose them in getBoundingClientRect result?

We should recheck this to see if it still fails now that 918185 has landed.

Still asserts on trunk.

When a line of editable text has trailing whitespace, for layout purposes the trailing whitespace is trimmed, so the text frame's layout box excludes the trailing whitespace. But when the caret is positioned at the end of the line, it must be positioned as if the whitespace was not trimmed, i.e. it is positioned outside the text frame's layout box. Hence the assertion is triggered. It's debatable how Range.getClientRects() and CaretPosition.getClientRect() should behave in this case. I think a reasonable approach would be to say that Range.getClientRects() clamps offsets to the textframe box (in ExtractRectFromOffset), but CaretPosition.getClientRect() returns the actual position of the caret. So nsDOMCaretPosition could call CollectClientRects directly and pass a flag to indicate that offsets outside the textframe box are allowed. I looked at what Chrome does, but its behavior is totally broken in multiple ways (even ignoring that it still has caretRangeFromPoint instead of caretPositionFromPoint). It doesn't seem to allow caret positions at the end of a text node with trailing whitespace; given <div contenteditable>Hello </div>, clicking at the end of the line puts the caret after the 'o', and typing a character replaces the space with that character... caretRangeFromPoint returns 5, not 6, in Jesse's testcase. Calling Range.getClientRects() on an a collapsed range at offset 6 returns an empty rect list! So we can ignore Chrome. Morris, would you like to fix this? :-)

> So we can ignore Chrome. Please file a bug on them, though?

Sure :) Here is patch base on your suggestion.

This is test case, a div with content "abcd<space><space>" black rect indicate range from 0 to 5, because of we clamp the rect to frame's rect. so black rect should not contain space. red rect indicate caret position which allow to outside the frame's rect, so this rect should be located at outside of text's rect.

Comment on attachment 8446382 [details] [diff] [review] assert_in_nsrange v1 Review of attachment 8446382 [details] [diff] [review]: ----------------------------------------------------------------- Looks good other than that. Needs an automated test. ::: content/base/src/nsRange.cpp @@ +2746,5 @@ > + > + // Keep left/right but point is on left/right side of rect, > + // replace it with point's position. > + if (aR->width < 0) { > + aR->width = 0; This can't happen, right? Because we clamped the point above.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18) > Comment on attachment 8446382 [details] [diff] [review] > assert_in_nsrange v1 > > Review of attachment 8446382 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good other than that. > > Needs an automated test. > > ::: content/base/src/nsRange.cpp > @@ +2746,5 @@ > > + > > + // Keep left/right but point is on left/right side of rect, > > + // replace it with point's position. > > + if (aR->width < 0) { > > + aR->width = 0; > > This can't happen, right? Because we clamped the point above. If aClampToEdge is true, yes. Maybe change this to "if (aR->width < 0 && !aClampToEdge)"?

Comment on attachment 8446382 [details] [diff] [review] assert_in_nsrange v1 Review of attachment 8446382 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsRange.cpp @@ +2746,5 @@ > + > + // Keep left/right but point is on left/right side of rect, > + // replace it with point's position. > + if (aR->width < 0) { > + aR->width = 0; OK, but it's not easy to see that this code is correct. How about this: if !aClampToEdge, then just return the zero-width rectangle at point.x and return. Otherwise clamp and apply the rest of the logic.

(In reply to Boris Zbarsky [:bz] from comment #15) > > So we can ignore Chrome. > > Please file a bug on them, though? Filed http://crbug.com/389054 and http://crbug.com/388976.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20) > Comment on attachment 8446382 [details] [diff] [review] > assert_in_nsrange v1 > > Review of attachment 8446382 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/base/src/nsRange.cpp > @@ +2746,5 @@ > > + > > + // Keep left/right but point is on left/right side of rect, > > + // replace it with point's position. > > + if (aR->width < 0) { > > + aR->width = 0; > > OK, but it's not easy to see that this code is correct. > > How about this: if !aClampToEdge, then just return the zero-width rectangle > at point.x and return. Otherwise clamp and apply the rest of the logic. But we return zero-width rect at point.x only if !aClampToEdge and point is outside frame's box. So the condition should be "if (!aClampToEdge && !aR->Contains(point))".

Return rect at point.x with zero-width when !aClampToEdge and point outside frame's rect.

Mochitest for this bug.

Comment on attachment 8446485 [details] [diff] [review] mochitest Review of attachment 8446485 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/test/test_bug864595.html @@ +9,5 @@ > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> > +</head> > +<body> > +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=864595">Mozilla Bug 864595</a> > +<div id='editable' style='display:inline-block;' contenteditable=true>abcd </div> You actually don't need contenteditable here.

Comment on attachment 8446484 [details] [diff] [review] assert_in_nsrange v2 I think roc's review is sufficient here.

Removed contenteditable and update commit message

Updated commit message.

try push: https://tbpl.mozilla.org/?tree=Try&rev=700b850a42e1

https://hg.mozilla.org/integration/mozilla-inbound/rev/28abd4f8ca40 https://hg.mozilla.org/integration/mozilla-inbound/rev/514b8287dd6e

https://hg.mozilla.org/mozilla-central/rev/28abd4f8ca40 https://hg.mozilla.org/mozilla-central/rev/514b8287dd6e