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]: strip leading newline characters after 'pre' elements #7269

Closed
wants to merge 1 commit into from

Conversation

Tapped
Copy link

@Tapped Tapped commented Feb 15, 2022

Close #7264

Strips the leading newline character after 'pre' elements per the spec https://www.w3.org/TR/2011/WD-html5-20110113/grouping-content.html#the-pre-element

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

// A leading newline character immediately following the pre element start tag is stripped
// See spec: https://www.w3.org/TR/2011/WD-html5-20110113/grouping-content.html#the-pre-element
if (!is_closing_tag && name === 'pre') {
if (!parser.eat('\r\n')) {
Copy link
Author

Choose a reason for hiding this comment

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

I'm unsure if carriage return is already stripped from the source before feed into the parser?

Couldn't find any references to CRLF or '\r' in the parser, but I don't know.

@Tapped
Copy link
Author

Tapped commented Feb 16, 2022

This change will cause issues for preserveWhitespace: true, as the leading newline after the pre tag is now not part of the HTML model (directly at least). We might want to consider refactoring how preserveWhitespace is done, as I think it's better if the parser eats leading and trailing whitespaces in contexts where it's allowed to, and stores the whitespace as a separate state on the HTML element, which can later in the HTML writer be used to preserve whitespaces. That will also likely remove almost identical whitespace handlers (trimmers) in sre_render and dom_render. Also I would expect preserveWhitespace to only be an option used in sre_render but seems to also be used in the dom_render for some reason.

@Tapped Tapped closed this Feb 16, 2022
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.

Handle <pre>whitespace formatting edge case (no new line)
1 participant