Closed Bug 1770877 Opened 2 years ago Closed 2 years ago

Make edit sub action handlers stop retrieving selection by themselves #2

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
103 Branch
Tracking Flags:
Tracking Status
firefox103 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: perf-alert)

Attachments

(45 files)

Bug 1770877 - part 1: Make `HTMLEditor::CreateOrChangeBlockContainerElement` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 2: Make `HTMLEditor::SplitRangeOffFromBlock` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 3: Make `HTMLEditor::SplitRangeOffFromBlockAndRemoveMiddleContainer` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 4: Make `HTMLEditor::RemoveBlockContainerElements` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 5: Make `HTMLEditor::MoveNodesIntoNewBlockquoteElement` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 6: Get rid of `HTMLEditor::SplitInlinesAndCollectEditTargetNodesInOneHardLine` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 7: Get rid of `HTMLEditor::SplitInlinesAndCollectEditTargetNodesInExtendedSelectionRanges` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 8: Get rid of `HTMLEditor::CollectEditTargetNodesInExtendedSelectionRanges` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 9: Get rid of `HTMLEditor::GetSelectionRangesExtendedToIncludeAdjuscentWhiteSpaces` because of unused r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 10: Move `HTMLEditor::SelectBRElementIfCollapsedInEmptyBlock` to `AutoRangeArray` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 11: Move `HTMLEditor::GetCurrentHardLineStartPoint` and `HTMLEditor::GetCurrentHardLineEndPoint` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 12: Make `HTMLEditor` treat array of `nsRange` with `OwningNonNull` rather than `RefPtr` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 13: Move `HTMLEditor::CreateRangeExtendedToHardLineStartAndEnd` into `AutoRangeArray` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 14: Make `AutoRangeArray::CreateRangeWrappingStartAndEndLinesContainingBoundaries` use a static method which sets existing `nsRange` instance r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 15: Implement equivalent of `HTMLEditor::GetSelectionRangesExtendedToHardLineStartAndEnd` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 16: Make `AutoRangeArray` can be initialized with an `EditorDOMRangeBase` or `EditorDOMPointBase` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 17: Make `HTMLEditor::SplitElementsAtEveryBRElement` and `HTMLEditor::MaybeSplitElementsAtEveryBRElement` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 18: Make `HTMLEditor::SplitTextNodesAtRangeEnd` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 19: Make `HTMLEditor::SplitParentInlineElementsAtRangeEdges` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 20: Move `HTMLEditor::SplitTextNodesAtRangeEnd` and `HTMLEditor::SplitParentInlineElementsAtRangeEdges` into `AutoRangeArray` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 21: Get rid of `HTMLEditor::SplitInlinesAndCollectEditTargetNodes` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 22: Move `HTMLEditor::CollectEditTargetNodes` into `AutoRangeArray` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 23: Clean up callers of `AutoRangeArray::SplitTextNodesAtEndBoundariesAndParentInlineElementsAtBoundaries` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 24: Make `HTMLEditor::MaybeExtendSelectionToHardLineEdgesForBlockEditAction` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 25: Make `HTMLEditor::FormatBlockContainerWithTransaction` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 26: Make `HTMLEditor::InsertBRElementIfEmptyBlockElement` a lambda since it's used only by one method r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 27: Make `HTMLEditor::SplitParagraph` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 28-1: Isolate the part of adjusting split point for avoiding to create new empty link in `HTMLEditor::HandleInsertParagraphInParagraph` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 28-2: Make `HTMLEditor::HandleInsertParagraphInParagraph` return earlier if should be default to insert `<br>` instead r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 28-3: Make `HTMLEditor::HandleInsertParagraphInParagraph` insert `<br>` element immediately after considering so r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 28-4: Make `HTMLEditor::HandlerInsertParagraphInParagraph` take a split point and editing host r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 28-5: Get rid of unnecessary duplication of `EditorDOMPoint` in `HTMLEditor::HandleInsertParagraphInParagraph` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 28-6: Make `HTMLEditor::HandleInsertParagraphInParagraph` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 29: Make `HTMLEditor::HandleInsertParagraphInHeadingElement` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 30: Make `HTMLEditor::HandleInsertParagraphInListItemElement` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 31-1: Add WPT to check `insertParagraph` behavior r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 31-2: Make `HTMLEditor::InsertParagraphSeparatorAsSubAction` call `HandleInsertParagraphInParagraph` without `Selection` r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 31-3: Replace `insertLineBreak` variable in `HTMLEditor::InsertParagraphSeparatorAsSubAction` with a lambda r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 32: Make `HTMLEditor::InsertContainerWithTransaction` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 33: Make `HTMLEditor::RelativeFontChangeOnTextNode` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 34: Make `HTMLEditor::RemoveStyleInside` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 35: Make `HTMLEditor::SetInlinePropertyOnNode` and `HTMLEditor::SetInlinePropertyOnNodeImpl` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 36: Make `HTMLEditor::CreateStyleForInsertText` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 37: Make `HTMLEditor::HandleInsertLinefeed` stop touching `Selection` directly r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1770877 - part 38: Make `HTMLEditor::InsertParagraphSeparatorAsSubAction` stop updating `Selection` while handling its job r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review

+++ This bug was initially created as a clone of Bug #1762115 +++

The next goal is, making HTMLEditor::InsertParagraphSeparatorAsSubAction modify Selection only once after deleting selected ranges.

Although the its only caller does not need the caret position, it should return
a candidate caret position for aligning to the other methods.

The constructor of SplitRangeOffFromNodeResult is used only by the method.
Therefore, we can customize the constructor and make it store the caret
suggestion.

Depends on D149065

And this renames it to RemoveBlockContainerElementWithTransactionBetween to
explain its job simpler.

Depends on D149066

For aligning the style of similar methods, this makes it suggest caret position,
but nobody uses it.

Depends on D149067

Similar to the previous patches, nobody uses the result, but it should return
the suggestion.

Depends on D149068

It's used only by HTMLEditor::MoveOneHardLineContentsWithTransaction and
it just calls 2 methods with some simple additional error handlings.

I'd like to make SplitInlinesAndCollectEditTargetNodes work with
AutoRangeArray. Therefore, I'd like to do this for avoiding to use
AutoRangeArray in HTMLEditor.h.

Depends on D149069

Similar to the previous patch, but there are more callers than the previous one,
we should unwrap it temporarily for the following patches.

Depends on D149070

This must make you feel odd. However, this is a separate patch for making easier
to review the following patches. This will be a private member of
AutoRangeArray.

Depends on D149073

Similar to the previous patch, you must feel odd to move the methods. They
will be static methods in the EditorUtils.cpp file later.

Depends on D149074

AutoRangeArran uses OwningNonNull. So making the following patch simpler
and smaller, all places should use array of OwningNonNull<nsRange> instead of
RefPtr<nsRange>.

Depends on D149075

This patch makes them take const Element& aEditingHost so that this patch
becomes bigger than what this patch does.

Depends on D149076

For making extending the range to wrap the lines containing start and end
boundaries of each range of AutoRangeArray::mRanges, we need a method which
updates existing nsRange instances rather than creating new one.

Depends on D149077

It's always work with Selection ranges. Then, it should be treated within
AutoRangeArray which can be initialized with Selection ranges automatically.
Then, we can making HTMLEditor simpler.

