Closed Bug 1713334 Opened 3 years ago Closed 3 years ago

setRangeText unexpectedly fires selectionchange when the text control was empty

Categories

(Core :: DOM: Selection, defect, P3)

defect

Tracking

()

RESOLVED FIXED
91 Branch
Tracking Status
firefox91 --- fixed

People

(Reporter: saschanaz, Assigned: saschanaz)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Crash Data

Attachments

(6 files)

<!DOCTYPE html>
<meta charset="utf-8">
<input id=text>
<script>
  text.addEventListener("selectionchange", console.log);
  text.focus();
  text.setRangeText("abc");
</script>

Expected: No selectionchange, since it does not change selectionStart/End
Actual: It fires one

This is because initially the internal focusNode is the internal <div> and then it moves to the text node.

Doing so prevents confusion when detecting selection change.

Severity: -- → S3
Priority: -- → P3
Attachment #9224097 - Attachment description: Bug 1713334 - Keep a text node inside text controls even if empty r=masayuki → Bug 1713334 - Part 1: Keep a text node inside text controls even if empty r=masayuki
Blocks: 1677253
No longer blocks: 1648944

I tried updating SelectEntireDocument to select within text node by SelectionRef().SetStartAndEnd(*text, 0, *text, text->Length(), error); to fix a crash, and then it broke some clipboard tests only on Linux:

https://treeherder.mozilla.org/jobs?repo=try&revision=a46132a4753bda91a49da1544705f93b8b169107&selectedTaskRun=AFqkiXvqRYGKuKuf5sP3rg.0

[task 2021-06-08T23:49:57.351Z] 23:49:57     INFO - Initializing clipboard with "waitForClipboard-known-value-0.6211546214201118"...
[task 2021-06-08T23:49:57.351Z] 23:49:57     INFO - Succeeded initializing clipboard, start requested things...
[task 2021-06-08T23:49:57.351Z] 23:49:57     INFO - TEST-PASS | editor/libeditor/tests/test_middle_click_paste.html | Clipboard has the given value: 'abc
[task 2021-06-08T23:49:57.351Z] 23:49:57     INFO - def
[task 2021-06-08T23:49:57.351Z] 23:49:57     INFO - ghi' 
[task 2021-06-08T23:49:57.351Z] 23:49:57     INFO - TEST-PASS | editor/libeditor/tests/test_middle_click_paste.html | Succeeded to copy "abc
[task 2021-06-08T23:49:57.351Z] 23:49:57     INFO - def
[task 2021-06-08T23:49:57.351Z] 23:49:57     INFO - ghi" to clipboard 
[task 2021-06-08T23:49:57.351Z] 23:49:57     INFO - Buffered messages finished
[task 2021-06-08T23:49:57.351Z] 23:49:57     INFO - TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_middle_click_paste.html | Pasted each line should start with "> " - got "> waitForClipboard-known-value-0.6211546214201118\n\n", expected "> abc\n> def\n> ghi\n\n"
[task 2021-06-08T23:49:57.351Z] 23:49:57     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:500:14
[task 2021-06-08T23:49:57.351Z] 23:49:57     INFO -     doTextareaTests@editor/libeditor/tests/test_middle_click_paste.html:138:5

I'm not sure why it succeeds to get the new value and then emits the previous value, do you see why?

Flags: needinfo?(masayuki)

According to the test log, once the copy succeeded immediately before the failure, but then, pasted content is back to its previous content (i.e., initialized random text before the copy). So, it seems that there is a race which shouldn't be.

The failure must be here:
https://searchfox.org/mozilla-central/rev/c0f286b1f541c675bbe052b21bdefa80d150ec35/editor/libeditor/tests/test_middle_click_paste.html#138-140

The clipboard content for this test is initialized here:
https://searchfox.org/mozilla-central/rev/c0f286b1f541c675bbe052b21bdefa80d150ec35/editor/libeditor/tests/test_middle_click_paste.html#133

It simply uses SpecialPowers' API with selecting a <textarea> with select all:
https://searchfox.org/mozilla-central/rev/c0f286b1f541c675bbe052b21bdefa80d150ec35/editor/libeditor/tests/test_middle_click_paste.html#31-44

According to:
https://searchfox.org/mozilla-central/rev/c0f286b1f541c675bbe052b21bdefa80d150ec35/widget/nsClipboardProxy.cpp#51
and
https://searchfox.org/mozilla-central/rev/c0f286b1f541c675bbe052b21bdefa80d150ec35/dom/ipc/PContent.ipdl#1140
, SpecialPowers scans the clipboard data synchronously.
https://searchfox.org/mozilla-central/rev/c0f286b1f541c675bbe052b21bdefa80d150ec35/testing/mochitest/tests/SimpleTest/SimpleTest.js#1191

