1169917 - ContentEventHandler::GetStartFrameAndOffset() shouldn't assert even if found frame is not a text frame

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

ContentEventHandler::GetStartFrameAndOffset() may find non-textframe. For example, selection range could be collapsed in anonymous div of <textarea>, be next to object node like <img>, actually, there is no text node.

In the first case, it should find text node to be modified by keyboard or IME.

I'm not sure what we should do in the second case. We should return same result but shouldn't assert it.

In the last case, perhaps, we need to return the found node's primary frame (i.e., same as current behavior).

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

The main purpose of this bug fix is, it shouldn't assert when the start frame is not textframe because it could be if caret is in empty block element or between non-text nodes, e.g., between two <img>s.

We should remove the assertion for making tests on debug builds stabler. The assertion causes unexpected oranges if we change the timing of querying caret rect.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8613302 [details] [diff] [review]
Patch

Oops, I found a bug. I'll post new patch soon.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Fixed some nits, not changed any logic.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8613502 [details] [diff] [review]
Patch

>+  // If the given point is next to text node, let's use the nearest point in
>+  // the found text node.
>+  nsINode* child = nullptr;
>+  int32_t offset = -1;
>+  if (!aOffsetInNode && aNode->HasChildren()) {
>+    child = aNode->GetFirstChild();
>+    offset = 0;
>+  } else if (static_cast<uint32_t>(aOffsetInNode) < aNode->Length()) {
>+    child = aNode->GetChildAt(aOffsetInNode - 1);
>+    offset = child->Length();
>+  }

I don't understand this code.
Using both HasChildren and Length() is quite misleading and I don't quite see why
GetFirstChild() in some case but GetChildAt in some other.
And child aNode->GetChildAt(aOffsetInNode - 1); yet offset = child->Length();?
Why we want to point to the end of the child node, not start, like we do in case of the
first child where we just always set offset to 0.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8613502 [details] [diff] [review]
> Patch
> 
> >+  // If the given point is next to text node, let's use the nearest point in
> >+  // the found text node.
> >+  nsINode* child = nullptr;
> >+  int32_t offset = -1;
> >+  if (!aOffsetInNode && aNode->HasChildren()) {
> >+    child = aNode->GetFirstChild();
> >+    offset = 0;
> >+  } else if (static_cast<uint32_t>(aOffsetInNode) < aNode->Length()) {
> >+    child = aNode->GetChildAt(aOffsetInNode - 1);
> >+    offset = child->Length();
> >+  }
> 
> I don't understand this code.
> Using both HasChildren and Length() is quite misleading and I don't quite
> see why
> GetFirstChild() in some case but GetChildAt in some other.
> And child aNode->GetChildAt(aOffsetInNode - 1); yet offset =
> child->Length();?
> Why we want to point to the end of the child node, not start, like we do in
> case of the
> first child where we just always set offset to 0.

If the range points the start of a container element's start, then, the start of the first child should be proper insertion point if it's a text node.

Otherwise, according to nsEditor's code, we prefer to insert previous text node. 
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditor.cpp#2287
Although, the code limits this works with editor root node.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Ok so we're just trying to copy editor's behavior here.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8613502 [details] [diff] [review]
Patch

>   // Otherwise, we should set the guessed caret rect.
>   nsRefPtr<nsRange> range = new nsRange(mRootContent);
So, here we create a range object.

>   rv = SetRangeFromFlatTextOffset(range, aEvent->mInput.mOffset, 0,
>                                   lineBreakType, true,
>                                   &aEvent->mReply.mOffset);
...update the range here


>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  range = GetRangeMaybeInTextNode(range->GetStartParent(),
>+                                  range->StartOffset());
and then create a new range here.
Somewhat odd and malloc heavy. Though this shouldn't be hot code. But still, couldn't we reuse the range?
Perhaps call GetRangeMaybeInTextNode something else which hints that the range is being modified and that its boundary points
are possibly moved to point some text node descendant.


>+  // If the given point is next to text node, let's use the nearest point in
>+  // the found text node.
So this comment is weird, since we're certainly not trying to find nearest text node.
We just check if we have child nodes, and try to use the first one is offset == 0, and otherwise the last child, and
then check if we have text node.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Okay, how about this?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8614048 [details] [diff] [review]
Patch

>+  // Couldn't find text node, return the range of given node and offset.
>+  if (!child || !child->IsNodeOfType(!nsINode::eTEXT)) {
!nsINode::eTEXT ?


>+    return result.forget();
and don't you want to return null here?


>+  }
>+
>+  if (NS_WARN_IF(offset < 0)) {
>+    return result.forget();
>+  }
and here

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Oops, my working repository was broken, and I created new patch in another repository. However, I posted the patch in old one.

Sorry, this is the new patch (the previous patch is older than the first patch, it's wired...)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8614454 [details] [diff] [review]
Patch

>+  AdjustCollapsedRangeMaybeIntoTextNode(range);
>+  if (NS_WARN_IF(!range)) {
>+    return NS_ERROR_FAILURE;
>+  }

you sure have range object always there. You want to check the return value of 
AdjustCollapsedRangeMaybeIntoTextNode



>+  // But if the found node isn't a text node, we cannot modify the range.
>+  if (!childNode || !childNode->IsNodeOfType(nsINode::eTEXT) ||
>+      NS_WARN_IF(offsetInChildNode < 0)) {
>+    return NS_OK;
>+  }
Why do we want to return NS_OK here?

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 8614454 [details] [diff] [review]
> Patch
> 
> >+  AdjustCollapsedRangeMaybeIntoTextNode(range);
> >+  if (NS_WARN_IF(!range)) {
> >+    return NS_ERROR_FAILURE;
> >+  }
> 
> you sure have range object always there. You want to check the return value
> of 
> AdjustCollapsedRangeMaybeIntoTextNode

Oh, right.

> >+  // But if the found node isn't a text node, we cannot modify the range.
> >+  if (!childNode || !childNode->IsNodeOfType(nsINode::eTEXT) ||
> >+      NS_WARN_IF(offsetInChildNode < 0)) {
> >+    return NS_OK;
> >+  }
> Why do we want to return NS_OK here?

because if there is no proper text node, we need to use the non-text node, e.g., <img>, <div> or something. In such case, we should hope that nsFrame::GetPointFromOffset() works not so odd.
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#6015

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

But don't we then assert in if (frame->GetType() != nsGkAtoms::textFrame) { ?
nsFrameSelection::GetFrameForNodeOffset sure can return non-null nsIFrame* which isn't textFrame

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

er, that isn't assert, just a warning.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8614454 [details] [diff] [review]
Patch

so, ok, fine, but remove that #ifdef DEBUG then.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/360ebcf8f058

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/360ebcf8f058