Page MenuHomePhabricator

Bug 1740416 - Make `HTMLEditor::HandleInsertParagraphInParagraph()` call `WhiteSpaceVisibilityKeeper::PrepareToSplitBlockElement()` before splitting a text node r=m_kato!,smaug!
ClosedPublic

Authored by masayuki on Nov 11 2021, 5:23 AM.
Referenced Files
Unknown Object (File)
Thu, Apr 9, 9:03 AM
Unknown Object (File)
Wed, Apr 8, 5:23 AM
Unknown Object (File)
Oct 13 2025, 4:24 AM
Unknown Object (File)
Oct 12 2025, 9:39 AM
Unknown Object (File)
May 13 2025, 9:47 AM
Unknown Object (File)
Apr 6 2025, 4:42 PM
Unknown Object (File)
Apr 3 2025, 1:43 AM
Unknown Object (File)
Mar 21 2025, 12:36 AM
Subscribers
None

Details

Summary

It does the following things when caret is collapsed in a text node in a <p>
or <div> element.

  1. Split the text node containing caret to insert <br> element
  2. Insert <br> element after it
  3. Split ancestor elements which inclusive descendants of the <p> or <div>
  4. Delete the <br> element if unnecessary from the left paragraph

#3 and #4 are performed by HTMLEditor::SplitParagraph() and it calls
WhiteSpaceVisibilityKeeper::PrepareToSplitBlockElement() correctly before
splitting the block. However, in the case (caret is at middle of a text node),
the text has already been split to 2 nodes because of #1. Therefore, it fails
to handle to keep the white-space visibility.

So that I believe that the root cause of this bug is, the method does much
complicated things which are required, and doing the redundant things will
eat memory space due to undo transactions. However, for now, I'd like to fix
this with a simple patch which just call the preparation method before splitting
the text node because I'd like to uplift this if it'd be approved (Note that
this is not a recent regression, the root cause was created by bug 92686 which
was fixed in 17 years ago:
https://searchfox.org/mozilla-central/commit/2e66280faef73e9be218e00758d4eb738395ac83,
but must be annoying bug for users who see this frequently).

The new WPTs are pass in Chrome.

Event Timeline

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.

@smaug I'd like you to review nsINode.cpp part. Adding info may be noisy for the other developers. So if you don't like some of them, let me know, I'll remove them.

I don't quite understand why the debugging code is included in this patch, but fine :)
(The test isn't for the nsINode part, so I didn't set testing tag.)

This revision is now accepted and ready to land.Nov 12 2021, 1:06 PM

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!

I don't quite understand why the debugging code is included in this patch, but fine :)
(The test isn't for the nsINode part, so I didn't set testing tag.)

Thanks. I had trouble when I debugging the cause of this bug with dumping the DOM nodes, and half of the change is obviously a bug fix (Text and Document do not appear in the dump), and it's not affect any user's behavior so that I kept the change as-is.

masayuki added a commit: Restricted Diffusion Commit.Feb 18 2022, 6:49 PM