Closed Bug 1591417 Opened 4 months ago Closed 3 months ago

`EditorDOMPoint::ToRawRangeBoundary()` contains invalid offset

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: mbrodesser, Assigned: mbrodesser)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Should be reproducible by running editor/libeditor/tests/test_bug1425997.html.

From the logs one can observe that https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/editor/libeditor/EditorDOMPoint.h#85-87 is violated.

It should be fixed and changed to a MOZ_ASSERT and.

The invalid offset is returned via https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/editor/libeditor/EditorDOMPoint.h#692.

Priority: -- → P3

The assertion is violated by docshell/test/navigation/test_bug386782.html and ./testing/web-platform/tests/editing/run/delete.html too.

All three call-paths seem to differ.

To unblock bug 1587433 only the call-path of the test mentioned in the description would need to fixed.

Findings about editor/libeditor/tests/test_bug1425997.html:

Despite its name, EditorBase::InsertTextIntoTextNodeWithTransaction, seems to not insert text as a transaction.

For the above mentioned test-case, a first transaction doesn't finish before the next one starts:

#0  0x00007fffe4cd7a63 in mozilla::EditorBase::InsertTextIntoTextNodeWithTransaction(nsTSubstring<char16_t> const&, mozilla::dom::Text&, int, bool) (this=0x7fffc831b800, aStringToInsert=..., aTextNode=..., aOffset=1, aSuppressIME=true) at /home/mirko/src/firefox/gecko/editor/libeditor/EditorBase.cpp:2827
#1  0x00007fffe4e07ca0 in mozilla::WSRunObject::InsertNBSPAndRemoveFollowingASCIIWhitespaces(mozilla::WSRunScanner::WSPoint) (this=0x7ffffffee1f0, aPoint=...)
    at /home/mirko/src/firefox/gecko/editor/libeditor/WSRunObject.cpp:1545
#2  0x00007fffe4e0570d in mozilla::WSRunObject::PrepareToDeleteRangePriv(mozilla::WSRunObject*) (this=0x7ffffffee1f0, aEndObject=0x7ffffffee128)
    at /home/mirko/src/firefox/gecko/editor/libeditor/WSRunObject.cpp:1291
#3  0x00007fffe4e05b17 in mozilla::WSRunObject::PrepareToDeleteRange(mozilla::HTMLEditor*, nsCOMPtr<nsINode>*, int*, nsCOMPtr<nsINode>*, int*) (aHTMLEditor=0x7fffc831b800, aStartNode=0x7ffffffee578, aStartOffset=0x7ffffffee574, aEndNode=0x7ffffffee568, aEndOffset=0x7ffffffee564) at /home/mirko/src/firefox/gecko/editor/libeditor/WSRunObject.cpp:149
#4  0x00007fffe4d34b4b in mozilla::HTMLEditor::HandleDeleteNonCollapsedSelection(short, short, mozilla::HTMLEditor::SelectionWasCollapsed) (this=0x7fffc831b800, aDirectionAndAmount=2, aStripWrappers=0, aSelectionWasCollapsed=mozilla::HTMLEditor::SelectionWasCollapsed::No) at /home/mirko/src/firefox/gecko/editor/libeditor/HTMLEditSubActionHandler.cpp:3095
#5  0x00007fffe4d30c30 in mozilla::HTMLEditor::HandleDeleteSelectionInternal(short, short) (this=0x7fffc831b800, aDirectionAndAmount=2, aStripWrappers=0)
    at /home/mirko/src/firefox/gecko/editor/libeditor/HTMLEditSubActionHandler.cpp:2428
#6  0x00007fffe4d2fffe in mozilla::HTMLEditor::HandleDeleteSelection(short, short) (this=0x7fffc831b800, aDirectionAndAmount=2, aStripWrappers=0)
    at /home/mirko/src/firefox/gecko/editor/libeditor/HTMLEditSubActionHandler.cpp:2289
#7  0x00007fffe4deeb28 in mozilla::TextEditor::DeleteSelectionAsSubAction(short, short) (this=0x7fffc831b800, aDirectionAndAmount=2, aStripWrappers=0)
    at /home/mirko/src/firefox/gecko/editor/libeditor/TextEditor.cpp:646
#8  0x00007fffe4df4ebf in mozilla::TextEditor::DeleteSelectionAsAction(short, short, nsIPrincipal*) (this=0x7fffc831b800, aDirection=2, aStripWrappers=0, aPrincipal=0x7fffcec73430)
    at /home/mirko/src/firefox/gecko/editor/libeditor/TextEditor.cpp:618
#9  0x00007fffe4ce8762 in mozilla::DeleteCommand::DoCommand(mozilla::Command, mozilla::TextEditor&, nsIPrincipal*) const (this=0x7fffce388620, aCommand=mozilla::Command::DeleteCharBackward, aTextEditor=
    ..., aPrincipal=0x7fffcec73430) at /home/mirko/src/firefox/gecko/editor/libeditor/EditorCommands.cpp:618
#10 0x00007fffe185b8db in mozilla::dom::Document::ExecCommand(nsTSubstring<char16_t> const&, bool, nsTSubstring<char16_t> const&, nsIPrincipal&, mozilla::ErrorResult&) (this=0x7fffce7d3000, aHTMLCommandName=..., aShowUI=false, aValue=..., aSubjectPrincipal=..., aRv=...) at /home/mirko/src/firefox/gecko/dom/base/Document.cpp:4657
#11 0x00007fffe2b6d498 in mozilla::dom::Document_Binding::execCommand(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Document*, JSJitMethodCallArgs const&) (cx=0x7fffd3831000, obj=..., self=0x7fffce7d3000, args=...) at DocumentBinding.cpp:3364
#12 0x00007fffe2f5e0a7 in mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) (cx=0x7fffd3831000, argc=2, vp=0x7fffc56a2200) at /home/mirko/src/firefox/gecko/dom/bindings/BindingUtils.cpp:3198
#13 0x00007fffe7c8a1ae in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) (cx=0x7fffd3831000, native=0x7fffe2f5dd20 <mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*)>, reason=js::CallReason::Call, args=...)
    at /home/mirko/src/firefox/gecko/js/src/vm/Interpreter.cpp:456
#14 0x00007fffe7c73c91 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) (cx=0x7fffd3831000, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Call)
    at /home/mirko/src/firefox/gecko/js/src/vm/Interpreter.cpp:548
#15 0x00007fffe7c744f1 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) (cx=0x7fffd3831000, args=..., reason=js::CallReason::Call)
    at /home/mirko/src/firefox/gecko/js/src/vm/Interpreter.cpp:617
#16 0x00007fffe7c742b2 in js::CallFromStack(JSContext*, JS::CallArgs const&) (cx=0x7fffd3831000, args=...) at /home/mirko/src/firefox/gecko/js/src/vm/Interpreter.cpp:621
#17 0x00007fffe7c66d00 in Interpret(JSContext*, js::RunState&) (cx=0x7fffd3831000, state=...) at /home/mirko/src/firefox/gecko/js/src/vm/Interpreter.cpp:3110
#18 0x00007fffe7c5ae6e in js::RunScript(JSContext*, js::RunState&) (cx=0x7fffd3831000, state=...) at /home/mirko/src/firefox/gecko/js/src/vm/Interpreter.cpp:423
#19 0x00007fffe7c73f58 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) (cx=0x7fffd3831000, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Call)
    at /home/mirko/src/firefox/gecko/js/src/vm/Interpreter.cpp:589
#20 0x00007fffe7c744f1 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) (cx=0x7fffd3831000, args=..., reason=js::CallReason::Call)
    at /home/mirko/src/firefox/gecko/js/src/vm/Interpreter.cpp:617
#21 0x00007fffe7c74597 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) (cx=0x7fffd3831000, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Call) at /home/mirko/src/firefox/gecko/js/src/vm/Interpreter.cpp:634
#22 0x00007fffe7e1806b in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (cx=0x7fffd3831000, thisv=..., fval=..., args=..., rval=...) at /home/mirko/src/firefox/gecko/js/src/jsapi.cpp:2702
#23 0x00007fffe2b53678 in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) (this=0x7fffce652100, cx=0x7fffd3831000, aThisVal=..., event=
    ..., aRv=...) at EventListenerBinding.cpp:52
#24 0x00007fffe3521eb5 in mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget*>(mozilla::dom::EventTarget* const&, mozilla::dom::Event&, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) (this=0x7fffce652100, thisVal=@0x7fffffff3340: 0x7fffc85afe50, event=..., aRv=..., aExecutionReason=0x7fffdaa8cb68 "EventListener.handleEvent", aExceptionHandling=mozilla::dom::CallbackObject::eReportExceptions, aRealm=0x0) at /home/mirko/src/firefox/gecko/obj-x86_64-pc-linux-gnu-debug/dist/include/mozilla/dom/EventListenerBinding.h:66
#25 0x00007fffe34f4db2 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) (this=0x7fffce948ac0, aListener=0x7fffffff3518, aDOMEvent=0x7fffc7a7ca00, aCurrentTarget=0x7fffc85afe50) at /home/mirko/src/firefox/gecko/dom/events/EventListenerManager.cpp:1033
#26 0x00007fffe34f5881 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) (this=0x7fffce948ac0, ---Type <return> to continue, or q <return> to quit---
aPresContext=0x7fffcf066800, aEvent=0x7fffd21c91c0, aDOMEvent=0x7fffffff3b88, aCurrentTarget=0x7fffc85afe50, aEventStatus=0x7fffffff3b90, aItemInShadowTree=false)
    at /home/mirko/src/firefox/gecko/dom/events/EventListenerManager.cpp:1231
#27 0x00007fffe352c318 in mozilla::EventListenerManager::HandleEvent(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) (this=0x7fffce948ac0, aPresContext=0x7fffcf066800, aEvent=0x7fffd21c91c0, aDOMEvent=0x7fffffff3b88, aCurrentTarget=0x7fffc85afe50, aEventStatus=0x7fffffff3b90, aItemInShadowTree=false)
    at /home/mirko/src/firefox/gecko/obj-x86_64-pc-linux-gnu-debug/dist/include/mozilla/EventListenerManager.h:353
#28 0x00007fffe351ec88 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) (this=0x7fffd0d43058, aVisitor=..., aCd=...)
    at /home/mirko/src/firefox/gecko/dom/events/EventDispatcher.cpp:349
#29 0x00007fffe34eba31 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) (aChain=nsTArray<mozilla::EventTargetChainItem> & = {...}, aVisitor=..., aCallback=0x0, aCd=...) at /home/mirko/src/firefox/gecko/dom/events/EventDispatcher.cpp:587
#30 0x00007fffe34edd00 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) (aTarget=0x7fffd05aff00, aPresContext=0x7fffcf066800, aEvent=0x7fffd21c91c0, aDOMEvent=0x7fffc7a7ca00, aEventStatus=0x7fffffff3f84, aCallback=0x0, aTargets=0x0)
    at /home/mirko/src/firefox/gecko/dom/events/EventDispatcher.cpp:1049
#31 0x00007fffe34ef9b7 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) (aTarget=0x7fffd05aff00, aEvent=0x0, aDOMEvent=0x7fffc7a7ca00, aPresContext=0x7fffcf066800, aEventStatus=0x7fffffff3f84) at /home/mirko/src/firefox/gecko/dom/events/EventDispatcher.cpp:1146
#32 0x00007fffe1aa4e2a in nsINode::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) (this=0x7fffd05aff00, aEvent=..., aCallerType=mozilla::dom::CallerType::NonSystem, aRv=...) at /home/mirko/src/firefox/gecko/dom/base/nsINode.cpp:1063
#33 0x00007fffe34fb939 in mozilla::dom::EventTarget::DispatchEvent(mozilla::dom::Event&) (this=0x7fffd05aff00, aEvent=...) at /home/mirko/src/firefox/gecko/dom/events/EventTarget.cpp:178
#34 0x00007fffe34ad9ac in mozilla::AsyncEventDispatcher::Run() (this=0x7fffd05279e0) at /home/mirko/src/firefox/gecko/dom/events/AsyncEventDispatcher.cpp:69
#35 0x00007fffe164d8c5 in nsContentUtils::RemoveScriptBlocker() () at /home/mirko/src/firefox/gecko/dom/base/nsContentUtils.cpp:5189
#36 0x00007fffe1866752 in mozilla::dom::Document::EndUpdate() (this=0x7fffce7d3000) at /home/mirko/src/firefox/gecko/dom/base/Document.cpp:7023
#37 0x00007fffe160100a in mozAutoDocUpdate::~mozAutoDocUpdate() (this=0x7fffffff4340) at /home/mirko/src/firefox/gecko/dom/base/mozAutoDocUpdate.h:34
#38 0x00007fffe1782674 in mozilla::dom::CharacterData::SetTextInternal(unsigned int, unsigned int, char16_t const*, unsigned int, bool, CharacterDataChangeInfo::Details*) (this=0x7fffd05aff00, aOffset=0, aCount=0, aBuffer=0x7fffc7e1e66c u" ", aLength=1, aNotify=true, aDetails=0x0) at /home/mirko/src/firefox/gecko/dom/base/CharacterData.cpp:354
#39 0x00007fffe17829d6 in mozilla::dom::CharacterData::InsertData(unsigned int, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) (this=0x7fffd05aff00, aOffset=0, aData=..., aRv=...)
    at /home/mirko/src/firefox/gecko/dom/base/CharacterData.cpp:191
#40 0x00007fffe4cbff9e in mozilla::EditorBase::DoInsertText(mozilla::dom::Text&, unsigned int, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) (this=0x7fffc831b800, aText=..., aOffset=0, aStringToInsert=..., aRv=...) at /home/mirko/src/firefox/gecko/editor/libeditor/EditorBase.cpp:2435
#41 0x00007fffe4dc48ff in mozilla::InsertTextTransaction::DoTransaction() (this=0x7fffce8eae40) at /home/mirko/src/firefox/gecko/editor/libeditor/InsertTextTransaction.cpp:65
#42 0x00007fffe4e33a7e in mozilla::TransactionItem::DoTransaction() (this=0x7fffcf021670) at /home/mirko/src/firefox/gecko/editor/txmgr/TransactionItem.cpp:143
#43 0x00007fffe4e3562b in mozilla::TransactionManager::BeginTransaction(nsITransaction*, nsISupports*) (this=0x7fffce771cd0, aTransaction=0x7fffce8eae40, aData=0x0)
    at /home/mirko/src/firefox/gecko/editor/txmgr/TransactionManager.cpp:634
#44 0x00007fffe4e35327 in mozilla::TransactionManager::DoTransaction(nsITransaction*) (this=0x7fffce771cd0, aTransaction=0x7fffce8eae40) at /home/mirko/src/firefox/gecko/editor/txmgr/TransactionManager.cpp:68
#45 0x00007fffe4cb8366 in mozilla::EditorBase::DoTransactionInternal(nsITransaction*) (this=0x7fffc831b800, aTxn=0x7fffce8eae40) at /home/mirko/src/firefox/gecko/editor/libeditor/EditorBase.cpp:784
#46 0x00007fffe4cd7d13 in mozilla::EditorBase::InsertTextIntoTextNodeWithTransaction(nsTSubstring<char16_t> const&, mozilla::dom::Text&, int, bool) (this=0x7fffc831b800, aStringToInsert=..., aTextNode=..., aOffset=0, aSuppressIME=true) at /home/mirko/src/firefox/gecko/editor/libeditor/EditorBase.cpp:2859

Can be reproduced by setting breakpoints on the beginning and end of the method.

It seems this happens because of mozAutoDocUpdate being used: https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/dom/base/CharacterData.cpp#236.

Masayuki: any clue if we could somehow transform the transaction to a real transaction? I'm new to this area of the code and not sure how much people have already thought about this.

Flags: needinfo?(masayuki)

(In reply to Mirko Brodesser (:mbrodesser) from comment #2)

Findings about editor/libeditor/tests/test_bug1425997.html:

Despite its name, EditorBase::InsertTextIntoTextNodeWithTransaction, seems to not insert text as a transaction.

"transaction" in editor means the action is undo-able. So, InsertTextIntoTextNodeWithTransaction() handles it with *Transaction classes and add it to current "placeholder transaction".

For the above mentioned test-case, a first transaction doesn't finish before the next one starts:

Yes, unfortunately, there are such bugs if web apps use Document.execCommand() from mutation event listeners. I think that in most cases, the nested transactions are added to the first edit action's "placehoder transaction". It means one "undo" cancels both edit actions.

Can be reproduced by setting breakpoints on the beginning and end of the method.

It seems this happens because of mozAutoDocUpdate being used: https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/dom/base/CharacterData.cpp#236.

Masayuki: any clue if we could somehow transform the transaction to a real transaction? I'm new to this area of the code and not sure how much people have already thought about this.

I and Makoto-san work on around the transactions, but Makoto-san must be busy for GV work. So, feel free to ask me.

In this case, insertedOffset is cached before calling DoTransactionInternal(), but used later. That's the reason why the offset can be invalid.

DidInsertText() call is necessary to record changed range for spellchecker etc. So that one possible solution here is, it should check the range is still valid only when EditorBase::HasMutationEventListeners(NS_EVENT_BITS_MUTATION_CHARACTERDATAMODIFIED) returns true. If the text node is removed from the tree, it needs to do nothing. If the offset is invalid or the text beween the offset and offset + inserted string length is NOT the inserted string, call DidInsertContent() with the text node to make the all of the text modified.

Flags: needinfo?(masayuki)

Ah, or call DidInsertText() with the start of the text node and length of the text node may be better than DidInsertContent().

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #3)

(In reply to Mirko Brodesser (:mbrodesser) from comment #2)

Findings about editor/libeditor/tests/test_bug1425997.html:

Despite its name, EditorBase::InsertTextIntoTextNodeWithTransaction, seems to not insert text as a transaction.

"transaction" in editor means the action is undo-able. So, InsertTextIntoTextNodeWithTransaction() handles it with *Transaction classes and add it to current "placeholder transaction".

For the above mentioned test-case, a first transaction doesn't finish before the next one starts:

Yes, unfortunately, there are such bugs if web apps use Document.execCommand() from mutation event listeners. I think that in most cases, the nested transactions are added to the first edit action's "placehoder transaction". It means one "undo" cancels both edit actions.

Intuitively, this could be solved by notifying the mutation event listeners just immediately after the transaction has finished. Besides being potentially a lot of refactoring work, are there reasons why that wouldn't work? It just seems wrong to not finish a transaction before beginning the next one.

Can be reproduced by setting breakpoints on the beginning and end of the method.

It seems this happens because of mozAutoDocUpdate being used: https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/dom/base/CharacterData.cpp#236.

Masayuki: any clue if we could somehow transform the transaction to a real transaction? I'm new to this area of the code and not sure how much people have already thought about this.

I and Makoto-san work on around the transactions, but Makoto-san must be busy for GV work. So, feel free to ask me.

Thanks, also for above explanations.

In this case, insertedOffset is cached before calling DoTransactionInternal(), but used later. That's the reason why the offset can be invalid.

DidInsertText() call is necessary to record changed range for spellchecker etc. So that one possible solution here is, it should check the range is still valid only when EditorBase::HasMutationEventListeners(NS_EVENT_BITS_MUTATION_CHARACTERDATAMODIFIED) returns true.

To be clear, an invalid EditorRawDOMPoint is created in EditorBase::TopLevelEditSubActionData::DidInsertText(. It's unclear to me, if that call is really (indirectly) about the spellchecker etc. Did you perhaps confuse the two DidInsertText methods? If not, it's also unclear to me, where exactly HasMutationEventListeners would have to be checked and why.

If the text node is removed from the tree, it needs to do nothing. If the offset is invalid or the text beween the offset and offset + inserted string length is NOT the inserted string, call DidInsertContent() with the text node to make the all of the text modified.

When EditorBase::TopLevelEditSubActionData::DidInsertText( is called, it seems the node is always part of the tree. Why wouldn't it be and how to check? Via nsIContent::IsInHTMLDocument? I guess calling DidInsertContent should work, even without the checks. Or is it too expensive to always call?

Ah, or call DidInsertText() with the start of the text node and length of the text node may be better than DidInsertContent().

That seems to be possibly incorrect. If the text between offset and offset + inserted string is not the inserted string, passing the node's length in EditorBase::TopLevelEditSubActionData::DidInsertText( could be too large, if the string was not inserted at the end. That is, it would be reported that the whole text changed, where it potentially didn't. Or is that acceptable in terms of correctness?

Instead, could it be OK to call in EditorBase::InsertTextIntoTextNodeWithTransaction always EditorBase::TopLevelEditSubActionData::DidInsertContent instead of DidInsertText? It would at least keep the code simpler.

Flags: needinfo?(masayuki)

(In reply to Mirko Brodesser (:mbrodesser) from comment #5)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #3)

(In reply to Mirko Brodesser (:mbrodesser) from comment #2)

Findings about editor/libeditor/tests/test_bug1425997.html:

Despite its name, EditorBase::InsertTextIntoTextNodeWithTransaction, seems to not insert text as a transaction.

"transaction" in editor means the action is undo-able. So, InsertTextIntoTextNodeWithTransaction() handles it with *Transaction classes and add it to current "placeholder transaction".

For the above mentioned test-case, a first transaction doesn't finish before the next one starts:

Yes, unfortunately, there are such bugs if web apps use Document.execCommand() from mutation event listeners. I think that in most cases, the nested transactions are added to the first edit action's "placehoder transaction". It means one "undo" cancels both edit actions.

Intuitively, this could be solved by notifying the mutation event listeners just immediately after the transaction has finished. Besides being potentially a lot of refactoring work, are there reasons why that wouldn't work? It just seems wrong to not finish a transaction before beginning the next one.

It might be possible to fix it with AutoScriptBlocker only around DoTransactionInternal(), but it means that if we modify the DOM tree multiple times in a call of DoTransactionInternal(), mutation event listeners earlier than last one may be broken if they refer the DOM tree directly. Although the mutation event listener usage is really low, but could break something.

Smaug may have some thoughts.

In this case, insertedOffset is cached before calling DoTransactionInternal(), but used later. That's the reason why the offset can be invalid.

DidInsertText() call is necessary to record changed range for spellchecker etc. So that one possible solution here is, it should check the range is still valid only when EditorBase::HasMutationEventListeners(NS_EVENT_BITS_MUTATION_CHARACTERDATAMODIFIED) returns true.

To be clear, an invalid EditorRawDOMPoint is created in EditorBase::TopLevelEditSubActionData::DidInsertText(. It's unclear to me, if that call is really (indirectly) about the spellchecker etc. Did you perhaps confuse the two DidInsertText methods? If not, it's also unclear to me, where exactly HasMutationEventListeners would have to be checked and why.

Must be there. In there, the container may not be in the DOM tree and the offset may be invalid since mutation event listener may have already modified contents in the container.

If the text node is removed from the tree, it needs to do nothing. If the offset is invalid or the text beween the offset and offset + inserted string length is NOT the inserted string, call DidInsertContent() with the text node to make the all of the text modified.

When EditorBase::TopLevelEditSubActionData::DidInsertText( is called, it seems the node is always part of the tree. Why wouldn't it be and how to check? Via nsIContent::IsInHTMLDocument? I guess calling DidInsertContent should work, even without the checks. Or is it too expensive to always call?

Yes, it may be expensive if a text node has really long text. The range means dirty so that the range will be cleaned up after handling all (nested) edit actions. Like specllchecker etc.
https://searchfox.org/mozilla-central/rev/1fe0cf575841dbf3b7e159e88ba03260cd1354c0/editor/libeditor/HTMLEditSubActionHandler.cpp#484-650

Ah, or call DidInsertText() with the start of the text node and length of the text node may be better than DidInsertContent().

That seems to be possibly incorrect. If the text between offset and offset + inserted string is not the inserted string, passing the node's length in EditorBase::TopLevelEditSubActionData::DidInsertText( could be too large, if the string was not inserted at the end. That is, it would be reported that the whole text changed, where it potentially didn't. Or is that acceptable in terms of correctness?

I meant that once we detect the modification from mutation event listener, we could call DidInsertText() with all ranges of the text node.

Instead, could it be OK to call in EditorBase::InsertTextIntoTextNodeWithTransaction always EditorBase::TopLevelEditSubActionData::DidInsertContent instead of DidInsertText? It would at least keep the code simpler.

No, at least, we should keep current performance if there is no mutation event listeners. When we worked on Quantum Flow, the performance of spellchecker appeared in the todo list a lot.

Flags: needinfo?(masayuki)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #6)

(In reply to Mirko Brodesser (:mbrodesser) from comment #5)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #3)

(In reply to Mirko Brodesser (:mbrodesser) from comment #2)

Findings about editor/libeditor/tests/test_bug1425997.html:

Despite its name, EditorBase::InsertTextIntoTextNodeWithTransaction, seems to not insert text as a transaction.

"transaction" in editor means the action is undo-able. So, InsertTextIntoTextNodeWithTransaction() handles it with *Transaction classes and add it to current "placeholder transaction".

For the above mentioned test-case, a first transaction doesn't finish before the next one starts:

Yes, unfortunately, there are such bugs if web apps use Document.execCommand() from mutation event listeners. I think that in most cases, the nested transactions are added to the first edit action's "placehoder transaction". It means one "undo" cancels both edit actions.

Intuitively, this could be solved by notifying the mutation event listeners just immediately after the transaction has finished. Besides being potentially a lot of refactoring work, are there reasons why that wouldn't work? It just seems wrong to not finish a transaction before beginning the next one.

It might be possible to fix it with AutoScriptBlocker only around DoTransactionInternal(), but it means that if we modify the DOM tree multiple times in a call of DoTransactionInternal(), mutation event listeners earlier than last one may be broken if they refer the DOM tree directly. Although the mutation event listener usage is really low, but could break something.

Smaug may have some thoughts.

Sorry, it seems I lack context and need more detailed explanations.

It's unclear to me, where the mutation event listeners are actually notified. It's not here I guess. At least the recursive call of EditorBase::InsertTextIntoTextNodeWithTransaction mentioned in c2 doesn't take that path.

In this case, insertedOffset is cached before calling DoTransactionInternal(), but used later. That's the reason why the offset can be invalid.

DidInsertText() call is necessary to record changed range for spellchecker etc. So that one possible solution here is, it should check the range is still valid only when EditorBase::HasMutationEventListeners(NS_EVENT_BITS_MUTATION_CHARACTERDATAMODIFIED) returns true.

To be clear, an invalid EditorRawDOMPoint is created in EditorBase::TopLevelEditSubActionData::DidInsertText(. It's unclear to me, if that call is really (indirectly) about the spellchecker etc. Did you perhaps confuse the two DidInsertText methods? If not, it's also unclear to me, where exactly HasMutationEventListeners would have to be checked and why.

Must be there. In there, the container may not be in the DOM tree and the offset may be invalid since mutation event listener may have already modified contents in the container.

It's still unclear to me where, where the HasMutationEventListeners( call should be added. Do you mean around the tagged code? It seems to be the wrong place and if not, can you please explain why?

If the text node is removed from the tree, it needs to do nothing. If the offset is invalid or the text beween the offset and offset + inserted string length is NOT the inserted string, call DidInsertContent() with the text node to make the all of the text modified.

When EditorBase::TopLevelEditSubActionData::DidInsertText( is called, it seems the node is always part of the tree. Why wouldn't it be and how to check? Via nsIContent::IsInHTMLDocument? I guess calling DidInsertContent should work, even without the checks. Or is it too expensive to always call?

Yes, it may be expensive if a text node has really long text. The range means dirty so that the range will be cleaned up after handling all (nested) edit actions. Like specllchecker etc.

Ok, that it may be too expensive is understood. The question of why the node wouldn't be part of the tree and how to check that is still open.

https://searchfox.org/mozilla-central/rev/1fe0cf575841dbf3b7e159e88ba03260cd1354c0/editor/libeditor/HTMLEditSubActionHandler.cpp#484-650

Ah, or call DidInsertText() with the start of the text node and length of the text node may be better than DidInsertContent().

That seems to be possibly incorrect. If the text between offset and offset + inserted string is not the inserted string, passing the node's length in EditorBase::TopLevelEditSubActionData::DidInsertText( could be too large, if the string was not inserted at the end. That is, it would be reported that the whole text changed, where it potentially didn't. Or is that acceptable in terms of correctness?

I meant that once we detect the modification from mutation event listener, we could call DidInsertText() with all ranges of the text node.

Instead, could it be OK to call in EditorBase::InsertTextIntoTextNodeWithTransaction always EditorBase::TopLevelEditSubActionData::DidInsertContent instead of DidInsertText? It would at least keep the code simpler.

No, at least, we should keep current performance if there is no mutation event listeners. When we worked on Quantum Flow, the performance of spellchecker appeared in the todo list a lot.

Ok.

"Intuitively, this could be solved by notifying the mutation event listeners just immediately after the transaction has finished. "
If transaction means a mutation in this context, one needs to remember that DOMNodeRemoved must be dispatched before the actual change to the DOM. So that event can't be moved to fire later.

(In reply to Mirko Brodesser (:mbrodesser) from comment #7)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #6)

(In reply to Mirko Brodesser (:mbrodesser) from comment #5)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #3)

(In reply to Mirko Brodesser (:mbrodesser) from comment #2)

Findings about editor/libeditor/tests/test_bug1425997.html:

Despite its name, EditorBase::InsertTextIntoTextNodeWithTransaction, seems to not insert text as a transaction.

"transaction" in editor means the action is undo-able. So, InsertTextIntoTextNodeWithTransaction() handles it with *Transaction classes and add it to current "placeholder transaction".

For the above mentioned test-case, a first transaction doesn't finish before the next one starts:

Yes, unfortunately, there are such bugs if web apps use Document.execCommand() from mutation event listeners. I think that in most cases, the nested transactions are added to the first edit action's "placehoder transaction". It means one "undo" cancels both edit actions.

Intuitively, this could be solved by notifying the mutation event listeners just immediately after the transaction has finished. Besides being potentially a lot of refactoring work, are there reasons why that wouldn't work? It just seems wrong to not finish a transaction before beginning the next one.

It might be possible to fix it with AutoScriptBlocker only around DoTransactionInternal(), but it means that if we modify the DOM tree multiple times in a call of DoTransactionInternal(), mutation event listeners earlier than last one may be broken if they refer the DOM tree directly. Although the mutation event listener usage is really low, but could break something.

Smaug may have some thoughts.

Sorry, it seems I lack context and need more detailed explanations.

It's unclear to me, where the mutation event listeners are actually notified. It's not here I guess. At least the recursive call of EditorBase::InsertTextIntoTextNodeWithTransaction mentioned in c2 doesn't take that path.

No, mutation events are fired synchronously unless there is one or more script blockers. The dispatcher is nsNodeUtils. For example:
https://searchfox.org/mozilla-central/rev/ce02064d8afc8673cef83c92896ee873bd35e7ae/dom/base/nsNodeUtils.cpp#149-155
So, you can assume that when you touch DOM tree, mutation event listener(s) immediately. (On the other hand, mutation event observers runs after everything finished.) So, touching DOM tree from editor means that the editor could be broken, the related frames could be destroyed, the document itself may be unloaded immediately.
https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-eventgroupings-mutationevents

In this case, insertedOffset is cached before calling DoTransactionInternal(), but used later. That's the reason why the offset can be invalid.

DidInsertText() call is necessary to record changed range for spellchecker etc. So that one possible solution here is, it should check the range is still valid only when EditorBase::HasMutationEventListeners(NS_EVENT_BITS_MUTATION_CHARACTERDATAMODIFIED) returns true.

To be clear, an invalid EditorRawDOMPoint is created in EditorBase::TopLevelEditSubActionData::DidInsertText(. It's unclear to me, if that call is really (indirectly) about the spellchecker etc. Did you perhaps confuse the two DidInsertText methods? If not, it's also unclear to me, where exactly HasMutationEventListeners would have to be checked and why.

Must be there. In there, the container may not be in the DOM tree and the offset may be invalid since mutation event listener may have already modified contents in the container.

It's still unclear to me where, where the HasMutationEventListeners( call should be added. Do you mean around the tagged code? It seems to be the wrong place and if not, can you please explain why?

No, I meant that If HasMutationEventListeners() returns true immediately after DoTransactionInternal() call, it's safe to use current path. However, otherwise, you need to check whether the node is in the tree or not, and the offset is still valid in the text node or not. Or just use another method like DidInsertContent().

If the text node is removed from the tree, it needs to do nothing. If the offset is invalid or the text beween the offset and offset + inserted string length is NOT the inserted string, call DidInsertContent() with the text node to make the all of the text modified.

When EditorBase::TopLevelEditSubActionData::DidInsertText( is called, it seems the node is always part of the tree. Why wouldn't it be and how to check? Via nsIContent::IsInHTMLDocument? I guess calling DidInsertContent should work, even without the checks. Or is it too expensive to always call?

Yes, it may be expensive if a text node has really long text. The range means dirty so that the range will be cleaned up after handling all (nested) edit actions. Like specllchecker etc.

Ok, that it may be too expensive is understood. The question of why the node wouldn't be part of the tree and how to check that is still open.

Because mutation event listeners can do anything while we're changing the DOM tree.

(In reply to Olli Pettay [:smaug] from comment #8)

"Intuitively, this could be solved by notifying the mutation event listeners just immediately after the transaction has finished. "
If transaction means a mutation in this context, one needs to remember that DOMNodeRemoved must be dispatched before the actual change to the DOM. So that event can't be moved to fire later.

Ah, I forgot that it's fired immediately before the DOM tree change... So, we must not use AutoScriptBlocker for the backward compatibility.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #9)

(In reply to Mirko Brodesser (:mbrodesser) from comment #7)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #6)

(In reply to Mirko Brodesser (:mbrodesser) from comment #5)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #3)

(In reply to Mirko Brodesser (:mbrodesser) from comment #2)

Findings about editor/libeditor/tests/test_bug1425997.html:

Despite its name, EditorBase::InsertTextIntoTextNodeWithTransaction, seems to not insert text as a transaction.

"transaction" in editor means the action is undo-able. So, InsertTextIntoTextNodeWithTransaction() handles it with *Transaction classes and add it to current "placeholder transaction".

For the above mentioned test-case, a first transaction doesn't finish before the next one starts:

Yes, unfortunately, there are such bugs if web apps use Document.execCommand() from mutation event listeners. I think that in most cases, the nested transactions are added to the first edit action's "placehoder transaction". It means one "undo" cancels both edit actions.

Intuitively, this could be solved by notifying the mutation event listeners just immediately after the transaction has finished. Besides being potentially a lot of refactoring work, are there reasons why that wouldn't work? It just seems wrong to not finish a transaction before beginning the next one.

It might be possible to fix it with AutoScriptBlocker only around DoTransactionInternal(), but it means that if we modify the DOM tree multiple times in a call of DoTransactionInternal(), mutation event listeners earlier than last one may be broken if they refer the DOM tree directly. Although the mutation event listener usage is really low, but could break something.

Smaug may have some thoughts.

Sorry, it seems I lack context and need more detailed explanations.

It's unclear to me, where the mutation event listeners are actually notified. It's not here I guess. At least the recursive call of EditorBase::InsertTextIntoTextNodeWithTransaction mentioned in c2 doesn't take that path.

No, mutation events are fired synchronously unless there is one or more script blockers. The dispatcher is nsNodeUtils. For example:
https://searchfox.org/mozilla-central/rev/ce02064d8afc8673cef83c92896ee873bd35e7ae/dom/base/nsNodeUtils.cpp#149-155

Mutation events (the legacy ones) are fired synchrounously, yes. But not at https://searchfox.org/mozilla-central/rev/ce02064d8afc8673cef83c92896ee873bd35e7ae/dom/base/nsNodeUtils.cpp#149-155. That method is for mutation observers: CharacterDataChanged is for mutation observers. The legay-mutation events seem to be created at https://searchfox.org/mozilla-central/rev/3300072e993ae05d50d5c63d815260367eaf9179/dom/base/CharacterData.cpp#329,332.

So, you can assume that when you touch DOM tree, mutation event listener(s) immediately. (On the other hand, mutation event observers runs after everything finished.)

Thanks for the explanation. From the code, how does one know when "everything has finished"? Couldn't it be immediately after calling DoTransactionInternal? If not, how to know when?

So, touching DOM tree from editor means that the editor could be broken, the related frames could be destroyed, the document itself may be unloaded immediately.

https://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-eventgroupings-mutationevents

In this case, insertedOffset is cached before calling DoTransactionInternal(), but used later. That's the reason why the offset can be invalid.

DidInsertText() call is necessary to record changed range for spellchecker etc. So that one possible solution here is, it should check the range is still valid only when EditorBase::HasMutationEventListeners(NS_EVENT_BITS_MUTATION_CHARACTERDATAMODIFIED) returns true.

To be clear, an invalid EditorRawDOMPoint is created in EditorBase::TopLevelEditSubActionData::DidInsertText(. It's unclear to me, if that call is really (indirectly) about the spellchecker etc. Did you perhaps confuse the two DidInsertText methods? If not, it's also unclear to me, where exactly HasMutationEventListeners would have to be checked and why.

Must be there. In there, the container may not be in the DOM tree and the offset may be invalid since mutation event listener may have already modified contents in the container.

It's still unclear to me where, where the HasMutationEventListeners( call should be added. Do you mean around the tagged code? It seems to be the wrong place and if not, can you please explain why?

No, I meant that If HasMutationEventListeners() returns true immediately after DoTransactionInternal() call, it's safe to use current path.

Isn't it the other way around? If HasMutationEventListeners() returns false, then use the current path. Even if that's the case, couldn't a mutation event listener un-register itself while DoTransactionInternal() is called? The idea here is to check HasMutationEventListeners() in order to determine if the DOM could be modified while calling DoTransactionInternal(), right?

However, otherwise, you need to check whether the node is in the tree or not, and the offset is still valid in the text node or not. Or just use another method like DidInsertContent().

If the text node is removed from the tree, it needs to do nothing. If the offset is invalid or the text beween the offset and offset + inserted string length is NOT the inserted string, call DidInsertContent() with the text node to make the all of the text modified.

When EditorBase::TopLevelEditSubActionData::DidInsertText( is called, it seems the node is always part of the tree. Why wouldn't it be and how to check? Via nsIContent::IsInHTMLDocument? I guess calling DidInsertContent should work, even without the checks. Or is it too expensive to always call?

Yes, it may be expensive if a text node has really long text. The range means dirty so that the range will be cleaned up after handling all (nested) edit actions. Like specllchecker etc.

Ok, that it may be too expensive is understood. The question of why the node wouldn't be part of the tree and how to check that is still open.

Because mutation event listeners can do anything while we're changing the DOM tree.

(In reply to Olli Pettay [:smaug] from comment #8)

"Intuitively, this could be solved by notifying the mutation event listeners just immediately after the transaction has finished. "
If transaction means a mutation in this context, one needs to remember that DOMNodeRemoved must be dispatched before the actual change to the DOM. So that event can't be moved to fire later.

Ah, I forgot that it's fired immediately before the DOM tree change... So, we must not use AutoScriptBlocker for the backward compatibility.

Ok, dropping that approach.

Flags: needinfo?(masayuki)

Ah, or call DidInsertText() with the start of the text node and length of the text node may be better than DidInsertContent().

That seems to be possibly incorrect. If the text between offset and offset + inserted string is not the inserted string, passing the node's length in EditorBase::TopLevelEditSubActionData::DidInsertText( could be too large, if the string was not inserted at the end. That is, it would be reported that the whole text changed, where it potentially didn't. Or is that acceptable in terms of correctness?

I meant that once we detect the modification from mutation event listener, we could call DidInsertText() with all ranges of the text node.

With "all ranges of the text node", the whole range is meant, because there is only one, I guess. So TopLevelEditSubActionData::DidInsertText would receive the end of the range as an argument instead of the aString. It's still unclear to me, if pretending (for some cases at least) that the whole string changed is correct. If so, why is it?

Depends on: 1594394

(In reply to Mirko Brodesser (:mbrodesser) from comment #10)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #9)

No, mutation events are fired synchronously unless there is one or more script blockers. The dispatcher is nsNodeUtils. For example:
https://searchfox.org/mozilla-central/rev/ce02064d8afc8673cef83c92896ee873bd35e7ae/dom/base/nsNodeUtils.cpp#149-155

Mutation events (the legacy ones) are fired synchrounously, yes. But not at https://searchfox.org/mozilla-central/rev/ce02064d8afc8673cef83c92896ee873bd35e7ae/dom/base/nsNodeUtils.cpp#149-155. That method is for mutation observers: CharacterDataChanged is for mutation observers. The legay-mutation events seem to be created at https://searchfox.org/mozilla-central/rev/3300072e993ae05d50d5c63d815260367eaf9179/dom/base/CharacterData.cpp#329,332.

Oh, thanks. I completely misunderstood the implementation.

So, you can assume that when you touch DOM tree, mutation event listener(s) immediately. (On the other hand, mutation event observers runs after everything finished.)

Thanks for the explanation. From the code, how does one know when "everything has finished"? Couldn't it be immediately after calling DoTransactionInternal? If not, how to know when?

I believe that the timing must be immediately after calling DoTransactionInternal().

No, I meant that If HasMutationEventListeners() returns true immediately after DoTransactionInternal() call, it's safe to use current path.

Isn't it the other way around? If HasMutationEventListeners() returns false, then use the current path. Even if that's the case, couldn't a mutation event listener un-register itself while DoTransactionInternal() is called? The idea here is to check HasMutationEventListeners() in order to determine if the DOM could be modified while calling DoTransactionInternal(), right?

Oops, I meant that if HasMutaionEventListeners() returns false, it's safe to use current path without any validation.

Even if the last mutation event listener is removed from the document, it keeps returning true. I.e., once a mutation event listener is added, the document may become working slow.

Flags: needinfo?(masayuki)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #12)

(In reply to Mirko Brodesser (:mbrodesser) from comment #10)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #9)

No, mutation events are fired synchronously unless there is one or more script blockers. The dispatcher is nsNodeUtils. For example:
https://searchfox.org/mozilla-central/rev/ce02064d8afc8673cef83c92896ee873bd35e7ae/dom/base/nsNodeUtils.cpp#149-155

Mutation events (the legacy ones) are fired synchrounously, yes. But not at https://searchfox.org/mozilla-central/rev/ce02064d8afc8673cef83c92896ee873bd35e7ae/dom/base/nsNodeUtils.cpp#149-155. That method is for mutation observers: CharacterDataChanged is for mutation observers. The legay-mutation events seem to be created at https://searchfox.org/mozilla-central/rev/3300072e993ae05d50d5c63d815260367eaf9179/dom/base/CharacterData.cpp#329,332.

Oh, thanks. I completely misunderstood the implementation.

So, you can assume that when you touch DOM tree, mutation event listener(s) immediately. (On the other hand, mutation event observers runs after everything finished.)

Thanks for the explanation. From the code, how does one know when "everything has finished"? Couldn't it be immediately after calling DoTransactionInternal? If not, how to know when?

I believe that the timing must be immediately after calling DoTransactionInternal().

No, I meant that If HasMutationEventListeners() returns true immediately after DoTransactionInternal() call, it's safe to use current path.

Isn't it the other way around? If HasMutationEventListeners() returns false, then use the current path. Even if that's the case, couldn't a mutation event listener un-register itself while DoTransactionInternal() is called? The idea here is to check HasMutationEventListeners() in order to determine if the DOM could be modified while calling DoTransactionInternal(), right?

Oops, I meant that if HasMutaionEventListeners() returns false, it's safe to use current path without any validation.

Even if the last mutation event listener is removed from the document, it keeps returning true. I.e., once a mutation event listener is added, the document may become working slow.

That's surprising and unintuitive, but presumably there are reasons. It means the name HasMutationEventListeners doesn't reflect what it actually checks. So a name like MaybeHasMutationEventListeners would be more accurate.

No longer depends on: 1594394
Blocks: 1594394
Assignee: nobody → mbrodesser

After removing all mutation event listeners, the corresponding method
still returns true, so the new name is more accurate.

Depends on D52341

Needed to compare with nsTextFragment::Get1b() which returns latin1-encoded
characters. Used in a subsequent review.

Depends on D52342

Otherwise, an invalid EditorRawDOMPoint was constructed by a test.

Depends on D52343

Pushed by mbrodesser@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ece388605bb
part 1) Assert `EditorDOMPointBase::ToRawRangeBoundary()` uses valid offset. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/34804d1ae0c0
part 2) Rename `HasMutationEventListeners` to `MaybeHasMutationEventListeners`. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/0dd20dae7caa
part 3) Add `nsTStringRepr<T>::EqualsLatin1`. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/e711f304a2cd
part 4) Adapt `InsertTextIntoTextNodeWithTransaction` to pass adjusted range to `TopLevelEditSubActionDataRef.DidInsertText`. r=masayuki
You need to log in before you can comment on or make changes to this bug.