Bug 1533293 - part 1: Create Selection::SetStartAndEnd() to set new range as far as faster r=smaug
authorMasayuki Nakano <masayuki@d-toybox.com>
Mon, 18 Mar 2019 01:50:59 +0000
changeset 464718 19cff61f4fed314bb596c9b82483d1bc88a246fd
parent 464717 7f712118be5d87b60579a6d26f887891bac92e60
child 464719 e536f6e123d8f54d5bf165e5e78da13c71a901af
push id112470
push useropoprus@mozilla.com
push dateMon, 18 Mar 2019 10:13:23 +0000
treeherdermozilla-inbound@9421b477d67c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1533293
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1533293 - part 1: Create Selection::SetStartAndEnd() to set new range as far as faster r=smaug `Selection::Extend()` is too slow because: - it may create some `nsRange` instances. - it users `nsContentUtils::ComparePoints()` multiple times. Therefore, we can improve the performance if we can stop using it in some places. First, this patch creates `Selection::SetStartAndEnd()` and `Selection::SetStartAndEndInLimiter()` for internal use. They remove current ranges, reuse `nsRange` instance as far as possible and add new range which is set by their arguments. Then, this patch makes `Selection::SelectAllChildren()` stop using `Selection::Extend()`. At this time, this fixes a web-compat issue. `Selection::Expand()` cannot cross the selection limiter boundary when there is a limiter (e.g., when an editing host has focus). But we can now fix this with using the new internal API. Note that methods in editor shouldn't move selection to outside of active editing host. Therefore, this patch adds `Selection::SetStartAndEndInLimiter()` and `Selection::SetBaseAndExtentInLimiter()` for them. Differential Revision: https://phabricator.services.mozilla.com/D23459
dom/base/Selection.cpp
dom/base/Selection.h
editor/libeditor/tests/test_bug430392.html
testing/web-platform/mozilla/meta/focus/Selection_selectAllChildren.html.ini
--- a/dom/base/Selection.cpp
+++ b/dom/base/Selection.cpp
@@ -2758,24 +2758,22 @@ void Selection::SelectAllChildren(nsINod
   if (!HasSameRoot(aNode)) {
     // Return with no error
     return;
   }
 
   if (mFrameSelection) {
     mFrameSelection->PostReason(nsISelectionListener::SELECTALL_REASON);
   }
-  SelectionBatcher batch(this);
-
-  Collapse(aNode, 0, aRv);
-  if (aRv.Failed()) {
-    return;
-  }
-
-  Extend(aNode, aNode.GetChildCount(), aRv);
+
+  // Chrome moves focus when aNode is outside of active editing host.
+  // So, we don't need to respect the limiter with this method.
+  SetStartAndEndInternal(InLimiter::eNo, RawRangeBoundary(&aNode, 0),
+                         RawRangeBoundary(&aNode, aNode.GetChildCount()),
+                         eDirNext, aRv);
 }
 
 bool Selection::ContainsNode(nsINode& aNode, bool aAllowPartial,
                              ErrorResult& aRv) {
   nsresult rv;
   if (mRanges.Length() == 0) {
     return false;
   }
@@ -3385,53 +3383,96 @@ void Selection::Modify(const nsAString& 
 void Selection::SetBaseAndExtentJS(nsINode& aAnchorNode, uint32_t aAnchorOffset,
                                    nsINode& aFocusNode, uint32_t aFocusOffset,
                                    ErrorResult& aRv) {
   AutoRestore<bool> calledFromJSRestorer(mCalledByJS);
   mCalledByJS = true;
   SetBaseAndExtent(aAnchorNode, aAnchorOffset, aFocusNode, aFocusOffset, aRv);
 }
 
-void Selection::SetBaseAndExtent(nsINode& aAnchorNode, uint32_t aAnchorOffset,
-                                 nsINode& aFocusNode, uint32_t aFocusOffset,
-                                 ErrorResult& aRv) {
+void Selection::SetBaseAndExtentInternal(InLimiter aInLimiter,
+                                         const RawRangeBoundary& aAnchorRef,
+                                         const RawRangeBoundary& aFocusRef,
+                                         ErrorResult& aRv) {
   if (!mFrameSelection) {
     return;
   }
 
-  if (!HasSameRoot(aAnchorNode) || !HasSameRoot(aFocusNode)) {
+  if (NS_WARN_IF(!aAnchorRef.IsSet()) || NS_WARN_IF(!aFocusRef.IsSet())) {
+    aRv.Throw(NS_ERROR_INVALID_ARG);
+    return;
+  }
+
+  if (!HasSameRoot(*aAnchorRef.Container()) ||
+      !HasSameRoot(*aFocusRef.Container())) {
     // Return with no error
     return;
   }
 
+  // Prevent "selectionchange" event temporarily because it should be fired
+  // after we set the direction.
+  // XXX If they are disconnected, shouldn't we return error before allocating
+  //     new nsRange instance?
   SelectionBatcher batch(this);
-
-  int32_t relativePosition = nsContentUtils::ComparePoints(
-      &aAnchorNode, aAnchorOffset, &aFocusNode, aFocusOffset);
-  nsINode* start = &aAnchorNode;
-  nsINode* end = &aFocusNode;
-  uint32_t startOffset = aAnchorOffset;
-  uint32_t endOffset = aFocusOffset;
-  if (relativePosition > 0) {
-    start = &aFocusNode;
-    end = &aAnchorNode;
-    startOffset = aFocusOffset;
-    endOffset = aAnchorOffset;
+  if (nsContentUtils::ComparePoints(aAnchorRef, aFocusRef) <= 0) {
+    SetStartAndEndInternal(aInLimiter, aAnchorRef, aFocusRef, eDirNext, aRv);
+    return;
+  }
+
+  SetStartAndEndInternal(aInLimiter, aFocusRef, aAnchorRef, eDirPrevious, aRv);
+}
+
+void Selection::SetStartAndEndInternal(InLimiter aInLimiter,
+                                       const RawRangeBoundary& aStartRef,
+                                       const RawRangeBoundary& aEndRef,
+                                       nsDirection aDirection,
+                                       ErrorResult& aRv) {
+  if (NS_WARN_IF(!aStartRef.IsSet()) || NS_WARN_IF(!aEndRef.IsSet())) {
+    aRv.Throw(NS_ERROR_INVALID_ARG);
+    return;
+  }
+
+  // Don't fire "selectionchange" event until everything done.
+  SelectionBatcher batch(this);
+
+  if (aInLimiter == InLimiter::eYes) {
+    if (!IsValidSelectionPoint(mFrameSelection, aStartRef.Container())) {
+      aRv.Throw(NS_ERROR_FAILURE);
+      return;
+    }
+    if (aStartRef.Container() != aEndRef.Container() &&
+        !IsValidSelectionPoint(mFrameSelection, aEndRef.Container())) {
+      aRv.Throw(NS_ERROR_FAILURE);
+      return;
+    }
+  }
+
+  // If we're not called by JS, we can remove all ranges first.  Then, we
+  // may be able to reuse one of current ranges for reducing the cost of
+  // nsRange allocation.  Note that if this is called by
+  // SetBaseAndExtentJS(), when we fail to initialize new range, we
+  // shouldn't remove current ranges.  Therefore, we need to check whether
+  // we're called by JS or internally.
+  if (!mCalledByJS && !mCachedRange) {
+    nsresult rv = RemoveAllRangesTemporarily();
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      aRv.Throw(rv);
+      return;
+    }
   }
 
   // If there is cached range, we should reuse it for saving the allocation
-  // const (and some other cost in nsRange::DoSetRange().
+  // const (and some other cost in nsRange::DoSetRange()).
   RefPtr<nsRange> newRange = std::move(mCachedRange);
 
   nsresult rv = NS_OK;
   if (newRange) {
-    rv = newRange->SetStartAndEnd(start, startOffset, end, endOffset);
+    rv = newRange->SetStartAndEnd(aStartRef, aEndRef);
   } else {
-    rv = nsRange::CreateRange(start, startOffset, end, endOffset,
-                              getter_AddRefs(newRange));
+    rv = nsRange::CreateRange(aStartRef, aEndRef, getter_AddRefs(newRange));
   }
 
   // nsRange::SetStartAndEnd() and nsRange::CreateRange() returns
   // IndexSizeError if any offset is out of bounds.
   if (NS_FAILED(rv)) {
     aRv.Throw(rv);
     return;
   }
@@ -3441,17 +3482,17 @@ void Selection::SetBaseAndExtent(nsINode
     return;
   }
 
   AddRange(*newRange, aRv);
   if (aRv.Failed()) {
     return;
   }
 
-  SetDirection(relativePosition > 0 ? eDirPrevious : eDirNext);
+  SetDirection(aDirection);
 }
 
 /** SelectionLanguageChange modifies the cursor Bidi level after a change in
  * keyboard direction
  *  @param aLangRTL is true if the new language is right-to-left or false if the
  * new language is left-to-right
  */
 nsresult Selection::SelectionLanguageChange(bool aLangRTL) {
--- a/dom/base/Selection.h
+++ b/dom/base/Selection.h
@@ -344,16 +344,17 @@ class Selection final : public nsSupport
    * @throws NS_ERROR_NOT_IMPLEMENTED if the granularity is "sentence",
    * "sentenceboundary", "paragraph", "paragraphboundary", or
    * "documentboundary".  Throws NS_ERROR_INVALID_ARG if alter, direction,
    * or granularity has an unrecognized value.
    */
   void Modify(const nsAString& aAlter, const nsAString& aDirection,
               const nsAString& aGranularity, mozilla::ErrorResult& aRv);
 
+  MOZ_CAN_RUN_SCRIPT
   void SetBaseAndExtentJS(nsINode& aAnchorNode, uint32_t aAnchorOffset,
                           nsINode& aFocusNode, uint32_t aFocusOffset,
                           mozilla::ErrorResult& aRv);
 
   bool GetInterlinePosition(mozilla::ErrorResult& aRv);
   void SetInterlinePosition(bool aValue, mozilla::ErrorResult& aRv);
 
   Nullable<int16_t> GetCaretBidiLevel(mozilla::ErrorResult& aRv) const;
@@ -439,19 +440,94 @@ class Selection final : public nsSupport
 
   /**
    * Adds all children of the specified node to the selection.
    * @param aNode the parent of the children to be added to the selection.
    */
   MOZ_CAN_RUN_SCRIPT_BOUNDARY
   void SelectAllChildren(nsINode& aNode, mozilla::ErrorResult& aRv);
 
+  /**
+   * SetStartAndEnd() removes all ranges and sets new range as given range.
+   * Different from SetBaseAndExtent(), this won't compare the DOM points of
+   * aStartRef and aEndRef for performance nor set direction to eDirPrevious.
+   * Note that this may reset the limiter and move focus.  If you don't want
+   * that, use SetStartAndEndInLimiter() instead.
+   */
+  MOZ_CAN_RUN_SCRIPT
+  void SetStartAndEnd(const RawRangeBoundary& aStartRef,
+                      const RawRangeBoundary& aEndRef, ErrorResult& aRv) {
+    SetStartAndEndInternal(InLimiter::eNo, aStartRef, aEndRef, eDirNext, aRv);
+  }
+  MOZ_CAN_RUN_SCRIPT
+  void SetStartAndEnd(nsINode& aStartContainer, uint32_t aStartOffset,
+                      nsINode& aEndContainer, uint32_t aEndOffset,
+                      ErrorResult& aRv) {
+    SetStartAndEnd(RawRangeBoundary(&aStartContainer, aStartOffset),
+                   RawRangeBoundary(&aEndContainer, aEndOffset), aRv);
+  }
+
+  /**
+   * SetStartAndEndInLimiter() is similar to SetStartAndEnd(), but this respects
+   * the selection limiter.  If all or part of given range is not in the
+   * limiter, this returns error.
+   */
+  MOZ_CAN_RUN_SCRIPT
+  void SetStartAndEndInLimiter(const RawRangeBoundary& aStartRef,
+                               const RawRangeBoundary& aEndRef,
+                               ErrorResult& aRv) {
+    SetStartAndEndInternal(InLimiter::eYes, aStartRef, aEndRef, eDirNext, aRv);
+  }
+  MOZ_CAN_RUN_SCRIPT
+  void SetStartAndEndInLimiter(nsINode& aStartContainer, uint32_t aStartOffset,
+                               nsINode& aEndContainer, uint32_t aEndOffset,
+                               ErrorResult& aRv) {
+    SetStartAndEndInLimiter(RawRangeBoundary(&aStartContainer, aStartOffset),
+                            RawRangeBoundary(&aEndContainer, aEndOffset), aRv);
+  }
+
+  /**
+   * SetBaseAndExtent() is alternative of the JS API for internal use.
+   * Different from SetStartAndEnd(), this sets anchor and focus points as
+   * specified, then if anchor point is after focus node, this sets the
+   * direction to eDirPrevious.
+   * Note that this may reset the limiter and move focus.  If you don't want
+   * that, use SetBaseAndExtentInLimier() instead.
+   */
+  MOZ_CAN_RUN_SCRIPT
   void SetBaseAndExtent(nsINode& aAnchorNode, uint32_t aAnchorOffset,
                         nsINode& aFocusNode, uint32_t aFocusOffset,
-                        mozilla::ErrorResult& aRv);
+                        ErrorResult& aRv) {
+    SetBaseAndExtent(RawRangeBoundary(&aAnchorNode, aAnchorOffset),
+                     RawRangeBoundary(&aFocusNode, aFocusOffset), aRv);
+  }
+  MOZ_CAN_RUN_SCRIPT
+  void SetBaseAndExtent(const RawRangeBoundary& aAnchorRef,
+                        const RawRangeBoundary& aFocusRef, ErrorResult& aRv) {
+    SetBaseAndExtentInternal(InLimiter::eNo, aAnchorRef, aFocusRef, aRv);
+  }
+
+  /**
+   * SetBaseAndExtentInLimier() is similar to SetBaseAndExtent(), but this
+   * respects the selection limiter.  If all or part of given range is not in
+   * the limiter, this returns error.
+   */
+  MOZ_CAN_RUN_SCRIPT
+  void SetBaseAndExtentInLimiter(nsINode& aAnchorNode, uint32_t aAnchorOffset,
+                                 nsINode& aFocusNode, uint32_t aFocusOffset,
+                                 ErrorResult& aRv) {
+    SetBaseAndExtentInLimiter(RawRangeBoundary(&aAnchorNode, aAnchorOffset),
+                              RawRangeBoundary(&aFocusNode, aFocusOffset), aRv);
+  }
+  MOZ_CAN_RUN_SCRIPT
+  void SetBaseAndExtentInLimiter(const RawRangeBoundary& aAnchorRef,
+                                 const RawRangeBoundary& aFocusRef,
+                                 ErrorResult& aRv) {
+    SetBaseAndExtentInternal(InLimiter::eYes, aAnchorRef, aFocusRef, aRv);
+  }
 
   void AddSelectionChangeBlocker();
   void RemoveSelectionChangeBlocker();
   bool IsBlockingSelectionChangeEvents() const;
 
   // Whether this selection is focused in an editable element.
   bool IsEditorSelection() const;
 
@@ -531,16 +607,35 @@ class Selection final : public nsSupport
                                                nsIFrame** aReturnFrame,
                                                int32_t* aOffsetUsed,
                                                bool aVisual) const;
 
   // Get the cached value for nsTextFrame::GetPointFromOffset.
   nsresult GetCachedFrameOffset(nsIFrame* aFrame, int32_t inOffset,
                                 nsPoint& aPoint);
 
+  enum class InLimiter {
+    // If eYes, the method may reset selection limiter and move focus if the
+    // given range is out of the limiter.
+    eYes,
+    // If eNo, the method won't reset selection limiter.  So, if given range
+    // is out of bounds, the method may return error.
+    eNo,
+  };
+  MOZ_CAN_RUN_SCRIPT
+  void SetStartAndEndInternal(InLimiter aInLimiter,
+                              const RawRangeBoundary& aStartRef,
+                              const RawRangeBoundary& aEndRef,
+                              nsDirection aDirection, ErrorResult& aRv);
+  MOZ_CAN_RUN_SCRIPT
+  void SetBaseAndExtentInternal(InLimiter aInLimiter,
+                                const RawRangeBoundary& aAnchorRef,
+                                const RawRangeBoundary& aFocusRef,
+                                ErrorResult& aRv);
+
  public:
   SelectionType GetType() const { return mSelectionType; }
   void SetType(SelectionType aSelectionType) {
     mSelectionType = aSelectionType;
   }
 
   SelectionCustomColors* GetCustomColors() const { return mCustomColors.get(); }
 
--- a/editor/libeditor/tests/test_bug430392.html
+++ b/editor/libeditor/tests/test_bug430392.html
@@ -33,19 +33,17 @@ function test() {
   // it's neither the original value nor expectedValue.
   var tests = [["adding returns", () => {
     getSelection().collapse(edit.firstChild, 0);
     synthesizeKey("KEY_ArrowRight");
     synthesizeKey("KEY_Enter");
     synthesizeKey("KEY_Enter");
     synthesizeKey("KEY_Backspace");
     synthesizeKey("KEY_Backspace");
-  // For some reason this test fails if the separator is not "br"
-  }, () => document.queryCommandValue("defaultParagraphSeparator") == "br"
-           ? undefined : " A; B ; C "],
+  }],
   ["adding shift-returns", () => {
     getSelection().collapse(edit.firstChild, 0);
     synthesizeKey("KEY_ArrowRight");
     synthesizeKey("KEY_Enter", {shiftKey: true});
     synthesizeKey("KEY_Enter", {shiftKey: true});
     synthesizeKey("KEY_Backspace");
     synthesizeKey("KEY_Backspace");
   }]];
deleted file mode 100644
--- a/testing/web-platform/mozilla/meta/focus/Selection_selectAllChildren.html.ini
+++ /dev/null
@@ -1,41 +0,0 @@
-[Selection_selectAllChildren.html]
-  type: testharness
-  [Active element should be 'editor' after Selection.selectAllChildren() to select the children of 'staticBefore' when active element is 'editor']
-    expected: FAIL
-
-  [Active element should be 'outerEditor' after Selection.selectAllChildren() to select the children of 'outerEditor' when active element is 'editor']
-    expected: FAIL
-
-  [Active element should be 'editor' after Selection.selectAllChildren() to select the children of 'staticInEditor' when active element is 'editor']
-    expected: FAIL
-
-  [Active element should be 'innerEditor' after Selection.selectAllChildren() to select the children of 'innerEditor' when active element is 'editor']
-    expected: FAIL
-
-  [Active element should be 'editor' after Selection.selectAllChildren() to select the children of 'anchor' when active element is 'editor']
-    expected: FAIL
-
-  [Active element should be 'outerEditor' after Selection.selectAllChildren() to select the children of 'staticBefore' when active element is 'outerEditor']
-    expected: FAIL
-
-  [Active element should be 'editor' after Selection.selectAllChildren() to select the children of 'editor' when active element is 'outerEditor']
-    expected: FAIL
-
-  [Active element should be 'outerEditor' after Selection.selectAllChildren() to select the children of 'anchor' when active element is 'outerEditor']
-    expected: FAIL
-
-  [Active element should be 'innerEditor' after Selection.selectAllChildren() to select the children of 'staticBefore' when active element is 'innerEditor']
-    expected: FAIL
-
-  [Active element should be 'editor' after Selection.selectAllChildren() to select the children of 'editor' when active element is 'innerEditor']
-    expected: FAIL
-
-  [Active element should be 'outerEditor' after Selection.selectAllChildren() to select the children of 'outerEditor' when active element is 'innerEditor']
-    expected: FAIL
-
-  [Active element should be 'innerEditor' after Selection.selectAllChildren() to select the children of 'staticInEditor' when active element is 'innerEditor']
-    expected: FAIL
-
-  [Active element should be 'innerEditor' after Selection.selectAllChildren() to select the children of 'anchor' when active element is 'innerEditor']
-    expected: FAIL
-