Closed Bug 864595 Opened 7 years ago Closed 6 years ago

"ASSERTION: point.x should not be outside of rect r" with getBoundingClientRect on an nsRange that falls within trailing whitespace of a text node

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jruderman, Assigned: mtseng)

References

Details

(Keywords: assertion, testcase)

Attachments

(5 files, 5 obsolete files)

Attached file testcase
###!!! 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
Attached file stack+ (obsolete) —
OS: Mac OS X → All
Hardware: x86_64 → All
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.
Attached patch b864595 (obsolete) — Splinter Review
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.
Assignee: nobody → sjohnson
Attachment #762124 - Flags: review?(tnikkel)
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)?
Attached file testcase2
Another testcase showing that this can be reproduced in javascript without the use of caretPositionFromPoint.
Summary: "ASSERTION: point.x should not be outside of rect r" with caretPositionFromPoint, getClientRect → "ASSERTION: point.x should not be outside of rect r" with getBoundingClientRect on an nsRange that falls within trailing whitespace of a text node
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.
Attachment #762124 - Flags: review?(tnikkel) → review-
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.
Depends on: 918185
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? :-)
Flags: needinfo?(mtseng)
> So we can ignore Chrome.

Please file a bug on them, though?
Attached patch assert_in_nsrange v1 (obsolete) — Splinter Review
Sure :)

Here is patch base on your suggestion.
Assignee: jaywir3 → mtseng
Status: NEW → ASSIGNED
Attachment #8446382 - Flags: review?(tnikkel)
Attachment #8446382 - Flags: review?(roc)
Flags: needinfo?(mtseng)
Attached file testcase3
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.
Attachment #8446382 - Flags: review?(roc) → review+
(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))".
Attached patch assert_in_nsrange v2 (obsolete) — Splinter Review
Return rect at point.x with zero-width when !aClampToEdge and point outside frame's rect.
Attachment #8446382 - Attachment is obsolete: true
Attachment #8446382 - Flags: review?(tnikkel)
Attachment #8446484 - Flags: review?(tnikkel)
Attachment #8446484 - Flags: review?(roc)
Attached patch mochitest (obsolete) — Splinter Review
Mochitest for this bug.
Attachment #8446485 - Flags: review?(tnikkel)
Attachment #8446485 - Flags: review?(roc)
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.
Attachment #8446485 - Flags: review?(roc) → review+
Comment on attachment 8446484 [details] [diff] [review]
assert_in_nsrange v2

I think roc's review is sufficient here.
Attachment #8446484 - Flags: review?(tnikkel)
Attachment #8446485 - Flags: review?(tnikkel)
Removed contenteditable and update commit message
Attachment #740581 - Attachment is obsolete: true
Attachment #762124 - Attachment is obsolete: true
Attachment #8446485 - Attachment is obsolete: true
Updated commit message.
Attachment #8446484 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/28abd4f8ca40
https://hg.mozilla.org/mozilla-central/rev/514b8287dd6e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.