Note tha this tries to make it get editing host when the corresponding
public method is called (after dispatched beforeinput event if handling a
users' operation). However, as commented in
HTMLEditor::SetParagraphFormatAsAction, it breaks a tricky crash test.
Therefore, only the format block path computes it after the preparation.

Depends on D149078

Although they update the DOM tree with transactions, but for making HTMLEditor
smaller, and the following patches makes the related code simpler, we need to
move them into AutoRangeArray.

Depends on D149083

It's callers become messy with this patch. The following patches try to make
them smaller.

Depends on D149084

And also this moves HTMLEditor::CollectChildren to HTMLEditUtils.

Depends on D149085

They at most twice updates the Selection right now. We can make them update
Selection once. And this patch makes the scope of AutoRangeArray clearer.

Depends on D149086

This patch makes it take AutoRangeArray and it can save a snapshot of the
ranges and restore it like AutoSelectionRestorer.

For accessing HTMLEditor::RangeUpdaterRef for clearing the saved ranges,
AutoRangeArray needs to store HTMLEditor. Therefore, its constructors
and the destructor cannot be inlined in the header due to the include-hell.

Depends on D149088

And its selection update are not used except the legacy mutation event
listeners so that we can drop them.

Depends on D149089

The method does several different things. For making each part isolated,
this patch wraps the first part with a lambda.

Depends on D149091

If it needs to insert a <br> but the editor does not want to create new
paragraph, its caller should be default to insert a <br> element. Rather
than checking this at inserting <br> element, it should do it at considering
the place to insert the new <br>.

Depends on D149092

Using the wide scope EditorDOMPoint pointToInsertBR makes it harder to read.
Although this duplicates a call of HTMLEditor::InsertBRElement and a couple of
post processing, but I think that it's easier to read/understand.

Depends on D149093

I tried to make the latter half preparation to call
HTMLEditor::SplitParagraphWithTransaction, but I cannot make it cleaner
because it needs to return HTMLBRElement* and EditorDOMPoint with maybe
modifying the DOM tree. So, I keep it as-is, but I make replace the unnecessary
duplicated EditorDOMPoint with the original one which is adjusted to avoid
to create new empty link in the right paragraph.

Depends on D149095

It touches Selection redundantly (i.e., immediately after that, it or its
caller collapse Selection). So we can just stop it because we can ignore
the cases when the handling fails after the redundant Selection update and
it can be caused by tricky things with the mutation event listeners.

Note that without changing SplitNodeResult a lot, we cannot make it return
SplitNodeResult, and currently there is no motivation to do it because the
only caller does not need the detail of the split. Therefore, I give up doing
it.

Depends on D149098

The expectations are exactly same as Chrome's behavior. And checked by my
hands briefly, Safari also behaves as same as Chrome.

Depends on D149099

The preceding call of InsertBRElement may collapse selection at the inserted
padding <br> element. Only when calling HandleInsertParagraphInParagraph,
InsertParagraphSeparatorAsSubAction uses Selection. So, only in this case,
we need to recompute the point to split for keeping current (odd) behavior.

Note that in normal cases, using atStartOfSelection gets same result.
However, if there are some invisible nodes such as comment nodes, doing it
may change the behavior. For now, we should keep the current behavior. It
should be sorted out when we make it stop inserting <br> elements for the
preparation of split without checking whether it's actually necessary.

Depends on D149100

This changes the behavior a little bit, unfortunately.

InsertContainerWithTransaction starts return code. And the behavior changed
WPTs are hit here:
https://searchfox.org/mozilla-central/rev/a8bdd0feeb7ae596a202333ee324a68153f9f4c4/editor/libeditor/EditorBase.cpp#2179-2180

Then, the editor specific error will be converted to NS_OK at the
corresponding public method. Therefore, the return code may be changed from
NS_ERROR_FAILURE to NS_OK, and that causes the return value change of
Document.execCommand.

Depends on D149102

Note that CSSEditUtils does not change Selection except
RemoveCSSInlineStyleWithTransaction which is used only by aligning in a block.
Therefore, this patch does not touch CSSEditUtils.

Depends on D149105

Note that the odd path which always returns NS_ERROR_FAILURE is not covered by
the tests. Therefore, this patch adds MOZ_ASSERT to make somebody hit it and
report a bug.

Depends on D149107

Finally, this patch makes it update Selection once after finishing its
preparation.

Depends on D149108

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b93803e04862
part 1: Make `HTMLEditor::CreateOrChangeBlockContainerElement` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/af21a3acf888
part 2: Make `HTMLEditor::SplitRangeOffFromBlock` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3dd0175024bc
part 3: Make `HTMLEditor::SplitRangeOffFromBlockAndRemoveMiddleContainer` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/32950f477d43
part 4: Make `HTMLEditor::RemoveBlockContainerElements` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/0d007ba4578d
part 5: Make `HTMLEditor::MoveNodesIntoNewBlockquoteElement` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/87c738ac3a60
part 6: Get rid of `HTMLEditor::SplitInlinesAndCollectEditTargetNodesInOneHardLine` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/024fd3a7ed03
part 7: Get rid of `HTMLEditor::SplitInlinesAndCollectEditTargetNodesInExtendedSelectionRanges` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/59d6322396d3
part 8: Get rid of `HTMLEditor::CollectEditTargetNodesInExtendedSelectionRanges` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d912ee58ca33
part 9: Get rid of `HTMLEditor::GetSelectionRangesExtendedToIncludeAdjuscentWhiteSpaces` because of unused r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/905516eade8b
part 10: Move `HTMLEditor::SelectBRElementIfCollapsedInEmptyBlock` to `AutoRangeArray` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4fd4b04570e6
part 11: Move `HTMLEditor::GetCurrentHardLineStartPoint` and `HTMLEditor::GetCurrentHardLineEndPoint` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/e625100b0a85
part 12: Make `HTMLEditor` treat array of `nsRange` with `OwningNonNull` rather than `RefPtr` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b874507c919f
part 13: Move `HTMLEditor::CreateRangeExtendedToHardLineStartAndEnd` into `AutoRangeArray` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/6078753cb890
part 14: Make `AutoRangeArray::CreateRangeWrappingStartAndEndLinesContainingBoundaries` use a static method which sets existing `nsRange` instance r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4f0f0addd055
part 15: Implement equivalent of `HTMLEditor::GetSelectionRangesExtendedToHardLineStartAndEnd` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/33352a8ca7b4
part 16: Make `AutoRangeArray` can be initialized with an `EditorDOMRangeBase` or `EditorDOMPointBase` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/5021cfa2df4a
part 17: Make `HTMLEditor::SplitElementsAtEveryBRElement` and `HTMLEditor::MaybeSplitElementsAtEveryBRElement` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/a07aafab6392
part 18: Make `HTMLEditor::SplitTextNodesAtRangeEnd` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/c7d39fc1c51a
part 19: Make `HTMLEditor::SplitParentInlineElementsAtRangeEdges` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4645504a0995
part 20: Move `HTMLEditor::SplitTextNodesAtRangeEnd` and `HTMLEditor::SplitParentInlineElementsAtRangeEdges` into `AutoRangeArray` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/74aeec5ca9a9
part 21: Get rid of `HTMLEditor::SplitInlinesAndCollectEditTargetNodes` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3738f1426b07
part 22: Move `HTMLEditor::CollectEditTargetNodes` into `AutoRangeArray` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/cc7dffd956f9
part 23: Clean up callers of `AutoRangeArray::SplitTextNodesAtEndBoundariesAndParentInlineElementsAtBoundaries` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/5a257be173a4
part 24: Make `HTMLEditor::MaybeExtendSelectionToHardLineEdgesForBlockEditAction` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/a1a871c03b3d
part 25: Make `HTMLEditor::FormatBlockContainerWithTransaction` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/54aff5bcc909
part 26: Make `HTMLEditor::InsertBRElementIfEmptyBlockElement` a lambda since it's used only by one method r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/500b28ff7eb6
part 27: Make `HTMLEditor::SplitParagraph` stop touching `Selection` directly r=m_kato

(In reply to Pulsebot from comment #80)

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/500b28ff7eb6
part 27: Make HTMLEditor::SplitParagraph stop touching Selection
directly r=m_kato

== Change summary for alert #34584 (as of Tue, 21 Jun 2022 04:47:33 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
17% ebay loadtime macosx1015-64-shippable-qr cold fission webrender 468.25 -> 387.46
17% microsoft loadtime windows10-64-shippable-qr cold fission webrender 295.12 -> 245.38
17% microsoft ContentfulSpeedIndex windows10-64-shippable-qr cold fission webrender 346.00 -> 288.92
16% microsoft loadtime linux1804-64-shippable-qr cold fission webrender 379.15 -> 317.42
16% ebay fcp linux1804-64-shippable-qr cold fission webrender 376.29 -> 317.17
... ... ... ... ...
5% expedia loadtime linux1804-64-shippable-qr cold fission webrender 1,566.31 -> 1,494.67

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

Quite surprising, that alert, but... yay?

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/a672ee6e35fc
part 28-1: Isolate the part of adjusting split point for avoiding to create new empty link in `HTMLEditor::HandleInsertParagraphInParagraph` r=m_kato

(In reply to Emilio Cobos Álvarez (:emilio) from comment #83)

Quite surprising, that alert, but... yay?

My patches omit redundant Selection updates while the editor touches the DOM tree multiple times at handling one edit. Therefore, they may improve the performance of benchmarks if they use editing API.

Note that, in strictly speaking, my patches change the behavior from point of view of legacy DOM mutation event listeners. They could cause breaking existing apps, but such apps should be rare because the other browsers put the events into the queue (like nsContentUtils::AddScriptRunner) and dispatches them after finishing to touch the DOM tree, so, the changes should work with actual web apps in the wild.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/2e8167df4d77
part 28-2: Make `HTMLEditor::HandleInsertParagraphInParagraph` return earlier if should be default to insert `<br>` instead r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/f18761f6020d
part 28-3: Make `HTMLEditor::HandleInsertParagraphInParagraph` insert `<br>` element immediately after considering so r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/e7941e3d1c02
part 28-4: Make `HTMLEditor::HandlerInsertParagraphInParagraph` take a split point and editing host r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/631abbaad8b1
part 28-5: Get rid of unnecessary duplication of `EditorDOMPoint` in `HTMLEditor::HandleInsertParagraphInParagraph` r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/6572b95e8870
part 28-6: Make `HTMLEditor::HandleInsertParagraphInParagraph` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/9f542033a195
part 29: Make `HTMLEditor::HandleInsertParagraphInHeadingElement` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/27233700181a
part 30: Make `HTMLEditor::HandleInsertParagraphInListItemElement` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/2922b0526888
part 31-1: Add WPT to check `insertParagraph` behavior r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e71eb63a2c41
part 31-2: Make `HTMLEditor::InsertParagraphSeparatorAsSubAction` call `HandleInsertParagraphInParagraph` without `Selection` r=m_kato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/34516 for changes under testing/web-platform/tests
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/192e532ea403
part 31-3: Replace `insertLineBreak` variable in `HTMLEditor::InsertParagraphSeparatorAsSubAction` with a lambda r=m_kato

(In reply to Pulsebot from comment #48)

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3dd0175024bc
part 3: Make HTMLEditor::SplitRangeOffFromBlockAndRemoveMiddleContainer
stop touching Selection directly r=m_kato

== Change summary for alert #34538 (as of Fri, 17 Jun 2022 04:12:44 GMT) ==

Regressions:

Ratio Test Platform Options Absolute values (old vs new)
19% ebay fcp linux1804-64-shippable-qr cold fission webrender 318.90 -> 379.42
17% ebay loadtime linux1804-64-shippable-qr cold fission webrender 472.54 -> 552.79
12% ebay FirstVisualChange windows10-64-shippable-qr cold fission webrender 332.54 -> 372.33
4% ebay PerceptualSpeedIndex windows10-64-shippable-qr cold fission webrender 1,146.79 -> 1,186.92

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

Merging the perf regressions of part 3 (comment 97), and the perf improvements of part 27 (comment 82),
ebay fcp (Linux): 318.9 -> 317.2
ebay loadtime (Linux): 472.54 -> 473.9
ebay FirstVisualChange (Win): 332.54 -> 372.33 (not improved by part 27)
ebay PerceptualSpeedIndex (Win): 1,146.79 -> 1,186.92 (not improved by part 27)

I have no idea how the part.3 causes the regression because it just moves what it does to its caller. Could cause a failure of optimization?

(I wonder, why is the score of load time tests and visual change tests affected by the changes in the complicated editing path? part 27 is in a path of common scenario like using execCommand("insertParagraph"), but part 3 is mainly used by formatting block element command which are typically not used in the wild.

And I don't find any nodes matching with document.querySelectorAll("[contenteditable]") in ebay...)

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ff2f12be9907
part 32: Make `HTMLEditor::InsertContainerWithTransaction` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3652f1676211
part 33: Make `HTMLEditor::RelativeFontChangeOnTextNode` stop touching `Selection` directly r=m_kato

I accessed both https://www.ebay.com/ and https://www.ebay.com/deals with debug build which is attached by the debugger to stop if the methods run in the process. However, the break points are not hit. So, I think that at least the score changes of ebay's cases shouldn't be caused by the patches for this bug.

I also don't see elements matching with [contenteditable] in the main document of both microsoft and expedia. So, the improvement is also odd to me.

Upstream PR merged by moz-wptsync-bot
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/757a2158f293
part 34: Make `HTMLEditor::RemoveStyleInside` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/f5a4ea36e50b
part 35: Make `HTMLEditor::SetInlinePropertyOnNode` and `HTMLEditor::SetInlinePropertyOnNodeImpl` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b7064633d353
part 36: Make `HTMLEditor::CreateStyleForInsertText` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/2b6f258080b1
part 37: Make `HTMLEditor::HandleInsertLinefeed` stop touching `Selection` directly r=m_kato
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/e9b6a7f08dc9
part 38: Make `HTMLEditor::InsertParagraphSeparatorAsSubAction` stop updating `Selection` while handling its job r=m_kato

Backed out changeset e9b6a7f08dc9 for causing wpt failures on editing-div-outside-body.html

Backout link

Push with failures

Failure log

Flags: needinfo?(masayuki)

Thank you.

Sigh, it's disabled in debug build, that's why I didn't realize the regression in try server.

And the reason of failure is, probably aEditingHost is limited in the <body> element and AutoRangeArray::EnsureOnlyEditableRanges does not allow outside it. Currently, it's tested only whether the container is editable or not. So I think we should use another editing host which is not limited in the <body> only at calling AutoRangeArray::EnsureOnlyEditableRanges.

Flags: needinfo?(masayuki)
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/7f64e67f3144
part 38: Make `HTMLEditor::InsertParagraphSeparatorAsSubAction` stop updating `Selection` while handling its job r=m_kato
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox103: --- → fixed
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: