Make edit sub action handlers stop retrieving selection by themselves #2
Categories
(Core :: DOM: Editor, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox103 | --- | fixed |
People
(Reporter: masayuki, Assigned: masayuki)
References
(Blocks 1 open bug)
Details
(Keywords: perf-alert)
Attachments
(45 files)
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
+++ 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.
Assignee | ||
Comment 1•2 years ago
|
||
Although the its only caller does not need the caret position, it should return
a candidate caret position for aligning to the other methods.
Assignee | ||
Comment 2•2 years ago
|
||
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
Assignee | ||
Comment 3•2 years ago
|
||
And this renames it to RemoveBlockContainerElementWithTransactionBetween
to
explain its job simpler.
Depends on D149066
Assignee | ||
Comment 4•2 years ago
|
||
For aligning the style of similar methods, this makes it suggest caret position,
but nobody uses it.
Depends on D149067
Assignee | ||
Comment 5•2 years ago
|
||
Similar to the previous patches, nobody uses the result, but it should return
the suggestion.
Depends on D149068
Assignee | ||
Comment 6•2 years ago
|
||
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
Assignee | ||
Comment 7•2 years ago
|
||
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
Assignee | ||
Comment 8•2 years ago
|
||
Depends on D149071
Assignee | ||
Comment 9•2 years ago
|
||
Depends on D149072
Assignee | ||
Comment 10•2 years ago
|
||
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
Assignee | ||
Comment 11•2 years ago
|
||
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
Assignee | ||
Comment 12•2 years ago
|
||
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
Assignee | ||
Comment 13•2 years ago
|
||
This patch makes them take const Element& aEditingHost
so that this patch
becomes bigger than what this patch does.
Depends on D149076
Assignee | ||
Comment 14•2 years ago
|
||
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
Assignee | ||
Comment 15•2 years ago
|
||
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
Assignee | ||
Comment 16•2 years ago
|
||
Depends on D149079
Assignee | ||
Comment 17•2 years ago
|
||
Depends on D149080
Assignee | ||
Comment 18•2 years ago
|
||
Depends on D149081
Assignee | ||
Comment 19•2 years ago
|
||
Depends on D149082
Assignee | ||
Comment 20•2 years ago
|
||
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
Assignee | ||
Comment 21•2 years ago
|
||
It's callers become messy with this patch. The following patches try to make
them smaller.
Depends on D149084
Assignee | ||
Comment 22•2 years ago
|
||
And also this moves HTMLEditor::CollectChildren
to HTMLEditUtils
.
Depends on D149085
Assignee | ||
Comment 23•2 years ago
|
||
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
Assignee | ||
Comment 24•2 years ago
|
||
Depends on D149087
Assignee | ||
Comment 25•2 years ago
|
||
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
Assignee | ||
Comment 26•2 years ago
|
||
And its selection update are not used except the legacy mutation event
listeners so that we can drop them.
Depends on D149089
Assignee | ||
Comment 27•2 years ago
|
||
Depends on D149090
Assignee | ||
Comment 28•2 years ago
|
||
The method does several different things. For making each part isolated,
this patch wraps the first part with a lambda.
Depends on D149091
Assignee | ||
Comment 29•2 years ago
|
||
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
Assignee | ||
Comment 30•2 years ago
|
||
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
Assignee | ||
Comment 31•2 years ago
|
||
Depends on D149094
Assignee | ||
Comment 32•2 years ago
|
||
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
Assignee | ||
Comment 33•2 years ago
|
||
Depends on D149096
Assignee | ||
Comment 34•2 years ago
|
||
Depends on D149097
Assignee | ||
Comment 35•2 years ago
|
||
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
Assignee | ||
Comment 36•2 years ago
|
||
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
Assignee | ||
Comment 37•2 years ago
|
||
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
Assignee | ||
Comment 38•2 years ago
|
||
This can encapsule the long if-elseif-else blocks into the lambda.
Depends on D149101
Assignee | ||
Comment 39•2 years ago
|
||
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
Assignee | ||
Comment 40•2 years ago
|
||
Depends on D149103
Assignee | ||
Comment 41•2 years ago
|
||
Depends on D149104
Assignee | ||
Comment 42•2 years ago
|
||
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
Assignee | ||
Comment 43•2 years ago
|
||
Depends on D149106
Assignee | ||
Comment 44•2 years ago
|
||
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
Assignee | ||
Comment 45•2 years ago
|
||
Finally, this patch makes it update Selection
once after finishing its
preparation.
Depends on D149108
Assignee | ||
Updated•2 years ago
|
Comment 46•2 years ago
|
||
Comment 47•2 years ago
|
||
Comment 48•2 years ago
|
||
Comment 49•2 years ago
|
||
Comment 50•2 years ago
|
||
Comment 51•2 years ago
|
||
bugherder |
Comment 52•2 years ago
|
||
Comment 53•2 years ago
|
||
Comment 54•2 years ago
|
||
Comment 55•2 years ago
|
||
Comment 56•2 years ago
|
||
Comment 57•2 years ago
|
||
Comment 58•2 years ago
|
||
Comment 59•2 years ago
|
||
Comment 60•2 years ago
|
||
Comment 61•2 years ago
|
||
bugherder |
Comment 62•2 years ago
|
||
Comment 63•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d912ee58ca33
https://hg.mozilla.org/mozilla-central/rev/905516eade8b
https://hg.mozilla.org/mozilla-central/rev/4fd4b04570e6
https://hg.mozilla.org/mozilla-central/rev/e625100b0a85
https://hg.mozilla.org/mozilla-central/rev/b874507c919f
https://hg.mozilla.org/mozilla-central/rev/6078753cb890
Comment 64•2 years ago
|
||
bugherder |
Comment 65•2 years ago
|
||
Comment 66•2 years ago
|
||
bugherder |
Comment 67•2 years ago
|
||
Comment 68•2 years ago
|
||
Comment 69•2 years ago
|
||
bugherder |
Comment 70•2 years ago
|
||
bugherder |
Comment 71•2 years ago
|
||
Comment 72•2 years ago
|
||
Comment 73•2 years ago
|
||
Comment 74•2 years ago
|
||
Comment 75•2 years ago
|
||
Comment 76•2 years ago
|
||
Comment 77•2 years ago
|
||
bugherder |
Comment 78•2 years ago
|
||
Comment 79•2 years ago
|
||
Comment 80•2 years ago
|
||
Comment 81•2 years ago
|
||
bugherder |
Comment 82•2 years ago
|
||
(In reply to Pulsebot from comment #80)
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/500b28ff7eb6
part 27: MakeHTMLEditor::SplitParagraph
stop touchingSelection
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
Updated•2 years ago
|
Comment 83•2 years ago
|
||
Quite surprising, that alert, but... yay?
Comment 84•2 years ago
|
||
bugherder |
Comment 85•2 years ago
|
||
Assignee | ||
Comment 86•2 years ago
|
||
(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.
Comment 87•2 years ago
|
||
Comment 88•2 years ago
|
||
Comment 89•2 years ago
|
||
Comment 90•2 years ago
|
||
Comment 91•2 years ago
|
||
Comment 92•2 years ago
|
||
Comment 93•2 years ago
|
||
Comment 94•2 years ago
|
||
Comment 96•2 years ago
|
||
Comment 97•2 years ago
|
||
(In reply to Pulsebot from comment #48)
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3dd0175024bc
part 3: MakeHTMLEditor::SplitRangeOffFromBlockAndRemoveMiddleContainer
stop touchingSelection
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
Assignee | ||
Comment 98•2 years ago
|
||
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?
Assignee | ||
Comment 99•2 years ago
|
||
(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...)
Comment 100•2 years ago
|
||
Comment 101•2 years ago
|
||
Assignee | ||
Comment 102•2 years ago
|
||
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.
Assignee | ||
Comment 103•2 years ago
|
||
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.
Comment 104•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a672ee6e35fc
https://hg.mozilla.org/mozilla-central/rev/2e8167df4d77
https://hg.mozilla.org/mozilla-central/rev/f18761f6020d
https://hg.mozilla.org/mozilla-central/rev/e7941e3d1c02
https://hg.mozilla.org/mozilla-central/rev/631abbaad8b1
https://hg.mozilla.org/mozilla-central/rev/6572b95e8870
https://hg.mozilla.org/mozilla-central/rev/9f542033a195
https://hg.mozilla.org/mozilla-central/rev/27233700181a
https://hg.mozilla.org/mozilla-central/rev/2922b0526888
https://hg.mozilla.org/mozilla-central/rev/e71eb63a2c41
https://hg.mozilla.org/mozilla-central/rev/192e532ea403
Comment 106•2 years ago
|
||
Comment 107•2 years ago
|
||
Comment 108•2 years ago
|
||
Comment 109•2 years ago
|
||
Comment 110•2 years ago
|
||
Comment 111•2 years ago
|
||
bugherder |
Comment 112•2 years ago
|
||
Backed out changeset e9b6a7f08dc9 for causing wpt failures on editing-div-outside-body.html
Assignee | ||
Comment 113•2 years ago
|
||
Thank you.
Sigh, it's disabled in debug build, that's why I didn't realize the regression in try server.
Assignee | ||
Comment 114•2 years ago
|
||
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
.
Comment 115•2 years ago
|
||
Comment 116•2 years ago
|
||
bugherder |
Assignee | ||
Updated•2 years ago
|
Description
•