On the other hand, TextEditor::PasteAsQuotationAsAction() also uses nsIClipboard::GetData() here:
https://searchfox.org/mozilla-central/rev/c0f286b1f541c675bbe052b21bdefa80d150ec35/editor/libeditor/TextEditor.cpp#485

So, I have no idea why the clipboard data is restored...

Olli, you did a lot of reviews at implementing e10s around clipboard. Do you know who is familiar with current design?

Flags: needinfo?(masayuki) → needinfo?(bugs)

It seems those tests depend on AutoCopyListener for "primary selection clipboard" (Linux-only) and thus needs nsISelectionListener::SELECTALL_REASON to be updated. It's very weird that the general clipboard is checked and then the other one is actually used.

Flags: needinfo?(bugs)
Attachment #9225620 - Attachment description: Bug 1713334 - Part 4: Allow caret to be at the end of text node r=masayuki → Bug 1713334 - Part 5: Allow caret to be at the end of text node r=masayuki
Pushed by krosylight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/78b0cdebc890 Part 1: Keep a text node inside text controls even if empty r=masayuki,Jamie https://hg.mozilla.org/integration/autoland/rev/875b520387b8 Part 2: Initialize TextEditor always with a text node r=masayuki https://hg.mozilla.org/integration/autoland/rev/858410646bf7 Part 3: Assume TextEditor always have a text node r=masayuki https://hg.mozilla.org/integration/autoland/rev/f4032094748d Part 4: Always select the text node r=masayuki https://hg.mozilla.org/integration/autoland/rev/ab7b2061f5e8 Part 5: Allow caret to be at the end of text node r=masayuki
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/29359 for changes under testing/web-platform/tests

Backed out for failures on test_texteditor_keyevent_handling.html

[task 2021-06-14T17:38:25.174Z] 17:38:25 INFO - TEST-OK | editor/libeditor/tests/test_texteditor_keyevent_handling.html | took 714ms
[task 2021-06-14T17:38:25.175Z] 17:38:25 INFO - GECKO(2220) | [Parent 2220, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005 (NS_ERROR_FAILURE): file /builds/worker/checkouts/gecko/chrome/nsChromeRegistry.cpp:180
[task 2021-06-14T17:38:25.175Z] 17:38:25 INFO - GECKO(2220) | [Parent 2220, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005 (NS_ERROR_FAILURE): file /builds/worker/checkouts/gecko/dom/security/nsCSPService.cpp:191
[task 2021-06-14T17:38:25.176Z] 17:38:25 INFO - GECKO(2220) | [Parent 2220, Main Thread] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /builds/worker/checkouts/gecko/parser/html/nsHtml5StreamParser.cpp:1449
[task 2021-06-14T17:38:25.176Z] 17:38:25 INFO - TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_texteditor_keyevent_handling.html | assertion count 10 is more than expected 0 assertions
[task 2021-06-14T17:38:25.176Z] 17:38:25 INFO - TEST-START | Shutdown

After

[task 2021-06-14T17:38:25.138Z] 17:38:25     INFO - GECKO(2220) | [Parent 2220, Main Thread] ###!!! ASSERTION: For avoiding to throw incompatible exception for `execCommand`, fix the caller: 'false', file /builds/worker/checkouts/gecko/editor/libeditor/EditorBase.cpp:4890
[task 2021-06-14T17:38:25.138Z] 17:38:25     INFO - GECKO(2220) | #01: NS_DebugBreak [xpcom/base/nsDebugImpl.cpp:431]
[task 2021-06-14T17:38:25.139Z] 17:38:25     INFO - GECKO(2220) | #02: mozilla::EditorBase::DeleteRangesWithTransaction(short, short, mozilla::AutoRangeArray const&) [editor/libeditor/EditorBase.cpp:4898]
[task 2021-06-14T17:38:25.139Z] 17:38:25     INFO - GECKO(2220) | #03: mozilla::TextEditor::HandleDeleteSelectionInternal(short, short) [editor/libeditor/TextEditSubActionHandler.cpp:683]
[task 2021-06-14T17:38:25.140Z] 17:38:25     INFO - GECKO(2220) | #04: mozilla::TextEditor::HandleDeleteSelection(short, short) [editor/libeditor/TextEditSubActionHandler.cpp:615]
[task 2021-06-14T17:38:25.140Z] 17:38:25     INFO - GECKO(2220) | #05: mozilla::EditorBase::DeleteSelectionAsSubAction(short, short) [editor/libeditor/EditorBase.cpp:4495]
[task 2021-06-14T17:38:25.140Z] 17:38:25     INFO - GECKO(2220) | #06: mozilla::EditorBase::DeleteSelectionAsAction(short, short, nsIPrincipal*) [editor/libeditor/EditorBase.cpp:4459]
[task 2021-06-14T17:38:25.141Z] 17:38:25     INFO - GECKO(2220) | #07: mozilla::DeleteCommand::DoCommand(mozilla::Command, mozilla::EditorBase&, nsIPrincipal*) const [editor/libeditor/EditorCommands.cpp:619]
[task 2021-06-14T17:38:25.141Z] 17:38:25     INFO - GECKO(2220) | #08: mozilla::EditorCommand::DoCommand(char const*, nsISupports*) [editor/libeditor/EditorCommands.cpp:65]
[task 2021-06-14T17:38:25.142Z] 17:38:25     INFO - GECKO(2220) | #09: nsControllerCommandTable::DoCommand(char const*, nsISupports*) [dom/commandhandler/nsControllerCommandTable.cpp:138]
[task 2021-06-14T17:38:25.142Z] 17:38:25     INFO - GECKO(2220) | #10: nsBaseCommandController::DoCommand(char const*) [dom/commandhandler/nsBaseCommandController.cpp:114]
[task 2021-06-14T17:38:25.142Z] 17:38:25     INFO - GECKO(2220) | #11: mozilla::DoCommandCallback(mozilla::Command, void*) [dom/html/TextControlState.cpp:912]
[task 2021-06-14T17:38:25.143Z] 17:38:25     INFO - GECKO(2220) | #12: mozilla::WidgetKeyboardEvent::ExecuteEditCommands(nsIWidget::NativeKeyBindingsType, void (*)(mozilla::Command, void*), void*) [widget/WidgetEventImpl.cpp:901]
[task 2021-06-14T17:38:25.143Z] 17:38:25     INFO - GECKO(2220) | #13: mozilla::TextInputListener::HandleEvent(mozilla::dom::Event*) [dom/html/TextControlState.cpp:993]
[task 2021-06-14T17:38:25.144Z] 17:38:25     INFO - GECKO(2220) | #14: mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) [dom/events/EventListenerManager.cpp:1114]
[task 2021-06-14T17:38:25.144Z] 17:38:25     INFO - GECKO(2220) | #15: mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) [dom/events/EventListenerManager.cpp:1307]
[task 2021-06-14T17:38:25.145Z] 17:38:25     INFO - GECKO(2220) | #16: mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) [dom/events/EventDispatcher.cpp:358]
[task 2021-06-14T17:38:25.145Z] 17:38:25     INFO - GECKO(2220) | #17: mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) [dom/events/EventDispatcher.cpp:559]
[task 2021-06-14T17:38:25.146Z] 17:38:25     INFO - GECKO(2220) | #18: mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) [dom/events/EventDispatcher.cpp:638]
[task 2021-06-14T17:38:25.146Z] 17:38:25     INFO - GECKO(2220) | #19: mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) [dom/events/EventDispatcher.cpp:1116]
[task 2021-06-14T17:38:25.147Z] 17:38:25     INFO - GECKO(2220) | #20: mozilla::PresShell::EventHandler::DispatchEventToDOM(mozilla::WidgetEvent*, nsEventStatus*, nsPresShellEventCB*) [layout/base/PresShell.cpp:8673]
[task 2021-06-14T17:38:25.147Z] 17:38:25     INFO - GECKO(2220) | #21: mozilla::PresShell::EventHandler::DispatchEvent(mozilla::EventStateManager*, mozilla::WidgetEvent*, bool, nsEventStatus*, nsIContent*) [layout/base/PresShell.cpp:8251]
[task 2021-06-14T17:38:25.148Z] 17:38:25     INFO - GECKO(2220) | #22: mozilla::PresShell::EventHandler::HandleEventWithCurrentEventInfo(mozilla::WidgetEvent*, nsEventStatus*, bool, nsIContent*) [layout/base/PresShell.cpp:8184]
[task 2021-06-14T17:38:25.148Z] 17:38:25     INFO - GECKO(2220) | #23: mozilla::PresShell::EventHandler::HandleEventAtFocusedContent(mozilla::WidgetGUIEvent*, nsEventStatus*) [layout/base/PresShell.cpp:7913]
[task 2021-06-14T17:38:25.148Z] 17:38:25     INFO - GECKO(2220) | #24: mozilla::PresShell::EventHandler::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) [layout/base/PresShell.cpp:6929]
[task 2021-06-14T17:38:25.149Z] 17:38:25     INFO - GECKO(2220) | #25: mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) [layout/base/PresShell.cpp:6829]
[task 2021-06-14T17:38:25.149Z] 17:38:25     INFO - GECKO(2220) | #26: mozilla::PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell(nsIFrame*, mozilla::WidgetGUIEvent*, nsEventStatus*, nsresult*) [layout/base/PresShell.cpp:7584]
[task 2021-06-14T17:38:25.150Z] 17:38:25     INFO - GECKO(2220) | #27: mozilla::PresShell::EventHandler::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) [layout/base/PresShell.cpp:6891]
[task 2021-06-14T17:38:25.150Z] 17:38:25     INFO - GECKO(2220) | #28: mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) [layout/base/PresShell.cpp:6829]
[task 2021-06-14T17:38:25.151Z] 17:38:25     INFO - GECKO(2220) | #29: nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) [view/nsViewManager.cpp:703]
[task 2021-06-14T17:38:25.151Z] 17:38:25     INFO - GECKO(2220) | #30: nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) [view/nsView.cpp:1137]
[task 2021-06-14T17:38:25.151Z] 17:38:25     INFO - GECKO(2220) | #31: nsChildView::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) [widget/cocoa/nsChildView.mm:1273]
[task 2021-06-14T17:38:25.152Z] 17:38:25     INFO - GECKO(2220) | #32: mozilla::widget::TextEventDispatcher::DispatchInputEvent(nsIWidget*, mozilla::WidgetInputEvent&, nsEventStatus&) [widget/TextEventDispatcher.cpp:285]
[task 2021-06-14T17:38:25.152Z] 17:38:25     INFO - GECKO(2220) | #33: mozilla::widget::TextEventDispatcher::DispatchKeyboardEventInternal(mozilla::EventMessage, mozilla::WidgetKeyboardEvent const&, nsEventStatus&, void*, unsigned int, bool) [widget/TextEventDispatcher.cpp:699]
[task 2021-06-14T17:38:25.153Z] 17:38:25     INFO - GECKO(2220) | #34: mozilla::widget::TextEventDispatcher::MaybeDispatchKeypressEvents(mozilla::WidgetKeyboardEvent const&, nsEventStatus&, void*, bool) [widget/TextEventDispatcher.cpp:727]
[task 2021-06-14T17:38:25.153Z] 17:38:25     INFO - GECKO(2220) | #35: mozilla::TextInputProcessor::KeydownInternal(mozilla::WidgetKeyboardEvent const&, unsigned int, bool, unsigned int&) [dom/base/TextInputProcessor.cpp:1146]
[task 2021-06-14T17:38:25.154Z] 17:38:25     INFO - GECKO(2220) | #36: NS_InvokeByIndex [/opt/worker/tasks/task_162369086617260/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x1d8fee]
[task 2021-06-14T17:38:25.154Z] 17:38:25     INFO - GECKO(2220) | [Parent 2220, Main Thread] WARNING: EditorBase::DeleteRangesWithTransaction(eNoStrip) failed: 'NS_SUCCEEDED(rv)', file /builds/worker/checkouts/gecko/editor/libeditor/TextEditSubActionHandler.cpp:687
Flags: needinfo?(krosylight)
Upstream PR was closed without merging
Pushed by krosylight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/61e15374e617 Part 1: Keep a text node inside text controls even if empty r=masayuki,Jamie https://hg.mozilla.org/integration/autoland/rev/7d7feef654c7 Part 2: Initialize TextEditor always with a text node r=masayuki https://hg.mozilla.org/integration/autoland/rev/5a4f4514d99a Part 3: Assume TextEditor always have a text node r=masayuki https://hg.mozilla.org/integration/autoland/rev/876ed18c5126 Part 4: Always select the text node r=masayuki https://hg.mozilla.org/integration/autoland/rev/bd1c37ce2c61 Part 5: Allow caret to be at the end of text node r=masayuki
Flags: needinfo?(krosylight)
Upstream PR merged by moz-wptsync-bot
Status: RESOLVED → REOPENED
Flags: needinfo?(krosylight)
Resolution: FIXED → ---
Target Milestone: 91 Branch → ---
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/29416 for changes under testing/web-platform/tests

Note: triple click still selects the whole paragraph and thus causes text node removal.

Flags: needinfo?(krosylight)
Upstream PR merged by moz-wptsync-bot
Pushed by krosylight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/56d21f67888f Part 1: Keep a text node inside text controls even if empty r=masayuki,Jamie https://hg.mozilla.org/integration/autoland/rev/8fbaade4a731 Part 2: Initialize TextEditor always with a text node r=masayuki https://hg.mozilla.org/integration/autoland/rev/3859f5364319 Part 3: Assume TextEditor always have a text node r=masayuki https://hg.mozilla.org/integration/autoland/rev/ba5307e28661 Part 4: Always select the text node r=masayuki https://hg.mozilla.org/integration/autoland/rev/2daff4d41983 Part 5: Allow caret to be at the end of text node r=masayuki https://hg.mozilla.org/integration/autoland/rev/b00db45005cc Part 6: Actively prevent text deletion in DeleteRangeTransaction r=masayuki
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/29428 for changes under testing/web-platform/tests
Crash Signature: [@ nsINode::Length]
Crash Signature: [@ nsINode::Length] → [@ nsINode::Length] [@ mozilla::EditorBase::SetTextNodeWithoutTransaction] [@ mozilla::TextEditor::SelectEntireDocument] [@ mozilla::TextEditor::SetTextAsSubAction] [@ mozilla::TextEditor::SetTextWithoutTransaction]
Upstream PR merged by moz-wptsync-bot

sorry for being this late but for commit b00db45005cc51fd26aa96a1fdeb95590a24a0e3 we have a perf improvment

== Change summary for alert #30489 (as of Tue, 29 Jun 2021 09:28:26 GMT) ==

Improvements:

Ratio Suite Test Platform Options Absolute values (old vs new)
3% speedometer windows10-64-shippable-qr webrender 103.14 -> 106.16
2% speedometer linux1804-64-shippable-qr webrender 95.47 -> 97.81
2% speedometer windows10-64-shippable-qr webrender 102.92 -> 104.63

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=30489

Crash Signature: [@ nsINode::Length] [@ mozilla::EditorBase::SetTextNodeWithoutTransaction] [@ mozilla::TextEditor::SelectEntireDocument] [@ mozilla::TextEditor::SetTextAsSubAction] [@ mozilla::TextEditor::SetTextWithoutTransaction] → [@ nsINode::Length] [@ mozilla::EditorBase::SetTextNodeWithoutTransaction] [@ mozilla::TextEditor::SelectEntireDocument] [@ mozilla::TextEditor::SetTextAsSubAction] [@ mozilla::TextEditor::SetTextWithoutTransaction]

Really nice news! And it must improve footprint when an <input> or <textarea> is modified a lot since only one text node is stored in the transaction classes.

Regressions: 1724811
No longer regressions: 1719150

Is that a conclusion by a bisect? This technically shouldn't affect contenteditable (I saw https://github.com/mozilla-mobile/fenix/issues/20342#issuecomment-908840691)

Flags: needinfo?(m_kato)

(In reply to Kagami :saschanaz from comment #31)

Is that a conclusion by a bisect? This technically shouldn't affect contenteditable (I saw https://github.com/mozilla-mobile/fenix/issues/20342#issuecomment-908840691)

Twitter uses <textarea>.

Flags: needinfo?(m_kato)

And although bug 1724811 causes by this bug (https://hg.mozilla.org/integration/autoland/rev/b00db45005cc), root cause is that GeckoView (GeckoEditableSupport) sets unexpected selection. Of course, due to unexpected selection, it hits assertion by this changeset. (Then all text in text node is removed in non-debug build.)

Twitter uses <textarea>.

That's new to me since its DOM tree does not have it but only contenteditable 👀 (Twitter uses Draft.js and it doesn't have textarea AFAICT? But maybe I'm wrong...)

root cause is that GeckoView (GeckoEditableSupport) sets unexpected selection. Of course, due to unexpected selection, it hits assertion by this changeset. (Then all text in text node is removed in non-debug build.)

Oh, that sounds plausible, yes.

(Twitter uses Draft.js and it doesn't have textarea AFAICT? But maybe I'm wrong...)

Desktop version uses contenteditable (Draft.js), but mobile version doesn't use it. They seems to use <textarea> for mobile.twitter.com.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: