1766355 - (create-move-node-transaction) Optimize `HTMLEditor::MoveNodeTransaction` and stop touching `Selection` directly

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement, P2)

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1766355 - part 1: Add `MoveNodeTransaction` to handle delete node and insert node in a transaction class instance r=m_kato!

Creating both DeleteNodeTransaction and InsertNodeTransaction wastes
memory. They should be done in an instance instead.

Fortunately, no edit action listener checks whether the deleted node is still
in the composed document or not, etc. Therefore, we can simply notify them of
both deletion and insertion which were done in
EditorBase::InsertNodeWithTransaction and
EditorBase::DeleteNodeWithTransaction. Note that previously, the range
updater needs to ignore the notifications from them while the node is being
moved. However, it does not require anymore. Therefore, this patch makes it
stop locking, and that would fix minor problem in the case of legacy mutation
event listeners run another edit action.

On the other hand, this changes some edge cases handling of
MoveNodeWithTransaction which are detected by the WPT. According to the
previous result of applying this patch, nsINode::InsertBefore fails and that
leads some errors at updating the changed range. I guess that the cause is
that there is some bugs at updating insertion point after deleting the node from
the DOM tree around here:
https://searchfox.org/mozilla-central/rev/0ffae75b690219858e5a45a39f8759a8aee7b9a2/editor/libeditor/HTMLEditor.cpp#5058-5071

However, it's safely fixed by the new code which does not remove the node from
the DOM tree explicitly. So, I think that it's safe to accept this behavior
change for web apps in the wild.

Depends on D146365

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1766355 - part 2: Make `MoveNodeResult` method names alinging to `Result` r=m_kato!

Depends on D146397

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1766355 - part 3: Make `MoveNodeResult` store caret point suggestion r=m_kato!

Depends on D146398

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1766355 - part 4: Make `HTMLEditor::MoveNodeWithTransaction` and `HTMLEditor::MoveNodeToEndWithTransaction` return `MoveNodeResult` r=m_kato!

Depends on D146399

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1766355 - part 5: Make `HTMLEditor::MoveChildrenWithTransaction` and `HTMLEditor::MoveNodeOrChildrenWithTransaction` stop touching `Selection` directly r=m_kato!

Unfortunately, they call each other. Therefore, this patch updates these 2
methods once.

Depends on D146400

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1766355 - part 6: Make `HTMLEditor::MoveOneHardLineContents` return `MoveNodeResult` with caret point suggestion r=m_kato!

Depends on D146401

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1766355 - part 7: Rewrite `HTMLEditor::RemoveContainerWithTransaction` with `MoveNodeTransaction` and make it stop touching `Selection` directly r=m_kato!

Depends on D146402

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1766355 - part 8: Make `HTMLEditor::RemoveBlockContainerWithTransaction` stop touching `Selection` directly r=m_kato!

Depends on D146403

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1766355 - part 9: Make `HTMLEditor::ReplaceContainerWithTransactionInternal` and its callers use `MoveNodeTransaction` and return `CreateElementResult` r=m_kato!

Depends on D146404

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a70ca805cf3
part 1: Add `MoveNodeTransaction` to handle delete node and insert node in a transaction class instance r=m_kato
https://hg.mozilla.org/integration/autoland/rev/95f6971b0c8b
part 2: Make `MoveNodeResult` method names alinging to `Result` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/18cc1b0df88f
part 3: Make `MoveNodeResult` store caret point suggestion r=m_kato
https://hg.mozilla.org/integration/autoland/rev/f018103a7c22
part 4: Make `HTMLEditor::MoveNodeWithTransaction` and `HTMLEditor::MoveNodeToEndWithTransaction` return `MoveNodeResult` r=m_kato

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4d0bdba5d61
part 5: Make `HTMLEditor::MoveChildrenWithTransaction` and `HTMLEditor::MoveNodeOrChildrenWithTransaction` stop touching `Selection` directly r=m_kato

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3909db95d934
part 6: Make `HTMLEditor::MoveOneHardLineContents` return `MoveNodeResult` with caret point suggestion r=m_kato

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf8266e8b933
part 7: Rewrite `HTMLEditor::RemoveContainerWithTransaction` with `MoveNodeTransaction` and make it stop touching `Selection` directly r=m_kato

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d817ed26cad0
part 8: Make `HTMLEditor::RemoveBlockContainerWithTransaction` stop touching `Selection` directly r=m_kato

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a052a54eb167
part 9: Make `HTMLEditor::ReplaceContainerWithTransactionInternal` and its callers use `MoveNodeTransaction` and return `CreateElementResult` r=m_kato

Comment 16

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/0a70ca805cf3
https://hg.mozilla.org/mozilla-central/rev/95f6971b0c8b
https://hg.mozilla.org/mozilla-central/rev/18cc1b0df88f
https://hg.mozilla.org/mozilla-central/rev/f018103a7c22
https://hg.mozilla.org/mozilla-central/rev/e4d0bdba5d61
https://hg.mozilla.org/mozilla-central/rev/3909db95d934
https://hg.mozilla.org/mozilla-central/rev/cf8266e8b933
https://hg.mozilla.org/mozilla-central/rev/d817ed26cad0
https://hg.mozilla.org/mozilla-central/rev/a052a54eb167