1632724 - Clean up around `DeleteSelectionWithTransaction()`

Status: RESOLVED FIXED

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

Description

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

I'd like to land some patches to clean up around DeleteSelectionWithTransaction() which are written for bug 1618457.

Comment 1

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

Attachment: Bug 1632724 - part 1: Move common methods for handling delete selection to `EditorBase` r=m_kato!

DeleteSelection*() are members of TextEditor, but they are also used by
HTMLEditor. Therefore, they and pref cache members for them should be
in EditorBase too.

Depends on D71911

Comment 2

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

Attachment: Bug 1632724 - part 2: Move `DeleteSelectionWithTransaction()` from `TextEditor` to `EditorBase` since it's used by `HTMLEditor` too r=m_kato!

Depends on D72290

Comment 3

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

Attachment: Bug 1632724 - part 3: Make helper methods of `EditorBase::DeleteSelectionWithTransaction()` take new `enum class` instead of `nsIEditor::EDirection` r=m_kato!

The parameter is used only by EditorBase::CreateTxnForDeleteRange() to
extend collapsed range, but it accepts only nsIEditor::eNext and
nsIEditor::ePrevious. Therefore, using nsIEditor::EDirection does not
make sense. Instead, they should use new enum class,
HowToHandleCollapsedRange.

Depends on D72291

Comment 4

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

Attachment: Bug 1632724 - part 4: Rename `HTMLEditor::DeleteSelectionWithTransaction()` r=m_kato!

HTMLEditor::DeleteSelectionWithTransaction() calls EditorBase's overridden
method and handles nsIEditor::eStrip case. Therefore, we can rename it with
stop calling the EditorBase::DeleteSelectionWithTransaction(), and make it
called by EditorBase::DeleteSelectionWithTransaction() only when it's
necessary.

Additionally, we can make all internal method callers of editor classes always
set nsIEditor::eNoStrip if the instance is TextEditor. This must make
the code easier to understand.

Depends on D72292

Comment 5

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

Attachment: Bug 1632724 - part 5: Remove out params of `EditorBase::CreateTransactionForDelete*()` r=m_kato!

The out params mean the last collapsed selection range's result (although,
the meaning is odd and offset and length are not overwritten when there is
another collapsed range and it causes DeleteNodeTransaction). Additionally,
when and only when DeleteNodeTransaction and DeleteTextTransaction are
added to the EditAggregationTransaction created by
CreateTransactionForSelection(). Therefore, same result can be looked for
from its only caller, DeleteSelectionWithTransaction().

Note that this makes the method slower if there are too many selection ranges,
but such case must be rare because:

1. We can assume that users rarely use multiple selection ranges for removing
   multiple ranges of content except table.
2. Multiple selection is supported only by Gecko. Therefore, web apps must
   not use multiple selection for this purpose.

So, it must be okay to use this slower approach for making the methods simpler.
If it'd become damage to some benchmarks, let's create faster access to get
transaction type.

Depends on D72293

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f234ed7095b
part 1: Move common methods for handling delete selection to `EditorBase` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/2b18958a59bb
part 2: Move `DeleteSelectionWithTransaction()` from `TextEditor` to `EditorBase` since it's used by `HTMLEditor` too r=m_kato
https://hg.mozilla.org/integration/autoland/rev/06cd5b2202f6
part 3: Make helper methods of `EditorBase::DeleteSelectionWithTransaction()` take new `enum class` instead of `nsIEditor::EDirection` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/0433ceeed6a8
part 4: Rename `HTMLEditor::DeleteSelectionWithTransaction()` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/f6310e0f0afd
part 5: Remove out params of `EditorBase::CreateTransactionForDelete*()` r=m_kato

Comment 7

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/9f234ed7095b
https://hg.mozilla.org/mozilla-central/rev/2b18958a59bb
https://hg.mozilla.org/mozilla-central/rev/06cd5b2202f6
https://hg.mozilla.org/mozilla-central/rev/0433ceeed6a8
https://hg.mozilla.org/mozilla-central/rev/f6310e0f0afd