Skip to content

Conversation

jvithlani
Copy link
Contributor

@jvithlani jvithlani commented Jun 3, 2025

Description

Context:
Currently the Lexical reconciler adds an extra br tag in the DOM to ensure the br tag added by us is not ignored by the parent block element.
So if there are n consecutive LB nodes in a block node, there will be n+1 br elements in the DOM. This hold true only if the last element of the block node is LB node.
This is handled by reconcileElementTerminatingLineBreak helper present in packages/lexical/src/LexicalReconciler.ts

In order to handle this, the importDOM method of the lineBreak node, ignores the last br node present in the imported HTML to avoid adding extra line breaks.

Issue:
The export DOM method does not export the extra br tag, which is anticipated to be present in the output by the import method, hence if we have 3 LB nodes added, there will be 4 br elements in the DOM. but the exported output will only have 3 LB nodes.
If we try to parse the same HTML output in our editor, we will trim away the last LB node, leading to only 2 line breaks in the given output which leads to a discrepancy.

I logged the HTML output in our playground, attaching screenshots below
Before:
Here there are 3 nodes inserted in the video, 4 BR elements in the DOM but only 3 br elements are exported

Screen.Recording.2025-06-03.at.5.21.19.PM.mov

After:
Here there are 2 nodes inserted in the video, 3 BR elements in the DOM but 3 br elements are exported

Screenshot 2025-06-03 at 4 44 00 PM
Screen.Recording.2025-06-03.at.4.46.30.PM.mov

Closes #

Test plan

Before

Insert relevant screenshots/recordings/automated-tests

After

Insert relevant screenshots/recordings/automated-tests

Copy link

vercel bot commented Jun 3, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 3, 2025 0:03am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 3, 2025 0:03am

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 3, 2025
Copy link
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the right way around, IIRC the extra <br> is there for input reasons and not rendering reasons so it would probably make more sense to remove this quirk from importDOM rather than add a quirk to exportDOM. We could add a data-lexical-linebreak="true" attribute to these managed line breaks in the reconciler to make them easy to ignore if their rendered output is piped back in to importDOM.

Either way if we are changing behavior here we should make sure to add test coverage for it.

@jvithlani
Copy link
Contributor Author

@etrepum Sure, will add the tests once we finalise the approach
About removing the quirk from importDOM i visited the PR in which this was added #6395 it was done to make it consistent with how HTML reacts to the br tags. This is also helpful to main the WYSIWYG editor, so i felt this is the right thing to do, export the HTML which will render corectly even if rendered outside lexical editor, let me know your thoughts im open to both.

@etrepum
Copy link
Collaborator

etrepum commented Jun 4, 2025

/cc @potatowagon

@jvithlani
Copy link
Contributor Author

@potatowagon bumping on this

@jvithlani
Copy link
Contributor Author

@etrepum what can we do here next?

@etrepum
Copy link
Collaborator

etrepum commented Jun 10, 2025

We can wait and see if @potatowagon responds or proceed with the suggestion I had to add an attribute to managed line breaks for this purpose and add test coverage for the new behavior

@potatowagon
Copy link
Contributor

potatowagon commented Jun 11, 2025

add a data-lexical-linebreak="true" attribute to these managed line breaks in the reconciler

im not too clear what this approach would look like, is it a heavy lift?

@etrepum
Copy link
Collaborator

etrepum commented Jun 11, 2025

That was my suggestion, so that it can be determined whether the linebreak is part of the document or a rendering artifact that should be excluded on import. The PR as it stands tries to export the extra linebreak, I guess as a workaround for what #6395 does

@etrepum
Copy link
Collaborator

etrepum commented Jun 11, 2025

It should not be a heavy lift to set an attribute on managed linebreaks. The "hardest" part will be to fix the tests that are sensitive to that change in the rendered HTML, but that should be very mechanical.

@jvithlani
Copy link
Contributor Author

If we just wanna ignore the imported extra BR, we can rather remove that from the importDOM method instead of exporting and only ignoring that bit. As far as i understood this importDOM is written to handle even non-lexical generated HTML, so rather than just ignoring the br tag with our custom data-attribute we can take away that importing logic

@jvithlani
Copy link
Contributor Author

@etrepum thoughts?

@etrepum
Copy link
Collaborator

etrepum commented Jun 17, 2025

I'm waiting to hear @potatowagon's thoughts because this PR is relevant to their #6395

@jvithlani
Copy link
Contributor Author

@potatowagon bumping on this, its open for a long time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants