1899083 - Make `HTMLEditor::AutoInsertParagraphHandler::HandleInParagraph` stop inserting maybe unnecessary `<br>` element before splitting the paragraph

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

The method inserts <br> element if there is no <br> element around the splitting point. Then, the <br> element is passed to SplitParagraphWithTransaction. Finally, it's removed when it's finally visible.

Comment 1

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

So, it can be checked before inserting a <br> with whether the <br> is:

• not followed by a block boundary
• and, not follows a collapsible white-space

Comment 2

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

Attachment: Bug 1899083 - part 1: Make the legacy `insertParagraph` handling cases outside `HandleInParagraph` r=m_kato!

In the IE/Netscape era, we inserted a <br> in specific cases even if
insertParagraph command is being handled. Although this patch makes
the legacy mode slower because of increasing the number of <br>
element scanning, we should split it off to make HandleInParagraph
simpler to optimize its logic later.

Comment 3

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

Attachment: Bug 1899083 - part 2: Make `AutoInsertParagraphHandler::GetBetterSplitPointToAvoidToContinueLink` work better r=m_kato!

The method does not check invisible white-spaces nor Comment nodes.
Therefore, it may work differently from the user's point of view in
some edge cases.

Additionally, for making the following patch simpler and safer, the
split point should be outside a non-splittable element like <img>.

Comment 4

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

Attachment: Bug 1899083 - part 3: Rewrite handling `insertParagraph` handler when `Selection` is in a normal paragraph r=m_kato!

Unfortunately, I don't have any ideas to separate this change because
the current behavior is broken, it assumes only simple cases. However,
with rewriting the code simpler, we'll get the unexpected passes.
Therefore, this patch has a big change.

The strategy of the new behavior is, first, put a padding <br> to next
to the split point if and only if it's immediately before a block
boundary and delete unnecessary existing invisible <br> element if
it puts a new <br>. Next, put a <br> to end of the left paragraph
if and only if the paragraph ends with a block boundary or a line break
or the split occurred at end of the deepest inline element.

Comment 5

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

Attachment: Bug 1899083 - part 4: Make `AutoInsertParagraphHandler::HandleInListItemElement` use `AutoInsertParagraphHandler::SplitParagraphWithTransaction` r=m_kato!

SplitParagraphWithTransaction() does a lot of additional works to
split a paragraph. This should be done when insertParagraph is
performed in an list item.

Then, we can drop the following code in HandleInListItemElement():

• Inserting a padding <br> into empty left list item
• Copying styles at end of the left <li> to the right empty one
• Considering the caret position with scanning the right list item

Comment 6

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

Attachment: Bug 1899083 - part 5: Make `AutoInsertParagraphHandler::HandleInListItemElement` stop splitting `<dt>`/`<dd>` if splitting at end r=m_kato!

If the right <dt>/<dd> is empty, the method replaces it with
opposite type element without copying the style. So, we can consider
whether we can just insert an opposite list item before splitting the
given list item.

Note that new failures were passed by a bug of current code. That is,
the method treats invisible <br> as visible. So, the cases cause
different result whether there is an invisible <br> at end of the
list item even though it does not cause different look for the users.

Inserting opposite type of list item is a Gecko specific behavior.
I think we should stop doing that in Firefox for the compatibility with
the other browsers in a follow up bug.

Comment 7

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

Attachment: Bug 1899083 - part 6: Rewrite `AutoInsertParagraphHandler::HandleInHeadingElement` r=m_kato!

This make AutoInsertParagraphHandler::HandleInHeadingElement:

• stop splitting the heading element if split at end
• use SplitParagraphWithTransaction if splitting the heading element

Then, this also cases the failures because not touching the invisible
<br> element in the heading element. Failing with this reason does
not make sense because not touching the heading element saves the
footprint for undo transactions. Therefore, this patch makes the tests
allow the <br>s to stay there.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/9495e7aaacfa
https://hg.mozilla.org/integration/autoland/rev/401f1f9d1b45
part 1: Make the legacy `insertParagraph` handling cases outside `HandleInParagraph` r=m_kato

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/fba329f07ef5
https://hg.mozilla.org/integration/autoland/rev/b0887987f495
part 2: Make `AutoInsertParagraphHandler::GetBetterSplitPointToAvoidToContinueLink` work better r=m_kato

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/81bea21d35e3
https://hg.mozilla.org/integration/autoland/rev/cf0d99adfd16
part 3: Rewrite handling `insertParagraph` handler when `Selection` is in a normal paragraph r=m_kato

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/420068615f21
https://hg.mozilla.org/integration/autoland/rev/5c38af816f1d
part 4: Make `AutoInsertParagraphHandler::HandleInListItemElement` use `AutoInsertParagraphHandler::SplitParagraphWithTransaction` r=m_kato

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7b601baf2a9d
https://hg.mozilla.org/integration/autoland/rev/8d66b04b16dd
part 5: Make `AutoInsertParagraphHandler::HandleInListItemElement` stop splitting `<dt>`/`<dd>` if splitting at end r=m_kato
https://github.com/mozilla-firefox/firefox/commit/48f67c673f2b
https://hg.mozilla.org/integration/autoland/rev/8f561bffb2ac
part 6: Rewrite `AutoInsertParagraphHandler::HandleInHeadingElement` r=m_kato

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/401f1f9d1b45
https://hg.mozilla.org/mozilla-central/rev/b0887987f495
https://hg.mozilla.org/mozilla-central/rev/cf0d99adfd16
https://hg.mozilla.org/mozilla-central/rev/5c38af816f1d
https://hg.mozilla.org/mozilla-central/rev/8d66b04b16dd
https://hg.mozilla.org/mozilla-central/rev/8f561bffb2ac

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/53890 for changes under testing/web-platform/tests

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot