Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue with Markdown rendering after line break in strict mode #889

Conversation

savetheclocktower
Copy link
Contributor

Identify the Bug

I noticed this text in the core settings:

Screenshot 2024-01-22 at 11 51 13 AM

But the text is correct in the config schema, so what's going on?

I'll distill it for you:

atom.ui.markdown.render(`profile.<br>Changing`);
//-> "<p>profile.<br>Changing</p>\n"

atom.ui.markdown.render(`profile.<br>Changing`, { disableMode: 'strict' });
//-> "<p>profile.<br>hanging</p>\n"

Description of the Change

I don't know enough about why this callback is in place, but I assume the reasoning is sound. Nonetheless, we're advancing a fixed number of characters in the parser when consuming it, even though the matched line break can be of variable width (<br>, <br />, etc.).

Alternate Designs

I did not think about this for long enough to consider alternate designs.

Possible Drawbacks

I might have misinterpreted this code, though it certainly does seem to fix the problem.

I also took the liberty of changing the token content from <br/> to <br>, since the former is invalid HTML (you'd need a space before the closing slash) and we're way past the XHTML days. But as far as I can tell, this makes no difference to the parser output anyway.

Verification Process

Run the example code from above in the console. It should do the wrong thing on master and the right thing on this PR branch. Also, the setting in question (in Core) should render correctly.

Release Notes

  • Fixed a rendering error in atom.ui.markdown.render when disableMode was set to "strict" and the input contained HTML line breaks.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Easy approval from me, as this seems like a proper way to find the increase to the state position.

But just for completion, I've confirmed the regex used here still will match for all variations of <br> / <br /> / <br/> just to make sure we are matching in every case.

Also took a look at where this originally came from and why it's broken now.

But it seems that originally this logic was placed into some callback within marked and the switch I did from marked to markdown-it seems to have broken it.

I was also curious about where this originally came from, looks like (from a quick look) that atom/settings-view#1161 was trying to get the markdown parsing to match what GitHub is doing.

@confused-Techie confused-Techie merged commit d34af8f into pulsar-edit:master Jan 26, 2024
101 of 102 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants