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).

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 on attachment 8613302 [details] [diff] [review] Patch Oops, I found a bug. I'll post new patch soon.

Fixed some nits, not changed any logic.

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.

(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.

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

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.

Okay, how about this?

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

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 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?

(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

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

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

Comment on attachment 8614454 [details] [diff] [review] Patch so, ok, fine, but remove that #ifdef DEBUG then.