Closed Bug 1632724 Opened 4 years ago Closed 4 years ago

Clean up around `DeleteSelectionWithTransaction()`


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




Tracking Status
firefox77 --- fixed


(Reporter: masayuki, Assigned: masayuki)


(Regressed 1 open bug)



(5 files)

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

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

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,

Depends on D72291

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

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

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

Pushed by
part 1: Move common methods for handling delete selection to `EditorBase` r=m_kato
part 2: Move `DeleteSelectionWithTransaction()` from `TextEditor` to `EditorBase` since it's used by `HTMLEditor` too r=m_kato
part 3: Make helper methods of `EditorBase::DeleteSelectionWithTransaction()` take new `enum class` instead of `nsIEditor::EDirection` r=m_kato
part 4: Rename `HTMLEditor::DeleteSelectionWithTransaction()` r=m_kato
part 5: Remove out params of `EditorBase::CreateTransactionForDelete*()` r=m_kato
No longer regressed by: 1633797
Regressions: 1633797
You need to log in before you can comment on or make changes to this bug.