From 4c53d4eb06913314fc3f48da6dfb2bd2d4de6d53 Mon Sep 17 00:00:00 2001 From: Ryan VanderMeulen Date: Thu, 17 Jun 2021 11:38:53 -0400 Subject: [PATCH] Backed out 3 changesets (bug 1677253) because it depends on bug 1713334 which was backed out. Backed out changeset 3d30a54409a7 (bug 1677253) Backed out changeset 1c385b21e0e5 (bug 1677253) Backed out changeset eb6001404a57 (bug 1677253) --- dom/base/Selection.h | 7 +- dom/base/SelectionChangeEventDispatcher.cpp | 81 +++++---- dom/base/SelectionChangeEventDispatcher.h | 2 - dom/events/EventListenerManager.cpp | 3 +- dom/html/TextControlState.cpp | 74 ++++---- dom/html/test/forms/test_set_range_text.html | 12 +- layout/forms/nsTextControlFrame.cpp | 9 +- .../textfieldselection/select-event.html.ini | 163 ++++++++++++++++++ .../textfieldselection/select-event.html | 12 +- 9 files changed, 273 insertions(+), 90 deletions(-) create mode 100644 testing/web-platform/meta/html/semantics/forms/textfieldselection/select-event.html.ini diff --git a/dom/base/Selection.h b/dom/base/Selection.h index 80317342e221..c7332d045744 100644 --- a/dom/base/Selection.h +++ b/dom/base/Selection.h @@ -935,15 +935,12 @@ class Selection final : public nsSupportsWeakReference, class MOZ_STACK_CLASS SelectionBatcher final { private: RefPtr mSelection; - int16_t mReason; public: explicit SelectionBatcher(Selection& aSelectionRef) : SelectionBatcher(&aSelectionRef) {} - explicit SelectionBatcher(Selection* aSelection, - int16_t aReason = nsISelectionListener::NO_REASON) { + explicit SelectionBatcher(Selection* aSelection) { mSelection = aSelection; - mReason = aReason; if (mSelection) { mSelection->StartBatchChanges(); } @@ -951,7 +948,7 @@ class MOZ_STACK_CLASS SelectionBatcher final { ~SelectionBatcher() { if (mSelection) { - mSelection->EndBatchChanges(mReason); + mSelection->EndBatchChanges(); } } }; diff --git a/dom/base/SelectionChangeEventDispatcher.cpp b/dom/base/SelectionChangeEventDispatcher.cpp index 85b25dd3d6f6..f53637081dfd 100644 --- a/dom/base/SelectionChangeEventDispatcher.cpp +++ b/dom/base/SelectionChangeEventDispatcher.cpp @@ -86,13 +86,12 @@ void SelectionChangeEventDispatcher::OnSelectionChange(Document* aDoc, // Don't bother checking this if we are hiding changes. if (mOldRanges.Length() == aSel->RangeCount() && !aSel->IsBlockingSelectionChangeEvents()) { - bool changed = mOldDirection != aSel->GetDirection(); - if (!changed) { - for (size_t i = 0; i < mOldRanges.Length(); i++) { - if (!mOldRanges[i].Equals(aSel->GetRangeAt(static_cast(i)))) { - changed = true; - break; - } + bool changed = false; + + for (size_t i = 0; i < mOldRanges.Length(); i++) { + if (!mOldRanges[i].Equals(aSel->GetRangeAt(i))) { + changed = true; + break; } } @@ -106,7 +105,6 @@ void SelectionChangeEventDispatcher::OnSelectionChange(Document* aDoc, for (size_t i = 0; i < aSel->RangeCount(); i++) { mOldRanges.AppendElement(RawRangeData(aSel->GetRangeAt(i))); } - mOldDirection = aSel->GetDirection(); if (doc) { nsPIDOMWindowInner* inner = doc->GetInnerWindow(); @@ -122,38 +120,53 @@ void SelectionChangeEventDispatcher::OnSelectionChange(Document* aDoc, return; } - nsCOMPtr textControl; - if (const nsFrameSelection* fs = aSel->GetFrameSelection()) { - if (nsCOMPtr root = fs->GetLimiter()) { - textControl = root->GetClosestNativeAnonymousSubtreeRootParent(); - MOZ_ASSERT_IF(textControl, - textControl->IsTextControlElement() && - !textControl->IsInNativeAnonymousSubtree()); - } - } - - // Selection changes with non-JS reason only cares about whether the new - // selection is collapsed or not. See TextInputListener::OnSelectionChange. - if (textControl && (aReason & nsISelectionListener::JS_REASON)) { - RefPtr asyncDispatcher = - new AsyncEventDispatcher(textControl, eFormSelect, CanBubble::eYes); - asyncDispatcher->PostDOMEvent(); - } - // The spec currently doesn't say that we should dispatch this event on text // controls, so for now we only support doing that under a pref, disabled by // default. // See https://github.com/w3c/selection-api/issues/53. - if (textControl && !StaticPrefs::dom_select_events_textcontrols_enabled()) { - return; - } + if (StaticPrefs::dom_select_events_textcontrols_enabled()) { + nsCOMPtr target; - nsCOMPtr target = textControl ? textControl : aDoc; + // Check if we should be firing this event to a different node than the + // document. The limiter of the nsFrameSelection will be within the native + // anonymous subtree of the node we want to fire the event on. We need to + // climb up the parent chain to escape the native anonymous subtree, and + // then fire the event. + if (const nsFrameSelection* fs = aSel->GetFrameSelection()) { + if (nsCOMPtr root = fs->GetLimiter()) { + while (root && root->IsInNativeAnonymousSubtree()) { + root = root->GetParent(); + } - if (target) { - RefPtr asyncDispatcher = - new AsyncEventDispatcher(target, eSelectionChange, CanBubble::eNo); - asyncDispatcher->PostDOMEvent(); + target = std::move(root); + } + } + + // If we didn't get a target before, we can instead fire the event at the + // document. + if (!target) { + target = aDoc; + } + + if (target) { + RefPtr asyncDispatcher = + new AsyncEventDispatcher(target, eSelectionChange, CanBubble::eNo); + asyncDispatcher->PostDOMEvent(); + } + } else { + if (const nsFrameSelection* fs = aSel->GetFrameSelection()) { + if (nsCOMPtr root = fs->GetLimiter()) { + if (root->IsInNativeAnonymousSubtree()) { + return; + } + } + } + + if (aDoc) { + RefPtr asyncDispatcher = + new AsyncEventDispatcher(aDoc, eSelectionChange, CanBubble::eNo); + asyncDispatcher->PostDOMEvent(); + } } } diff --git a/dom/base/SelectionChangeEventDispatcher.h b/dom/base/SelectionChangeEventDispatcher.h index a734b4490d63..8f94523d8ade 100644 --- a/dom/base/SelectionChangeEventDispatcher.h +++ b/dom/base/SelectionChangeEventDispatcher.h @@ -11,7 +11,6 @@ #include "nsCycleCollectionParticipant.h" #include "nsTArray.h" #include "nsCOMPtr.h" -#include "nsDirection.h" class nsINode; class nsRange; @@ -59,7 +58,6 @@ class SelectionChangeEventDispatcher final { private: nsTArray mOldRanges; - nsDirection mOldDirection; ~SelectionChangeEventDispatcher() = default; }; diff --git a/dom/events/EventListenerManager.cpp b/dom/events/EventListenerManager.cpp index e65a86622107..76ec743ad4c6 100644 --- a/dom/events/EventListenerManager.cpp +++ b/dom/events/EventListenerManager.cpp @@ -370,8 +370,7 @@ void EventListenerManager::AddEventListenerInternal( if (!aFlags.mInSystemGroup) { mMayHaveInputOrCompositionEventListener = true; } - } else if (aEventMessage == eSelectionChange || - aEventMessage == eFormSelect) { + } else if (aEventMessage == eSelectionChange) { mMayHaveSelectionChangeEventListener = true; if (nsPIDOMWindowInner* window = GetInnerWindowForTarget()) { window->SetHasSelectionChangeEventListeners(); diff --git a/dom/html/TextControlState.cpp b/dom/html/TextControlState.cpp index 7089f88be6e6..8d2f1f0ab8c6 100644 --- a/dom/html/TextControlState.cpp +++ b/dom/html/TextControlState.cpp @@ -2041,7 +2041,20 @@ void TextControlState::SetSelectionRange( aStart = aEnd; } - if (!IsSelectionCached()) { + bool changed = false; + nsresult rv = NS_OK; // For the ScrollSelectionIntoView() return value. + if (IsSelectionCached()) { + SelectionProperties& props = GetSelectionProperties(); + if (!props.HasMaxLength()) { + // A clone without a dirty value flag may not have a max length yet + nsAutoString value; + GetValue(value, false); + props.SetMaxLength(value.Length()); + } + changed |= props.SetStart(aStart); + changed |= props.SetEnd(aEnd); + changed |= props.SetDirection(aDirection); + } else { MOZ_ASSERT(mBoundFrame, "Our frame should still be valid"); aRv = mBoundFrame->SetSelectionRange(aStart, aEnd, aDirection); if (aRv.Failed() || @@ -2053,38 +2066,36 @@ void TextControlState::SetSelectionRange( // example. mBoundFrame->ScrollSelectionIntoViewAsync(); } - return; + // Press on to firing the event even if that failed, like our old code did. + // But is that really what we want? Firing the event _and_ throwing from + // here is weird. Maybe we should just ignore ScrollSelectionIntoView + // failures? + + // XXXbz This is preserving our current behavior of firing a "select" event + // on all mutations when we have an editor, but we should really consider + // fixing that... + changed = true; } - SelectionProperties& props = GetSelectionProperties(); - if (!props.HasMaxLength()) { - // A clone without a dirty value flag may not have a max length yet - nsAutoString value; - GetValue(value, false); - props.SetMaxLength(value.Length()); - } - - bool changed = props.SetStart(aStart); - changed |= props.SetEnd(aEnd); - changed |= props.SetDirection(aDirection); - - if (!changed) { - return; - } - - // It sure would be nice if we had an existing Element* or so to work with. - RefPtr asyncDispatcher = - new AsyncEventDispatcher(mTextCtrlElement, eFormSelect, CanBubble::eYes); - asyncDispatcher->PostDOMEvent(); - - // SelectionChangeEventDispatcher covers this when !IsSelectionCached(). - // XXX(krosylight): Shouldn't it fire before select event? - // Currently Gecko and Blink both fire selectionchange after select. - if (IsSelectionCached() && - StaticPrefs::dom_select_events_textcontrols_enabled()) { - asyncDispatcher = new AsyncEventDispatcher( - mTextCtrlElement, eSelectionChange, CanBubble::eNo); + if (changed) { + // It sure would be nice if we had an existing Element* or so to work with. + RefPtr asyncDispatcher = new AsyncEventDispatcher( + mTextCtrlElement, eFormSelect, CanBubble::eYes); asyncDispatcher->PostDOMEvent(); + + // SelectionChangeEventDispatcher covers this when !IsSelectionCached(). + // XXX(krosylight): Shouldn't it fire before select event? + // Currently Gecko and Blink both fire selectionchange after select. + if (IsSelectionCached() && + StaticPrefs::dom_select_events_textcontrols_enabled()) { + asyncDispatcher = new AsyncEventDispatcher( + mTextCtrlElement, eSelectionChange, CanBubble::eNo); + asyncDispatcher->PostDOMEvent(); + } + } + + if (NS_FAILED(rv)) { + aRv.Throw(rv); } } @@ -2266,8 +2277,7 @@ void TextControlState::SetRangeText(const nsAString& aReplacement, // Batch selectionchanges from SetValueFromSetRangeText and SetSelectionRange Selection* selection = mSelCon ? mSelCon->GetSelection(SelectionType::eNormal) : nullptr; - SelectionBatcher selectionBatcher( - selection, nsISelectionListener::JS_REASON); // no-op if nullptr + SelectionBatcher selectionBatcher(selection); // no-op if nullptr MOZ_ASSERT(aStart <= aEnd); value.Replace(aStart, aEnd - aStart, aReplacement); diff --git a/dom/html/test/forms/test_set_range_text.html b/dom/html/test/forms/test_set_range_text.html index f85014ae77a7..901f54bb83ad 100644 --- a/dom/html/test/forms/test_set_range_text.html +++ b/dom/html/test/forms/test_set_range_text.html @@ -72,6 +72,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=850364 elem.focus(); try { elem.setRangeText("abc"); + expectedNumOfSelectCalls += 1; } catch (ex) { opThrows = true; } @@ -99,7 +100,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=850364 is(elem.selectionEnd, 4, msg + test); elem.setRangeText("mnk"); is(elem.value, "0mnk6789ABCDEF", msg + test); - expectedNumOfSelectCalls += 2; + expectedNumOfSelectCalls += 3; test = " setRange(replacement), expand"; elem.value = "0123456789ABCDEF"; @@ -110,7 +111,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=850364 is(elem.selectionEnd, 4, msg + test); elem.setRangeText("mnk"); is(elem.value, "0mnk23456789ABCDEF", msg + test); - expectedNumOfSelectCalls += 2; + expectedNumOfSelectCalls += 3; test = " setRange(replacement) pure insertion at start"; elem.value = "0123456789ABCDEF"; @@ -121,7 +122,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=850364 is(elem.selectionEnd, 0, msg + test); elem.setRangeText("mnk"); is(elem.value, "mnkxyz0123456789ABCDEF", msg + test); - expectedNumOfSelectCalls += 1; + expectedNumOfSelectCalls += 3; test = " setRange(replacement) pure insertion in the middle"; elem.value = "0123456789ABCDEF"; @@ -132,7 +133,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=850364 is(elem.selectionEnd, 4, msg + test); elem.setRangeText("mnk"); is(elem.value, "0123mnkxyz456789ABCDEF", msg + test); - expectedNumOfSelectCalls += 1; + expectedNumOfSelectCalls += 3; test = " setRange(replacement) pure insertion at the end"; elem.value = "0123456789ABCDEF"; @@ -143,6 +144,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=850364 is(elem.selectionEnd, 16, msg + test); elem.setRangeText("mnk"); is(elem.value, "0123456789ABCDEFmnkxyz", msg + test); + expectedNumOfSelectCalls += 3; //test SetRange(replacement, start, end, mode) with start > end try { @@ -203,7 +205,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=850364 is(elem.value, "0Z23456789", msg + ".value == \"0Z23456789\""); is(elem.selectionStart, 6, msg + ".selectionStart == 6, with \"preserve\""); is(elem.selectionEnd, 9, msg + ".selectionEnd == 9, with \"preserve\""); - expectedNumOfSelectCalls += 1; + expectedNumOfSelectCalls += 2; //subcase: selection{Start|End} < end elem.value = "0123456789"; diff --git a/layout/forms/nsTextControlFrame.cpp b/layout/forms/nsTextControlFrame.cpp index 9933caf62868..be7c94c1951d 100644 --- a/layout/forms/nsTextControlFrame.cpp +++ b/layout/forms/nsTextControlFrame.cpp @@ -876,9 +876,12 @@ nsresult nsTextControlFrame::SetSelectionInternal( direction = (aDirection == eBackward) ? eDirPrevious : eDirNext; } - MOZ_TRY(selection->SetStartAndEndInLimiter(*aStartNode, aStartOffset, - *aEndNode, aEndOffset, direction, - nsISelectionListener::JS_REASON)); + ErrorResult error; + selection->SetStartAndEndInLimiter(*aStartNode, aStartOffset, *aEndNode, + aEndOffset, error); + MOZ_TRY(error.StealNSResult()); + + selection->SetDirection(direction); return NS_OK; } diff --git a/testing/web-platform/meta/html/semantics/forms/textfieldselection/select-event.html.ini b/testing/web-platform/meta/html/semantics/forms/textfieldselection/select-event.html.ini new file mode 100644 index 000000000000..f9740bce32a6 --- /dev/null +++ b/testing/web-platform/meta/html/semantics/forms/textfieldselection/select-event.html.ini @@ -0,0 +1,163 @@ +[select-event.html] + [textarea: select() a second time (must not fire select)] + expected: FAIL + + [textarea: selectionStart a second time (must not fire select)] + expected: FAIL + + [textarea: selectionEnd a second time (must not fire select)] + expected: FAIL + + [textarea: selectionDirection a second time (must not fire select)] + expected: FAIL + + [textarea: setSelectionRange() a second time (must not fire select)] + expected: FAIL + + [textarea: setRangeText() a second time (must not fire select)] + expected: FAIL + + [input type text: select() a second time (must not fire select)] + expected: FAIL + + [input type text: selectionStart a second time (must not fire select)] + expected: FAIL + + [input type text: selectionEnd a second time (must not fire select)] + expected: FAIL + + [input type text: selectionDirection a second time (must not fire select)] + expected: FAIL + + [input type text: setSelectionRange() a second time (must not fire select)] + expected: FAIL + + [input type text: setRangeText() a second time (must not fire select)] + expected: FAIL + + [input type search: select() a second time (must not fire select)] + expected: FAIL + + [input type search: selectionStart a second time (must not fire select)] + expected: FAIL + + [input type search: selectionEnd a second time (must not fire select)] + expected: FAIL + + [input type search: selectionDirection a second time (must not fire select)] + expected: FAIL + + [input type search: setSelectionRange() a second time (must not fire select)] + expected: FAIL + + [input type search: setRangeText() a second time (must not fire select)] + expected: FAIL + + [input type tel: select() a second time (must not fire select)] + expected: FAIL + + [input type tel: selectionStart a second time (must not fire select)] + expected: FAIL + + [input type tel: selectionEnd a second time (must not fire select)] + expected: FAIL + + [input type tel: selectionDirection a second time (must not fire select)] + expected: FAIL + + [input type tel: setSelectionRange() a second time (must not fire select)] + expected: FAIL + + [input type tel: setRangeText() a second time (must not fire select)] + expected: FAIL + + [input type url: select() a second time (must not fire select)] + expected: FAIL + + [input type url: selectionStart a second time (must not fire select)] + expected: FAIL + + [input type url: selectionEnd a second time (must not fire select)] + expected: FAIL + + [input type url: selectionDirection a second time (must not fire select)] + expected: FAIL + + [input type url: setSelectionRange() a second time (must not fire select)] + expected: FAIL + + [input type url: setRangeText() a second time (must not fire select)] + expected: FAIL + + [input type password: select() a second time (must not fire select)] + expected: FAIL + + [input type password: selectionStart a second time (must not fire select)] + expected: FAIL + + [input type password: selectionEnd a second time (must not fire select)] + expected: FAIL + + [input type password: selectionDirection a second time (must not fire select)] + expected: FAIL + + [input type password: setSelectionRange() a second time (must not fire select)] + expected: FAIL + + [input type password: setRangeText() a second time (must not fire select)] + expected: FAIL + + [textarea: selectionStart out of range a second time (must not fire select)] + expected: FAIL + + [textarea: selectionEnd out of range a second time (must not fire select)] + expected: FAIL + + [textarea: setSelectionRange out of range a second time (must not fire select)] + expected: FAIL + + [input type text: selectionStart out of range a second time (must not fire select)] + expected: FAIL + + [input type text: selectionEnd out of range a second time (must not fire select)] + expected: FAIL + + [input type text: setSelectionRange out of range a second time (must not fire select)] + expected: FAIL + + [input type search: selectionStart out of range a second time (must not fire select)] + expected: FAIL + + [input type search: selectionEnd out of range a second time (must not fire select)] + expected: FAIL + + [input type search: setSelectionRange out of range a second time (must not fire select)] + expected: FAIL + + [input type tel: selectionStart out of range a second time (must not fire select)] + expected: FAIL + + [input type tel: selectionEnd out of range a second time (must not fire select)] + expected: FAIL + + [input type tel: setSelectionRange out of range a second time (must not fire select)] + expected: FAIL + + [input type url: selectionStart out of range a second time (must not fire select)] + expected: FAIL + + [input type url: selectionEnd out of range a second time (must not fire select)] + expected: FAIL + + [input type url: setSelectionRange out of range a second time (must not fire select)] + expected: FAIL + + [input type password: selectionStart out of range a second time (must not fire select)] + expected: FAIL + + [input type password: selectionEnd out of range a second time (must not fire select)] + expected: FAIL + + [input type password: setSelectionRange out of range a second time (must not fire select)] + expected: FAIL + diff --git a/testing/web-platform/tests/html/semantics/forms/textfieldselection/select-event.html b/testing/web-platform/tests/html/semantics/forms/textfieldselection/select-event.html index cb54b64592e1..09626a5a5cef 100644 --- a/testing/web-platform/tests/html/semantics/forms/textfieldselection/select-event.html +++ b/testing/web-platform/tests/html/semantics/forms/textfieldselection/select-event.html @@ -59,12 +59,10 @@ const actions = [ } ]; - function initialize(el, t) { - el.setRangeText("foobar", 0, el.value.length, "start"); - // Make sure to flush async dispatches - return new Promise(resolve => { - t.step_timeout(resolve, 200); - }); +function initialize(el) { + el.value = "foobar"; + el.setSelectionRange(0, 0); + return new Promise(requestAnimationFrame); } els.forEach((el) => { @@ -74,7 +72,7 @@ els.forEach((el) => { // promise_test instead of async_test is important because these need to happen in sequence (to test that events // fire if and only if the selection changes). promise_test(async t => { - await initialize(el, t); + await initialize(el); const watcher = new EventWatcher(t, el, "select");