From d4cbc09db866cbc55a8ec7b9aa1a13e35b099e87 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Mon, 18 Mar 2019 01:50:59 +0000 Subject: [PATCH] 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 --HG-- extra : moz-landing-system : lando --- dom/base/Selection.cpp | 95 ++++++++++++------ dom/base/Selection.h | 97 ++++++++++++++++++- editor/libeditor/tests/test_bug430392.html | 4 +- .../Selection_selectAllChildren.html.ini | 41 -------- 4 files changed, 165 insertions(+), 72 deletions(-) delete mode 100644 testing/web-platform/mozilla/meta/focus/Selection_selectAllChildren.html.ini diff --git a/dom/base/Selection.cpp b/dom/base/Selection.cpp index 0a899e17d882..a2e6d9cdfefc 100644 --- a/dom/base/Selection.cpp +++ b/dom/base/Selection.cpp @@ -2763,14 +2763,12 @@ void Selection::SelectAllChildren(nsINode& aNode, ErrorResult& aRv) { 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, @@ -3390,43 +3388,86 @@ void Selection::SetBaseAndExtentJS(nsINode& aAnchorNode, uint32_t aAnchorOffset, 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); + 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); - 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 (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 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 @@ -3446,7 +3487,7 @@ void Selection::SetBaseAndExtent(nsINode& aAnchorNode, uint32_t aAnchorOffset, return; } - SetDirection(relativePosition > 0 ? eDirPrevious : eDirNext); + SetDirection(aDirection); } /** SelectionLanguageChange modifies the cursor Bidi level after a change in diff --git a/dom/base/Selection.h b/dom/base/Selection.h index 051b2ae8dac0..a3c029ad49f6 100644 --- a/dom/base/Selection.h +++ b/dom/base/Selection.h @@ -349,6 +349,7 @@ class Selection final : public nsSupportsWeakReference, 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); @@ -444,9 +445,84 @@ class Selection final : public nsSupportsWeakReference, 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(); @@ -536,6 +612,25 @@ class Selection final : public nsSupportsWeakReference, 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) { diff --git a/editor/libeditor/tests/test_bug430392.html b/editor/libeditor/tests/test_bug430392.html index 57504fff932f..85ebb3e219e2 100644 --- a/editor/libeditor/tests/test_bug430392.html +++ b/editor/libeditor/tests/test_bug430392.html @@ -38,9 +38,7 @@ function test() { 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"); diff --git a/testing/web-platform/mozilla/meta/focus/Selection_selectAllChildren.html.ini b/testing/web-platform/mozilla/meta/focus/Selection_selectAllChildren.html.ini deleted file mode 100644 index 3aaf0a201a09..000000000000 --- 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 -