1930565 - Refactor `AutoBlockElementsJoiner::HandleDeleteNonCollapsedRange` with implementing `DeleteRangeResult`

Status: RESOLVED FIXED

(Core :: DOM: Editor, task)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, delete handlers of HTMLEditor can return CaretPoint or EditActionResult. However, it's nicer to make some of the delete handlers return both data and actually deleted range.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1930565 - part 1: Make the line move methods of `WhiWhiteSpaceVisibilityKeeper` return `MoveNodeResult` r=m_kato!

MoveNodeResult is usually merged with an ignored result which store the first
insertion point and multiple handled results which store next insertion points.
Therefore, the final result can store the moved content nodes range. The range
is useful to clean up unnecessary line breaks and to insert padding line breaks.

Therefore, this patch makes MoveNodeResult compute the range in |= operator
and make the methods return the range of moved content.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1930565 - part 2: Implement `DeleteRangeResult` and make `AutoInclusiveAncestorBlockElementsJoiner::Run` return it r=m_kato!

AutoBlockElementsJoiner::HandleDeleteNonCollapsedRange may delete multiple
lines if selection ranges partially select table(s) because in such case, we
keep the table structure but delete the selected content. Therefore, it
requires clean up padding line breaks in multiple places. However, when it
wants to clean them up, there is no enough information about the deleted ranges.
Therefore, this patch makes the one of the callee return it.

Depends on D229857

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1930565 - part 3: Make `AutoBlockElementsJoiner::DeleteNodesEntirelyInRangeButKeepTableStructure` return `DeleteRangeResult` r=m_kato!

Then, AutoBlockElementsJoiner::HandleDeleteNonCollapsedRange can check
whether the deleted range is collapsed or not (if not collapsed, the handlers
kept a table structure or moved a line).

Depends on D229858

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1930565 - part 4: Rewrite `AutoBlockElementsJoiner::HandleDeleteNonCollapsedRange` r=m_kato!

Although we've not rewritten DeleteTextAtStartAndEndOfRange() with
DeleteRangeResult yet (it touches Selection), we should rewrite
HandleDeleteNonCollapsedRange before that for avoiding to make too big
patch.

This splits the while block to 2 lambdas which return DeleteRangeResult.
Then, the last part can be rewritten without dependency from the stateful
preparation for that. So, the new code should be easier to understand with
reading all of the method.

Depends on D229859

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1930565 - part 5: Make `AutoBlockElementsJoiner::DeleteTextAtStartAndEndOfRange` return `DeleteRangeResult` r=m_kato!

This patch makes DeleteTextAtStartAndEndOfRange stop touching Selection
and return deleted range and caret suggestion.

Unfortunately, stopping updating Selection from it causes failing 2
mochitests which press Delete from outside a child right block which does
not follow visible content in the ancestor left block. The reason is,
AutoMoveOneLineHandler::Run fails and returns error. Then,
AutoInclusiveAncestorBlockElementsJoiner::Run returns error and
AutoBlockElementsJoiner::HandleDeleteNonCollapsedRange stops handling it.
I.e., AutoBlockElementsJoiner::HandleDeleteNonCollapsedRange does not updage
Selection. Finally, the post processor,
HTMLEditor::OnEndHandlingTopLevelEditSubActionInternal will put a padding
<br> to start of the deleted range. In this case, it's at the caret position
at typing Delete key.

This issue will be fixed by the next patch.

Depends on D229860

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1930565 - part 6: Make `AutoMoveOneLineHandler::Prepare` extends the line outside the topmost ancestor block of the right block in its ancestor left block r=m_kato!

It extends a range to wrap a line whose contents moved to left block with
AutoRangeArray::ExtendRangesToWrapLines. However, if the line boundaries
are immediately after/before current block boundaries, it extends the range to
outside the block. However, when joining a right block and its ancestor left
block, the range should not wrap the right block itself because we won't move
the right block instead we should unwrap the right block.

The new behavior is different from Chrome's behavior. Chrome's joining blocks
rules are really different form ours. If the range starts from immediately
after a block boundary, Chrome does not unwrap the block, they just delete
first character/content. However, if there is a visible content before the
start boundary in the block, they unwrap the right block and join the content.

Now we are handling padding line breaks in the post processor,
HTMLEditor::OnEndHandlingTopLevelEditSubActionInternal, it's hard to align
the behavior only with this patch. Therefore, this patch changes the
expectations of new failure tests.

Depends on D229861

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e305846833ba
part 1: Make the line move methods of `WhiWhiteSpaceVisibilityKeeper` return `MoveNodeResult` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/d299319b2c18
part 2: Implement `DeleteRangeResult` and make `AutoInclusiveAncestorBlockElementsJoiner::Run` return it r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4759f7ea35b0
part 3: Make `AutoBlockElementsJoiner::DeleteNodesEntirelyInRangeButKeepTableStructure` return `DeleteRangeResult` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/fe1541a00d9c
part 4: Rewrite `AutoBlockElementsJoiner::HandleDeleteNonCollapsedRange` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/c713d46b543e
part 5: Make `AutoBlockElementsJoiner::DeleteTextAtStartAndEndOfRange` return `DeleteRangeResult` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/9af1bfafb95e
part 6: Make `AutoMoveOneLineHandler::Prepare` extends the line outside the topmost ancestor block of the right block in its ancestor left block r=m_kato

Comment 8

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/e305846833ba
https://hg.mozilla.org/mozilla-central/rev/d299319b2c18
https://hg.mozilla.org/mozilla-central/rev/4759f7ea35b0
https://hg.mozilla.org/mozilla-central/rev/fe1541a00d9c
https://hg.mozilla.org/mozilla-central/rev/c713d46b543e
https://hg.mozilla.org/mozilla-central/rev/9af1bfafb95e