Bug 1351170 - 2. Notify selection listeners after adjusting range offsets; r=smaug draft
authorJim Chen <nchen@mozilla.com>
Wed, 19 Jul 2017 14:29:59 -0400
changeset 611463 da0cc3073e4b8ad23c6f6eab42da5aa8b269cae9
parent 611462 7d86b6cf04581f1cd71fa85f8c8586541b3a84e9
child 611464 743828adfe9dfda93480a5372697989a77492d52
push id69220
push userbmo:nchen@mozilla.com
push dateWed, 19 Jul 2017 18:31:25 +0000
reviewerssmaug
bugs1351170
milestone56.0a1
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
dom/base/nsRange.cpp
dom/tests/mochitest/general/frameSelectEvents.html
widget/tests/window_composition_text_querycontent.xul
--- 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";