1762115 - Make edit sub action handlers stop retrieving selection by themselves #1

Status: RESOLVED FIXED

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

Description

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

Currently, a lot of edit sub-action handlers retrieve selection by themselves. This means that doing something depends on that the previously running code changed selection. So this long standing issue makes me investigating where need do change for fixing bug 1735608.

Comment 1

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

I think that I should land the patches (currently I have 15 patches) before the holiday week in Japan. Their performance improvement must make everybody happy. However, the changes may be risky. Therefore, it's fine to back them out even in the holiday week.

Comment 2

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

Attachment: Bug 1762115 - part 1: Make `HTMLEditor::HandleInsertBRElement` stop touching `Selection` directly r=m_kato!

The strategy of this patch and most following patches is, returning candidate
caret position with CreateNodeResultBase, and make each caller choose whether
it updates Selection immediately, put it off until exiting from a loop, or
just ignore.

This patch makes HTMLEditor::HandleInsertBRElement return
CreateElementResult and makes its callers handle selection as same as the
method does.

And note that I've not realized that NS_FAILED and NS_SUCCEEDED include
MOZ_(UN)LIKELY. Therefore, they don't need to be wrapped with them. And
also NS_WARN_IF has it too (but it's not in opt build, see bug 1765909).
On the other hand, neither Result::isErr nor Result::isOk has them
(bug 1765916). Therefore, except in the case of Result, this and following
patches will remove unnecessary MOZ_(UN)LIKELY from editor.

Comment 3

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

Attachment: Bug 1762115 - part 2: Make `InsertNodeTransaction::DoTransaction()` stop touching `Selection` directly r=m_kato!

Unfortunately, we need to keep InsertNodeTransaction::RedoTransaction()
touching Selection for now because redoing Selection management is out
of scope of this bug (it's probably require big changes, but no benefits for
now).

Depends on D144644

Comment 4

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

Attachment: Bug 1762115 - part 3: Make `HTMLEditor::CreateAndInsertElement` stop touching `Selection` directly r=m_kato!

Depends on D144645

Comment 5

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

Attachment: Bug 1762115 - part 4: Make `SplitNodeTransaction::DoTransaction()` stop touching selection r=m_kato!

Depends on D144646

Comment 6

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

Attachment: Bug 1762115 - part 5: Make `SplitNodeResult` store and suggest new caret position r=m_kato!

This patch allows methods which split some nodes to return new candidate
caret position and makes callers of them consider whether applying it to
the selection immediately or not.

Depends on D144647

Comment 7

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

Attachment: Bug 1762115 - part 6: Make `HTMLEditor::SplitNodeWithTransaction` stop touching `Selection` directly r=m_kato!

Depends on D144648

Comment 8

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

Attachment: Bug 1762115 - part 7: Make `HTMLEditor::SplitNodeDeepWithTransaction` stop touching `Selection` r=m_kato!

Now, it does not require to update selection, and its result may have a point
to put caret it is used to doing. This patch makes it stop collapsing selection
after each split, and instead, makes all callers of it collapse selection if
it's (probably) necessary.

Note that the method may not split any nodes but return it as "handled".
Therefore, the callers do not check whether a splitting node occurred but
suggesting point to put caret is invalid or the other cases. Therefore, in
strictly speaking, this patch changes the existing behavior, but it should not
affects web apps in the wild because of the low usage of the legacy mutation
event listeners.

Depends on D144649

Comment 9

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

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

Depends on D144650

Comment 10

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

Attachment: Bug 1762115 - part 9: Make `HTMLEditor::InsertElementWithSplittingAncestorsWithTransaction` stop touching `Selection` directly r=m_kato!

Depends on D144651

Comment 11

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

Attachment: Bug 1762115 - part 10: Make `HTMLEditor::InsertBRElement` stop touching `Selection` directly r=m_kato!

Depends on D144652

Comment 12

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

Attachment: Bug 1762115 - part 11: Make `HTMLEditor::CopyLastEditableChildStylesWithTransaction` stop touching `Selection` directly r=m_kato!

It's called only by HTMLEditor::HandleInsertParagraphInListItemElement and
it also just returns a point to put caret computed from the result of
CopyLastEditableChildStylesWithTransaction and its caller changes the
selection. Therefore, it's enough to make it return a recommend point to
put caret.

Depends on D144653

Comment 13

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

Attachment: Bug 1762115 - part 12: Make `WhiteSpaceVisibilityKeeper::InsertBRElement` stop touching `Selection` directly r=m_kato!

Its callers will touch Selection or just returns a recommend point to put
caret.

Depends on D144654

Comment 14

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

Attachment: Bug 1762115 - part 13: Make `EditorBase::InsertNodeWithTransaction` stop touching `Selection` directly r=m_kato!

For avoiding duplicated forward declarations, this patch also creates
EditorForwards.h which is exposed under mozilla. And then, for the future
changes, this makes the method templated to the callers do not require to
convert the result if the caller wants to return CreateElementResult etc.

Depends on D144655

Comment 15

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

Attachment: Bug 1762115 - part 14: Make `HTMLEditor::InsertPaddingBRElementForEmptyLastLineWithTransaction` stop touching `Selection` directly r=m_kato!

Depends on D144656

Comment 16

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

Attachment: Bug 1762115 - part 15: Make `HTMLEditor::InsertNodeIntoProperAncestorWithTransaction()` stop touching `Selection` directly r=m_kato!

This patch also makes that HTMLWithContextInserter::InsertContents
collect moving children first. Then, move each one with a transaction.
This is standard manner in these days to avoid infinite loop caused by
moving the removed child back by a mutation event listener.

Depends on D144657

Comment 17

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

I think that I should land the patches (currently I have 15 patches) before the holiday week in Japan. Their performance improvement must make everybody happy. However, the changes may be risky. Therefore, it's fine to back them out even in the holiday week.

Comment 18

[Pulsebot]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d50ee2ce9eaa
part 2: Make `InsertNodeTransaction::DoTransaction()` stop touching `Selection` directly r=m_kato

Comment 20

[Pulsebot]

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88a64ff079e1
part 4: Make `SplitNodeTransaction::DoTransaction()` stop touching selection r=m_kato

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5cbd7d6343f
part 5: Make `SplitNodeResult` store and suggest new caret position r=m_kato

Comment 23

[Pulsebot]

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a5c8b9e6f29
part 7: Make `HTMLEditor::SplitNodeDeepWithTransaction` stop touching `Selection` r=m_kato

Comment 25

[Pulsebot]

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

Comment 26

[Pulsebot]

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

Comment 27

[Pulsebot]

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

Comment 28

[Pulsebot]

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

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0a43b0f26fc
part 12: Make `WhiteSpaceVisibilityKeeper::InsertBRElement` stop touching `Selection` directly r=m_kato

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07ef1afedce6
part 13: Make `EditorBase::InsertNodeWithTransaction` stop touching `Selection` directly r=m_kato

Comment 31

[Pulsebot]

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

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40b512ed4dbf
part 15: Make `HTMLEditor::InsertNodeIntoProperAncestorWithTransaction()` stop touching `Selection` directly r=m_kato

Comment 33

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/c60b2478b2f2
https://hg.mozilla.org/mozilla-central/rev/d50ee2ce9eaa
https://hg.mozilla.org/mozilla-central/rev/8a1b659f4092
https://hg.mozilla.org/mozilla-central/rev/88a64ff079e1
https://hg.mozilla.org/mozilla-central/rev/c5cbd7d6343f
https://hg.mozilla.org/mozilla-central/rev/498113b89022
https://hg.mozilla.org/mozilla-central/rev/4a5c8b9e6f29
https://hg.mozilla.org/mozilla-central/rev/f5426e3f86ba
https://hg.mozilla.org/mozilla-central/rev/8f8a187b3446
https://hg.mozilla.org/mozilla-central/rev/d79abc3fb657
https://hg.mozilla.org/mozilla-central/rev/cf4b29edd52f
https://hg.mozilla.org/mozilla-central/rev/d0a43b0f26fc
https://hg.mozilla.org/mozilla-central/rev/07ef1afedce6
https://hg.mozilla.org/mozilla-central/rev/5cb08d1cbdd8
https://hg.mozilla.org/mozilla-central/rev/40b512ed4dbf

Comment 34

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to Pulsebot from comment #29)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0a43b0f26fc
part 12: Make WhiteSpaceVisibilityKeeper::InsertBRElement stop touching
Selection directly r=m_kato

== Change summary for alert #34093 (as of Tue, 10 May 2022 06:19:37 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
4%	facebook loadtime	android-hw-p2-8-0-android-aarch64-shippable-qr	warm webrender	681.19 -> 651.33

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