Closed Bug 1663370 Opened 4 years ago Closed 4 years ago

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)

Product:
Core
Component:
DOM: Editor
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
82 Branch
Tracking Flags:
Tracking Status
firefox82 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(9 files)

Bug 1663370 - part 1: Make `AutoInclusiveAncestorBlockElementsJoiner::Prepare()` stop returning `EditActionResult` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1663370 - part 2: Move `AutoTrackDOMPoint` for `AutoInclusiveAncestorBlockElementsJoiner::Run()` into the minimum scope r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1663370 - part 3: Use early-return style when `AutoInclusiveAncestorBlockElementsJoiner::Prepare()` returns `false` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1663370 - part 4: Sort out "handled" state settings around `AutoInclusiveAncestorBlockElementsJoiner` users r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1663370 - part 5: Make helper methods of `AutoInclusiveAncestorBlockElementsJoiner::Run()` use simple constructor of `EditActionResult` when they return error r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1663370 - part 6: Create a method to check whether `HTMLEditor::MoveOneHardLineContents()` returns "handled" or "canceled" r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1663370 - part 7: Make `AutoInclusiveAncestorBlockElementsJoiner::Prepare()` check the relation of its `mLeftBlockElement` and `mRightBlockElement` r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1663370 - part 8: Make `AutoInclusiveAncestorBlockElementsJoiner::Prepare()` scan preceding invisible `<br>` element r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1663370 - part 9: Make `AutoInclusiveAncestorBlockElementsJoiner::Prepare()` consider whether it will NOT join the elements or not r=m_kato!
47 bytes, text/x-phabricator-request
Details | Review

https://searchfox.org/mozilla-central/rev/ac142717cc067d875e83e4b1316f004f6e063a46/editor/libeditor/HTMLEditSubActionHandler.cpp#4158-4182

I forgot to do this before bug 1658702. This needs to be fixed for making the computation code much simpler, but maybe risky change.

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

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

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

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

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

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

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

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

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4b99c491fa96
part 1: Make `AutoInclusiveAncestorBlockElementsJoiner::Prepare()` stop returning `EditActionResult` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/06680416a55d
part 2: Move `AutoTrackDOMPoint` for `AutoInclusiveAncestorBlockElementsJoiner::Run()` into the minimum scope r=m_kato
https://hg.mozilla.org/integration/autoland/rev/64b7ca1d75ac
part 3: Use early-return style when `AutoInclusiveAncestorBlockElementsJoiner::Prepare()` returns `false` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/37d3602c4ea7
part 4: Sort out "handled" state settings around `AutoInclusiveAncestorBlockElementsJoiner` users r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e07fe4e659a0
part 5: Make helper methods of `AutoInclusiveAncestorBlockElementsJoiner::Run()` use simple constructor of `EditActionResult` when they return error r=m_kato
https://hg.mozilla.org/integration/autoland/rev/5ec20189c83b
part 6: Create a method to check whether `HTMLEditor::MoveOneHardLineContents()` returns "handled" or "canceled" r=m_kato
https://hg.mozilla.org/integration/autoland/rev/bef8e416efdd
part 7: Make `AutoInclusiveAncestorBlockElementsJoiner::Prepare()` check the relation of its `mLeftBlockElement` and `mRightBlockElement` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/783abfedd69c
part 8: Make `AutoInclusiveAncestorBlockElementsJoiner::Prepare()` scan preceding invisible `<br>` element r=m_kato
https://hg.mozilla.org/integration/autoland/rev/92d8da06449b
part 9: Make `AutoInclusiveAncestorBlockElementsJoiner::Prepare()` consider whether it will NOT join the elements or not r=m_kato
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: