1797247 - Make `DeleteRangeTransaction` and `DeleteTextTransaction` stop updating `Selection`

Status: RESOLVED FIXED

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

Comment 1

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

The deleting selection code in HTMLEditor is really hard to understand. One of the reasons of that is a lot of methods including the transaction classes' DoTransaction updates Selection immediately and cannot be controlled in root caller side. Therefore, I should refactor it when I have spare time.

Comment 2

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

I read the code around there a bit in my spare time.

DeleteRangeTransaction contains only DeleteNodeTransaction and DeleteTextTransaction. Perhaps, adding MOZ_ASSERT_IF check into EditAggretateTransaction to make this guaranteed at least in debug builds.

DeleteNodeTransaction does not touch Selection, therefore, we don't need to touch it.

DeleteTextTransaction::DoTransaction collapses Selection to start of the deletion range.

DeleteRangeTransaction::DoTransaction overwrites Selection of DeleteTextTransaction.

EditorBase::CreateTransactionForDeleteSelection creates EditAggregateTransaction for containing only DeleteRangeTransaction, DeleteNodeTransaction and DeleteTextTransaction. For making this safer at build time, DeleteNodeTransaction and DeleteTextTransaction should have DeleteContentTransaction as common base class. Then, the type check in EditAggretateTransaction can be simpler.

EditorBase::DeleteRangesWithTransaction is the only user of EditorBase::CreateTransactionForDeleteSelection, and it needs to collapse Selection to where the last executed DeleteRangeTransaction or DeleteTextTransaction wants to collapse. Therefore, I want a method to get an API to get selection to the result of CreateTransactionForDeleteSelection. However, it's also a base class of PlaceholderTransaction which is created a lot. Therefore, I don't like it'll have new member(s). I think that new sub class of EditAggregateTransaction is required such as DeleteMultipleRangesTransaction.

Comment 3

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

Attachment: Bug 1797247 - part 1: Add delete transaction classes to use build time type checks r=m_kato!

First, EditorBase::CreateTransactionForDeleteSelection returns an instance of
EditAggregateTransaction. It's a base class of PlaceholderTransaction and
DeleteRangeTransaction but it's also a concrete class. However, it's too
generic. Therefore, this patch creates DeleteMultipleRangesTransaction which
is a simple sub-class of it, and makes EditAggregateTransaction be an abstract
class. Then, add AddChild methods to each concrete class to restrict the
type of child transactions.

Next, DeleteRangeTransaction contains only DeleteNodeTransaction and
DeleteTextTransaction. Therefore, once they have a common base class,
we can check the type easier. Therefore, this patch also adds
DeleteContentTransactionBase and
EditorBase::CreateTransactionForCollapsedRange becomes clearer what it
returns.

With these changes, DeleteRangeTransaction obviously contains only
DeleteContentTransactionBase, DeleteMultipleRangesTransaction contains only
DeleteRangeTransaction, DeleteNodeTransaction and DeleteTextTransaction.
And they are guaranteed at build time (at least from outside the classes).

---

fix

Depends on D169037

Comment 4

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

Attachment: Bug 1797247 - part 2: Make `DeleteTextTransaction::DoTransaction` and `DeleteRangeTransaction::DoTransaction` stop updating `Selection` directly r=m_kato!

Depends on D169038

Comment 5

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

Attachment: Bug 1797247 - part 3: Make `EditorBase::DeleteRangesWithTransaction` do post-processing without `Selection` r=m_kato!

Due to the loose error checks, this patch changes the behavior in edge cases.
E.g., if there is a case that Selection is extended by some unexpected JS
listeners/observers, the result may be different. However, it must be not a
problem because normal web apps should handle it later.

Depends on D169039

Comment 6

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

Attachment: Bug 1797247 - part 4: Make `EditorBase::DeleteRangesWithTransaction` not touch `Selection` directly r=m_kato!

Depends on D169040

Comment 7

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

Attachment: Bug 1797247 - part 5: Make `EditorBase::DeleteTextWithTransaction` and `HTMLEditor::DeleteTextWithTransaction` stop touching `Selection` directly r=m_kato!

Depends on D169041

Comment 8

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

Attachment: Bug 1797247 - part 6: Make `AutoDeleteRangesHandler::FallbackToDeleteRangesWithTransaction` stop touching `Selection` directly r=m_kato!

Depends on D169042

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/363d2f6f523a
part 1: Add delete transaction classes to use build time type checks r=m_kato

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c5fbabd76ea
part 2: Make `DeleteTextTransaction::DoTransaction` and `DeleteRangeTransaction::DoTransaction` stop updating `Selection` directly r=m_kato

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99d62e2f0002
part 3: Make `EditorBase::DeleteRangesWithTransaction` do post-processing without `Selection` r=m_kato

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/716218023d3e
part 4: Make `EditorBase::DeleteRangesWithTransaction` not touch `Selection` directly r=m_kato

Comment 13

[Pulsebot]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/428d580e81db
part 6: Make `AutoDeleteRangesHandler::FallbackToDeleteRangesWithTransaction` stop touching `Selection` directly r=m_kato

Comment 15

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/363d2f6f523a
https://hg.mozilla.org/mozilla-central/rev/2c5fbabd76ea
https://hg.mozilla.org/mozilla-central/rev/99d62e2f0002
https://hg.mozilla.org/mozilla-central/rev/716218023d3e
https://hg.mozilla.org/mozilla-central/rev/606a58dc8f93
https://hg.mozilla.org/mozilla-central/rev/428d580e81db