(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`](https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/editor/libeditor/EditorBase.cpp#2816,2847,2852), 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](https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/editor/libeditor/EditorBase.cpp#2856-2863) I guess. At least the recursive call of `EditorBase::InsertTextIntoTextNodeWithTransaction` mentioned in [c2](https://bugzilla.mozilla.org/show_bug.cgi?id=1591417#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](https://searchfox.org/mozilla-central/rev/3300072e993ae05d50d5c63d815260367eaf9179/dom/base/nsNodeUtils.h#47). 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.) Understood, thanks for the explanation. 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](https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/editor/libeditor/EditorBase.cpp#2824,2838,2847,2852). 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(`](https://searchfox.org/mozilla-central/rev/9df2ccc256fe54c314b34e52920471a0d04b283e/editor/libeditor/EditorBase.cpp#5727-5728). 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](https://searchfox.org/mozilla-central/rev/1fe0cf575841dbf3b7e159e88ba03260cd1354c0/editor/libeditor/HTMLEditSubActionHandler.cpp#648-650)? 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(`](https://searchfox.org/mozilla-central/rev/9df2ccc256fe54c314b34e52920471a0d04b283e/editor/libeditor/EditorBase.cpp#5727-5728) is called, it seems the node is always part of the tree. Why wouldn't it be and how to check? Via [nsIContent::IsInHTMLDocument](https://searchfox.org/mozilla-central/rev/9df2ccc256fe54c314b34e52920471a0d04b283e/dom/base/nsIContent.h#216)? 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.
Bug 1591417 Comment 10 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
(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`](https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/editor/libeditor/EditorBase.cpp#2816,2847,2852), 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](https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/editor/libeditor/EditorBase.cpp#2856-2863) I guess. At least the recursive call of `EditorBase::InsertTextIntoTextNodeWithTransaction` mentioned in [c2](https://bugzilla.mozilla.org/show_bug.cgi?id=1591417#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](https://searchfox.org/mozilla-central/rev/3300072e993ae05d50d5c63d815260367eaf9179/dom/base/nsNodeUtils.h#47). 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`](https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/editor/libeditor/EditorBase.cpp#2847)? 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](https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/editor/libeditor/EditorBase.cpp#2824,2838,2847,2852). 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(`](https://searchfox.org/mozilla-central/rev/9df2ccc256fe54c314b34e52920471a0d04b283e/editor/libeditor/EditorBase.cpp#5727-5728). 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](https://searchfox.org/mozilla-central/rev/1fe0cf575841dbf3b7e159e88ba03260cd1354c0/editor/libeditor/HTMLEditSubActionHandler.cpp#648-650)? 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(`](https://searchfox.org/mozilla-central/rev/9df2ccc256fe54c314b34e52920471a0d04b283e/editor/libeditor/EditorBase.cpp#5727-5728) is called, it seems the node is always part of the tree. Why wouldn't it be and how to check? Via [nsIContent::IsInHTMLDocument](https://searchfox.org/mozilla-central/rev/9df2ccc256fe54c314b34e52920471a0d04b283e/dom/base/nsIContent.h#216)? 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.