Closed Bug 1632724 Opened 4 months ago Closed 3 months ago

Clean up around `DeleteSelectionWithTransaction()`

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla77
Tracking Status
firefox77 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

Attachments

(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,
HowToHandleCollapsedRange.

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
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

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 masayuki@d-toybox.com:
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
No longer regressed by: 1633797
Regressions: 1633797
You need to log in before you can comment on or make changes to this bug.