Bug 1467796 - part 3: Make mozInlineSpellChecker::ReplaceWord() use TextEditor::ReplaceTextAsAction() r=m_kato,smaug

mozInlineSpellChecker::ReplaceWord() is used for replacing misspelled word
with a word.  So, this is necessary to be distinguished from insertText
command when we implement InputEvent.inputType.  So, we should make it
use TextEditor::ReplaceTextAsAction() instead (same as autocomplete).

This patch makes TextEditor::ReplaceTextAsAction() take optional argument
to make callers can specify replace range.  Then, the range is a spellchecker
selection range if the caller is mozInlineSpellChecker::ReplaceWord().
Prior to this patch, it clones the range for normal selection, but it's
expensive and we may be able to reuse cached range of Selection in this case.
So, this patch makes Selection::AddRangeInternal() checks if given range is
in another Selection and use mCachedRange as far as possible.

MozReview-Commit-ID: JIOTTsxlj4Q

--HG--
extra : rebase_source : 7c26b0255f08608ebe8c7045c9bcdca1dc70cadf

dom/base/Selection.cpp

@@ -2130,7 +2130,27 @@ void
 Selection::AddRangeInternal(nsRange& aRange, nsIDocument* aDocument,
                             ErrorResult& aRv)
 {
-  nsINode* rangeRoot = aRange.GetRoot();
+  // If the given range is part of another Selection, we need to clone the
+  // range first.
+  RefPtr<nsRange> range;
+  if (aRange.IsInSelection() && aRange.GetSelection() != this) {
+    // Because of performance reason, when there is a cached range, let's use
+    // it.  Otherwise, clone the range.
+    if (mCachedRange) {
+      range = std::move(mCachedRange);
+      nsresult rv = range->SetStartAndEnd(aRange.StartRef().AsRaw(),
+                                          aRange.EndRef().AsRaw());
+      if (NS_WARN_IF(NS_FAILED(rv))) {
+        return;
+      }
+    } else {
+      range = aRange.CloneRange();
+    }
+  } else {
+    range = &aRange;
+  }
+
+  nsINode* rangeRoot = range->GetRoot();
   if (aDocument != rangeRoot && (!rangeRoot ||
                                  aDocument != rangeRoot->GetComposedDoc())) {
     // http://w3c.github.io/selection-api/#dom-selection-addrange
@@ -2150,14 +2170,14 @@ Selection::AddRangeInternal(nsRange& aRange, nsIDocument* aDocument,
   // and returns NS_OK if range doesn't contain just one table cell
   bool didAddRange;
   int32_t rangeIndex;
-  nsresult result = AddTableCellRange(&aRange, &didAddRange, &rangeIndex);
+  nsresult result = AddTableCellRange(range, &didAddRange, &rangeIndex);
   if (NS_FAILED(result)) {
     aRv.Throw(result);
     return;
   }
 
   if (!didAddRange) {
-    result = AddItem(&aRange, &rangeIndex);
+    result = AddItem(range, &rangeIndex);
     if (NS_FAILED(result)) {
       aRv.Throw(result);
       return;
@@ -2176,7 +2196,7 @@ Selection::AddRangeInternal(nsRange& aRange, nsIDocument* aDocument,
   }
 
   RefPtr<nsPresContext>  presContext = GetPresContext();
-  SelectFrames(presContext, &aRange, true);
+  SelectFrames(presContext, range, true);
 
   if (!mFrameSelection)
     return;//nothing to do

dom/base/nsRange.h

@@ -134,6 +134,11 @@ public:
    */
   void SetSelection(mozilla::dom::Selection* aSelection);
 
+  /**
+   * Returns pointer to a Selection if the range is associated with a Selection.
+   */
+  mozilla::dom::Selection* GetSelection() const { return mSelection; }
+
   /**
    * Return true if this range was generated.
    * @see SetIsGenerated

editor/libeditor/TextEditor.cpp

@@ -1145,7 +1145,8 @@ TextEditor::SetText(const nsAString& aString)
 }
 
 nsresult
-TextEditor::ReplaceTextAsAction(const nsAString& aString)
+TextEditor::ReplaceTextAsAction(const nsAString& aString,
+                                nsRange* aReplaceRange /* = nullptr */)
 {
   AutoPlaceholderBatch batch(this, nullptr);
 
@@ -1154,7 +1155,41 @@ TextEditor::ReplaceTextAsAction(const nsAString& aString)
                                       *this, EditSubAction::eInsertText,
                                       nsIEditor::eNext);
 
-  nsresult rv = SetTextAsSubAction(aString);
+  if (!aReplaceRange) {
+    nsresult rv = SetTextAsSubAction(aString);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return rv;
+    }
+    return NS_OK;
+  }
+
+  if (NS_WARN_IF(aString.IsEmpty() && aReplaceRange->Collapsed())) {
+    return NS_OK;
+  }
+
+  // Note that do not notify selectionchange caused by selecting all text
+  // because it's preparation of our delete implementation so web apps
+  // shouldn't receive such selectionchange before the first mutation.
+  AutoUpdateViewBatch preventSelectionChangeEvent(this);
+
+  RefPtr<Selection> selection = GetSelection();
+  if (NS_WARN_IF(!selection)) {
+    return NS_ERROR_FAILURE;
+  }
+
+  // Select the range but as far as possible, we should not create new range
+  // even if it's part of special Selection.
+  nsresult rv = selection->RemoveAllRangesTemporarily();
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return rv;
+  }
+  ErrorResult error;
+  selection->AddRange(*aReplaceRange, error);
+  if (NS_WARN_IF(error.Failed())) {
+    return error.StealNSResult();
+  }
+
+  rv = ReplaceSelectionAsSubAction(aString);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
@@ -1216,19 +1251,33 @@ TextEditor::SetTextAsSubAction(const nsAString& aString)
       rv = EditorBase::SelectEntireDocument(selection);
     }
     if (NS_SUCCEEDED(rv)) {
-      if (aString.IsEmpty()) {
+      rv = ReplaceSelectionAsSubAction(aString);
-        rv = DeleteSelectionAsSubAction(eNone, eStrip);
+      NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
-        NS_WARNING_ASSERTION(NS_FAILED(rv), "Failed to remove all text");
+        "Failed to replace selection with new string");
-      } else {
-        rv = InsertTextAsSubAction(aString);
-        NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Failed to insert the new text");
-      }
     }
   }
   // post-process
   return rules->DidDoAction(selection, subActionInfo, rv);
 }
 
+nsresult
+TextEditor::ReplaceSelectionAsSubAction(const nsAString& aString)
+{
+  if (aString.IsEmpty()) {
+    nsresult rv = DeleteSelectionAsSubAction(eNone, eStrip);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return rv;
+    }
+    return NS_OK;
+  }
+
+  nsresult rv = InsertTextAsSubAction(aString);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return rv;
+  }
+  return NS_OK;
+}
+
 bool
 TextEditor::EnsureComposition(WidgetCompositionEvent& aCompositionEvent)
 {

editor/libeditor/TextEditor.h

@@ -161,12 +161,15 @@ public:
   nsresult SetText(const nsAString& aString);
 
   /**
-   * Replace all text in this editor with aString and treat the change as
+   * Replace text in aReplaceRange or all text in this editor with aString and
-   * inserting the string.
+   * treat the change as inserting the string.
    *
-   * @param aString    The string to set.
+   * @param aString             The string to set.
+   * @param aReplaceRange       The range to be replaced.
+   *                            If nullptr, all contents will be replaced.
    */
-  nsresult ReplaceTextAsAction(const nsAString& aString);
+  nsresult ReplaceTextAsAction(const nsAString& aString,
+                               nsRange* aReplaceRange = nullptr);
 
   /**
    * OnInputParagraphSeparator() is called when user tries to separate current
@@ -279,6 +282,13 @@ protected: // May be called by friends.
    */
   nsresult SetTextAsSubAction(const nsAString& aString);
 
+  /**
+   * ReplaceSelectionAsSubAction() replaces selection with aString.
+   *
+   * @param aString    The string to replace.
+   */
+  nsresult ReplaceSelectionAsSubAction(const nsAString& aString);
+
   /**
    * InsertBrElementWithTransaction() creates a <br> element and inserts it
    * before aPointToInsert.  Then, tries to collapse selection at or after the

editor/libeditor/tests/mochitest.ini

@@ -284,6 +284,8 @@ subsuite = clipboard
 [test_select_all_without_body.html]
 [test_spellcheck_pref.html]
 skip-if = toolkit == 'android'
+[test_undo_after_spellchecker_replaces_word.html]
+skip-if = toolkit == 'android'
 [test_undo_redo_stack_after_setting_value.html]
 [test_backspace_vs.html]
 [test_css_chrome_load_access.html]

editor/libeditor/tests/test_undo_after_spellchecker_replaces_word.html

@@ -0,0 +1,49 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<div id="display"></div>
+<textarea id="textarea">abc abx abc</textarea>
+<pre id="test">
+</pre>
+
+<script class="testbody" type="application/javascript">
+SimpleTest.waitForExplicitFinish();
+SimpleTest.waitForFocus(() => {
+  let textarea = document.getElementById("textarea");
+  let editor = SpecialPowers.wrap(textarea).editor;
+
+  let inlineSpellChecker = editor.getInlineSpellChecker(true);
+
+  textarea.focus();
+
+  SpecialPowers.Cu.import(
+    "resource://testing-common/AsyncSpellCheckTestHelper.jsm")
+  .onSpellCheck(textarea, () => {
+    SimpleTest.executeSoon(() => {
+      let misspelledWord = inlineSpellChecker.getMisspelledWord(editor.rootElement.firstChild, 5);
+      is(misspelledWord.startOffset, 4,
+         "Misspelled word should start from 4");
+      is(misspelledWord.endOffset, 7,
+         "Misspelled word should end at 7");
+      inlineSpellChecker.replaceWord(editor.rootElement.firstChild, 5, "aux");
+      is(textarea.value, "abc aux abc",
+         "'abx' should be replaced with 'aux'");
+      synthesizeKey("z", { accelKey: true });
+      is(textarea.value, "abc abx abc",
+         "'abx' should be restored by undo");
+      synthesizeKey("z", { accelKey: true, shiftKey: true });
+      is(textarea.value, "abc aux abc",
+         "'aux' should be restored by redo");
+
+      SimpleTest.finish();
+    });
+  });
+});
+</script>
+</body>
+</html>

extensions/spellcheck/src/mozInlineSpellChecker.cpp

@@ -942,25 +942,12 @@ mozInlineSpellChecker::ReplaceWord(nsINode *aNode, int32_t aOffset,
   nsresult res = GetMisspelledWord(aNode, aOffset, getter_AddRefs(range));
   NS_ENSURE_SUCCESS(res, res);
 
-  if (range)
+  if (!range) {
-  {
+    return NS_OK;
-    // This range was retrieved from the spellchecker selection. As
-    // ranges cannot be shared between selections, we must clone it
-    // before adding it to the editor's selection.
-    RefPtr<nsRange> editorRange = range->CloneRange();
-
-    AutoPlaceholderBatch phb(mTextEditor, nullptr);
-
-    RefPtr<Selection> selection = mTextEditor->GetSelection();
-    NS_ENSURE_TRUE(selection, NS_ERROR_UNEXPECTED);
-    selection->RemoveAllRanges(IgnoreErrors());
-    selection->AddRange(*editorRange, IgnoreErrors());
-
-    MOZ_ASSERT(mTextEditor);
-    DebugOnly<nsresult> rv = mTextEditor->InsertTextAsAction(newword);
-    NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Failed to insert the new word");
   }
 
+  DebugOnly<nsresult> rv = mTextEditor->ReplaceTextAsAction(newword, range);
+  NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Failed to insert the new word");
   return NS_OK;
 }
 

extensions/spellcheck/tests/mochitest/test_bug1170484.html

@@ -14,10 +14,6 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1170484
   /** Test for Bug 1170484 **/
   SimpleTest.waitForExplicitFinish();
 
-  // An assertion is recorded due to nested replacing words from spellchecker but not a problem.
-  // It is necessary to detect invalid method call without event loop.
-  SimpleTest.expectAssertions(1, 1);
-
   SpecialPowers.Cu.import(
     "resource://testing-common/AsyncSpellCheckTestHelper.jsm", window);
 

extensions/spellcheck/tests/mochitest/test_bug1272623.html

@@ -26,10 +26,6 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1272623
 
 /** Test for Bug 1272623 **/
 
-  // 2 assertions are recorded due to nested replacing words from spellchecker but not a problem.
-  // They are necessary to detect invalid method call without event loop.
-  SimpleTest.expectAssertions(2, 2);
-
   async function performCorrection(misspelled, area) {
     synthesizeMouseAtCenter($(misspelled), {}, window);
     await new Promise(resolve => onSpellCheck($(area), resolve));