Make AutoInclusiveAncestorBlockElementsJoiner::Prepare() consider whether it should join the nodes or delete the leaf of found block in `Prepare()`
Categories
(Core :: DOM: Editor, task, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox82 | --- | fixed |
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
Attachments
(9 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 |
I forgot to do this before bug 1658702. This needs to be fixed for making the computation code much simpler, but maybe risky change.
Assignee | ||
Comment 1•4 years ago
|
||
For making the refactoring patch simpler, Prepare()
considers the
EditActionResult
value of its callers instead. However, this is odd
so that it just return whether the caller should keep working with
it or not as bool
result. Then, for the additional information, whether
the caller should consume the edit action handling or not, this patch
adds a new method, CanJoinBlocks()
.
Depends on D89440
Assignee | ||
Comment 2•4 years ago
|
||
Now, AutoInclusiveAncestorBlockElementsJoiner::Run()
is wrapped by a
small block in every caller. Therefore, AutoTrackDOMPoint
for it can
be moved into the small blocks.
Depends on D89571
Assignee | ||
Comment 3•4 years ago
|
||
Note that in such cases, current code mark the results as both "canceled" and
"handled", but only "canceled" state is important for the root caller.
- https://searchfox.org/mozilla-central/rev/ac142717cc067d875e83e4b1316f004f6e063a46/editor/libeditor/HTMLEditSubActionHandler.cpp#4200-4204
- https://searchfox.org/mozilla-central/rev/ac142717cc067d875e83e4b1316f004f6e063a46/editor/libeditor/HTMLEditSubActionHandler.cpp#3452-3457
- https://searchfox.org/mozilla-central/rev/ac142717cc067d875e83e4b1316f004f6e063a46/editor/libeditor/HTMLEditSubActionHandler.cpp#3100-3125
Depends on D89572
Assignee | ||
Comment 4•4 years ago
|
||
The 2 of them always mark the edit action as "handled", but it's not important
when it's "canceled" or an error. Therefore, we can make the users simpler
as this patch does.
Depends on D89573
Assignee | ||
Comment 5•4 years ago
|
||
When they return error, neither "handled" nor "canceled" state is referred.
So, for making the code simpler, we can make them return
EditActionResult(NS_ERROR_*)
instead of EditActionIgnored(NS_ERROR_*)
.
Depends on D89574
Assignee | ||
Comment 6•4 years ago
|
||
Unfortunately, HTMLEditor::MoveOneHardLineContents()
,
HTMLEditor::MoveChildrenWithTransaction()
and
HTMLEditor::MoveNodeOrChildrenWithTransaction()
return strict result whether
at least one node is moved or not. Therefore, we need to scan the DOM tree
whether there is at least one content node which can be moved by them for
computing target ranges.
We cannot do same thing for ``HTMLEditor::MoveOneHardLineContents()because it split parent elements first and use
ContentSubtreeIterator` which lists
up topmost nodes which are completely in the range, but we need to compute
target ranges without splitting nodes at the range boundaries. Therefore,
this patch checks whether the line containing specified point has content
except invisible `<br>` element.
The others are simple. We can use same logic with them.
Finally, this adds NS_ASSERTION()
s to check whether the computation is done
correctly at running any automated tests on debug build, and I don't see any
failure with them.
Depends on D89575
Assignee | ||
Comment 7•4 years ago
|
||
The relation is important when the class handles both joining them and computing
target ranges. Additionally, it's required to consider whether it can join the
block elements. So, it should store the relation when Prepare()
is called.
Depends on D89576
Assignee | ||
Comment 8•4 years ago
|
||
When its Run()
join the block elements, even if it won't move any content
nodes in first line, it may notify the callers as "handled" when there is a
preceding invisible <br>
element since it needs to delete it in any cases.
Therefore, whether there is or not a preceding invisible <br>
element is
important to compute target ranges since if there is one, Run()
won't return
"ignored" and the callers won't delete leaf content instead.
Note that the new assertions are not hit in any tests, but due to searching
preceding invisible <br>
element in Prepare()
, expected assertion
count of 2 tests is increased.
Depends on D89577
Assignee | ||
Comment 9•4 years ago
|
||
This makes AutoInclusiveAncestorBlockElementsJoiner
stores whether Run()
will return "ignored" or not when Prepare()
is called. And this patch adds
NS_ASSERTION()
to compare the result of Run()
and the guess.
Note that this shouldn't used for considering whether its user call or not
Run()
actually for the risk of regressions and if we do it with current
implementation, some web apps may be broken if they do modify the DOM tree
at white-space adjustment before joining the left and right block elements.
The method will be used by ComputeTargetRanges()
which will be implemented
by bug 1658702.
Depends on D89578
Comment 10•4 years ago
|
||
Comment 11•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4b99c491fa96
https://hg.mozilla.org/mozilla-central/rev/06680416a55d
https://hg.mozilla.org/mozilla-central/rev/64b7ca1d75ac
https://hg.mozilla.org/mozilla-central/rev/37d3602c4ea7
https://hg.mozilla.org/mozilla-central/rev/e07fe4e659a0
https://hg.mozilla.org/mozilla-central/rev/5ec20189c83b
https://hg.mozilla.org/mozilla-central/rev/bef8e416efdd
https://hg.mozilla.org/mozilla-central/rev/783abfedd69c
https://hg.mozilla.org/mozilla-central/rev/92d8da06449b
Description
•