Bug 1351170 - 2. Notify selection listeners after adjusting range offsets; r=smaug
`nsRange` registers mutation observers to adjust the range when content
changes. However, there are some cases where we adjust the start and/or
end offsets but don't notify selection listeners (i.e. we don't call
`nsRange::DoSetRange` to set the new range points, contrary to what the
comment above `nsRange::DoSetRange` says). This patch makes us call
`nsRange::DoSetRange` in those cases. The patch adds a testcase in
test_selectevents.html, and changes a few unexpected-pass cases in
test_composition_text_querycontent.xul that this patch fixed.
MozReview-Commit-ID: 73D8RYMS3MS
--- a/dom/base/nsRange.cpp
+++ b/dom/base/nsRange.cpp
@@ -453,28 +453,30 @@ nsRange::CharacterDataChanged(nsIDocumen
// then we need to increment the corresponding offset to account for the new
// text node that will be inserted. If so, we need to prevent the next
// ContentInserted or ContentAppended for this range from incrementing it
// again (when the new text node is notified).
nsINode* parentNode = aContent->GetParentNode();
int32_t index = -1;
if (parentNode == mEndContainer && mEndOffset > 0 &&
(index = parentNode->IndexOf(aContent)) + 1 == mEndOffset) {
- ++mEndOffset;
+ newEndNode = mEndContainer;
+ newEndOffset = mEndOffset + 1;
mEndOffsetWasIncremented = true;
}
if (parentNode == mStartContainer && mStartOffset > 0 &&
(index != -1 ? index : parentNode->IndexOf(aContent)) + 1 == mStartOffset) {
- ++mStartOffset;
+ newStartNode = mStartContainer;
+ newStartOffset = mStartOffset + 1;
mStartOffsetWasIncremented = true;
}
#ifdef DEBUG
if (mStartOffsetWasIncremented || mEndOffsetWasIncremented) {
mAssertNextInsertOrAppendIndex =
- (mStartOffsetWasIncremented ? mStartOffset : mEndOffset) - 1;
+ (mStartOffsetWasIncremented ? newStartOffset : newEndOffset) - 1;
mAssertNextInsertOrAppendNode = aInfo->mDetails->mNextSibling;
}
#endif
}
// If the changed node contains our start boundary and the change starts
// before the boundary we'll need to adjust the offset.
if (aContent == mStartContainer &&
@@ -499,17 +501,18 @@ nsRange::CharacterDataChanged(nsIDocumen
RegisterCommonAncestor(newStartNode);
}
if (mStartContainer->IsDescendantOfCommonAncestorForRangeInSelection()) {
newStartNode->SetDescendantOfCommonAncestorForRangeInSelection();
}
} else {
// If boundary is inside changed text, position it before change
// else adjust start offset for the change in length.
- mStartOffset = static_cast<uint32_t>(mStartOffset) <= aInfo->mChangeEnd ?
+ newStartNode = mStartContainer;
+ newStartOffset = static_cast<uint32_t>(mStartOffset) <= aInfo->mChangeEnd ?
aInfo->mChangeStart :
mStartOffset + aInfo->mChangeStart - aInfo->mChangeEnd +
aInfo->mReplaceLength;
}
}
// Do the same thing for the end boundary, except for splitText of a node
// with no parent then only switch to the new node if the start boundary
@@ -533,17 +536,18 @@ nsRange::CharacterDataChanged(nsIDocumen
UnregisterCommonAncestor(mStartContainer);
RegisterCommonAncestor(mStartContainer->GetParentNode());
newEndNode->SetDescendantOfCommonAncestorForRangeInSelection();
} else if (mEndContainer->
IsDescendantOfCommonAncestorForRangeInSelection()) {
newEndNode->SetDescendantOfCommonAncestorForRangeInSelection();
}
} else {
- mEndOffset = static_cast<uint32_t>(mEndOffset) <= aInfo->mChangeEnd ?
+ newEndNode = mEndContainer;
+ newEndOffset = static_cast<uint32_t>(mEndOffset) <= aInfo->mChangeEnd ?
aInfo->mChangeStart :
mEndOffset + aInfo->mChangeStart - aInfo->mChangeEnd +
aInfo->mReplaceLength;
}
}
if (aInfo->mDetails &&
aInfo->mDetails->mType == CharacterDataChangeInfo::Details::eMerge) {
@@ -635,26 +639,31 @@ nsRange::ContentAppended(nsIDocument* aD
void
nsRange::ContentInserted(nsIDocument* aDocument,
nsIContent* aContainer,
nsIContent* aChild,
int32_t aIndexInContainer)
{
NS_ASSERTION(mIsPositioned, "shouldn't be notified if not positioned");
+ bool rangeChanged = false;
+ uint32_t newStartOffset = mStartOffset;
+ uint32_t newEndOffset = mEndOffset;
nsINode* container = NODE_FROM(aContainer, aDocument);
// Adjust position if a sibling was inserted.
if (container == mStartContainer && aIndexInContainer < mStartOffset &&
!mStartOffsetWasIncremented) {
- ++mStartOffset;
+ ++newStartOffset;
+ rangeChanged = true;
}
if (container == mEndContainer && aIndexInContainer < mEndOffset &&
!mEndOffsetWasIncremented) {
- ++mEndOffset;
+ ++newEndOffset;
+ rangeChanged = true;
}
if (container->IsSelectionDescendant() &&
!aChild->IsDescendantOfCommonAncestorForRangeInSelection()) {
MarkDescendants(aChild);
aChild->SetDescendantOfCommonAncestorForRangeInSelection();
}
if (mStartOffsetWasIncremented || mEndOffsetWasIncremented) {
@@ -662,16 +671,21 @@ nsRange::ContentInserted(nsIDocument* aD
MOZ_ASSERT(mAssertNextInsertOrAppendNode == aChild);
MOZ_ASSERT(aChild->IsNodeOfType(nsINode::eDATA_NODE));
mStartOffsetWasIncremented = mEndOffsetWasIncremented = false;
#ifdef DEBUG
mAssertNextInsertOrAppendIndex = -1;
mAssertNextInsertOrAppendNode = nullptr;
#endif
}
+
+ if (rangeChanged) {
+ DoSetRange(mStartContainer, newStartOffset,
+ mEndContainer, newEndOffset, mRoot);
+ }
}
void
nsRange::ContentRemoved(nsIDocument* aDocument,
nsIContent* aContainer,
nsIContent* aChild,
int32_t aIndexInContainer,
nsIContent* aPreviousSibling)
@@ -680,45 +694,50 @@ nsRange::ContentRemoved(nsIDocument* aDo
MOZ_ASSERT(!mStartOffsetWasIncremented && !mEndOffsetWasIncremented &&
mAssertNextInsertOrAppendIndex == -1,
"splitText failed to notify insert/append?");
nsINode* container = NODE_FROM(aContainer, aDocument);
bool gravitateStart = false;
bool gravitateEnd = false;
bool didCheckStartParentDescendant = false;
+ bool rangeChanged = false;
+ uint32_t newStartOffset = mStartOffset;
+ uint32_t newEndOffset = mEndOffset;
// Adjust position if a sibling was removed...
if (container == mStartContainer) {
if (aIndexInContainer < mStartOffset) {
- --mStartOffset;
+ --newStartOffset;
+ rangeChanged = true;
}
} else { // ...or gravitate if an ancestor was removed.
didCheckStartParentDescendant = true;
gravitateStart =
nsContentUtils::ContentIsDescendantOf(mStartContainer, aChild);
}
// Do same thing for end boundry.
if (container == mEndContainer) {
if (aIndexInContainer < mEndOffset) {
- --mEndOffset;
+ --newEndOffset;
+ rangeChanged = true;
}
} else if (didCheckStartParentDescendant &&
mStartContainer == mEndContainer) {
gravitateEnd = gravitateStart;
} else {
gravitateEnd = nsContentUtils::ContentIsDescendantOf(mEndContainer, aChild);
}
- if (gravitateStart || gravitateEnd) {
+ if (gravitateStart || gravitateEnd || rangeChanged) {
DoSetRange(gravitateStart ? container : mStartContainer.get(),
- gravitateStart ? aIndexInContainer : mStartOffset,
+ gravitateStart ? aIndexInContainer : newStartOffset,
gravitateEnd ? container : mEndContainer.get(),
- gravitateEnd ? aIndexInContainer : mEndOffset,
+ gravitateEnd ? aIndexInContainer : newEndOffset,
mRoot);
}
if (container->IsSelectionDescendant() &&
aChild->IsDescendantOfCommonAncestorForRangeInSelection()) {
aChild->ClearDescendantOfCommonAncestorForRangeInSelection();
UnmarkDescendants(aChild);
}
}
--- a/dom/tests/mochitest/general/frameSelectEvents.html
+++ b/dom/tests/mochitest/general/frameSelectEvents.html
@@ -456,12 +456,30 @@
elt("textarea").setAttribute("style", "");
await spin();
is(selectstart, 0, "tdn - ss 2");
is(selectionchange, 0, "tdn - sc 2");
is(inputSelectionchange, 0, "tdn - isc 2");
is(textareaSelectionchange, 0, "tdn - tsc 2");
reset();
+
+ // When selection is at the end of contentEditable's content,
+ // clearing the content should trigger selection events.
+ var savedContent = elt("ce").innerHTML;
+ document.getSelection().setBaseAndExtent(elt("ce"), 1, elt("ce"), 1);
+ await spin();
+ reset();
+
+ elt("ce").firstChild.remove();
+ await spin();
+ is(selectstart, 0, "clear ce - ss");
+ is(selectionchange, 1, "clear ce - sc");
+ is(inputSelectionchange, 0, "clear ce - isc");
+ is(textareaSelectionchange, 0, "clear ce - tsc");
+
+ elt("ce").innerHTML = savedContent;
+ await spin();
+ reset();
});
</script>
</body>
</html>
--- a/widget/tests/window_composition_text_querycontent.xul
+++ b/widget/tests/window_composition_text_querycontent.xul
@@ -7879,19 +7879,19 @@ function* runIMEContentObserverTest()
if (isDefaultParagraphSeparatorBlock) {
// Joining two blocks causes removing both block elements and inserting new block element.
checkTextChangeNotification(notifications[0], description, { offset: offsetAtContainer - kLFLen, removedLength: getNativeText("\naB\nd").length, addedLength: getNativeText("\naBd").length });
checkSelectionChangeNotification(notifications[1], description, { offset: 2 + offsetAtContainer, text: "" });
checkPositionChangeNotification(notifications[2], description);
dumpUnexpectedNotifications(description, 3);
} else {
checkTextChangeNotification(notifications[0], description, { offset: 2 + offsetAtContainer, removedLength: kLFLen, addedLength: 0 });
- todo_is(notifications.length, 3, description + " should cause 3 notifications");
- todo_is(notifications[1] && notifications[1].type, "notify-selection-change", description + " should cause selection change notification");
- todo_is(notifications[2] && notifications[2].type, "notify-position-change", description + " should cause position change notification");
+ is(notifications.length, 3, description + " should cause 3 notifications");
+ is(notifications[1] && notifications[1].type, "notify-selection-change", description + " should cause selection change notification");
+ is(notifications[2] && notifications[2].type, "notify-position-change", description + " should cause position change notification");
}
// "a[B]d"
synthesizeKey("KEY_ArrowLeft", { code: "ArrowLeft", shiftKey: true }, win, callback);
yield flushNotifications();
// "a<br>[]d" or "<block>a</block><block>[]d</block>"
description = aDescription + "replacing 'B' with a line break with pressing Enter";