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 after <pre> and <textarea> #7280

Merged
merged 13 commits into from Apr 14, 2022

Conversation

ota-meshi
Copy link
Member

@ota-meshi ota-meshi commented Feb 17, 2022

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

Close #7264

This PR changes to strip the newline immediately after <pre>.
However, if innerHTML is used in the renderer and there are multiple newlines, the newline immediately after <pre> is restored.

However, the SSR remains the same as before. This isn't necessary because HTML parsing strips the newline immediately after <pre>, and it's simpler than handling multiple newlines.
It has been changed to handle the same in SSR.

@ota-meshi
Copy link
Member Author

I don't know how to fix the CI timeout. If you think my change is the cause, please let me know.

@ota-meshi ota-meshi changed the title [fix]: strip leading newline after <pre> [fix]: strip leading newline after <pre> and <textarea> Apr 8, 2022
@ota-meshi
Copy link
Member Author

@tanhauhau Thank you for checking this PR!

I made a fix for <textarea>.
However, since <textarea> has content in the value attribute, I added a different logic than <pre>.
Also, I changed it to use the content instead of the value attribute when using innerHTML.
Also, SSR will always include the newline after <textarea>. It is possible to reduce newlines by checking for the presence of the first newline, but I thought that the logic would be complicated, so I changed it to always put a newline.

@ota-meshi
Copy link
Member Author

Also, I changed it to use the content instead of the value attribute when using innerHTML.

Currently, the following JS is generated.

<div>
	<textarea>AAA</textarea>
</div>
div.innerHTML = `<textarea value="AAA"></textarea>`;

I don't think this should work, but it works fine with REPL. (I didn't understand why it worked.)

https://svelte.dev/repl/d5ccce1d50d547039053b78db9ce8477?version=3.46.6

However, it didn't work in the test, so I changed it to render to the content.

div.innerHTML = `<textarea>AAA</textarea>`;

@tanhauhau
Copy link
Member

I don't think this should work, but it works fine with REPL. (I didn't understand why it worked.)

oh the result u see in REPL uses dev mode to build, in which it does not have the .innerHTML optimisation.

@ota-meshi
Copy link
Member Author

oh the result u see in REPL uses dev mode to build, in which it does not have the .innerHTML optimisation.

Ah I see. Thank you for letting me know.
Should I open the PR as another bug?
(But, I don't think anyone actually uses fixed strings in <textarea>.)

@tanhauhau
Copy link
Member

Should I open the PR as another bug?

i see you already made relevant changes right, it's okay i'll review them later

@ota-meshi
Copy link
Member Author

Thank you for checking this PR.
I changed this PR. So please check again.

The CI error seems to be due to a timeout. If you think my change is the cause, please let me know.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! I enhanced the comments a little to explain in more detail the quirkyness that the leading newline is only ignored if innerHTML is used on a parent of the tag, not on the tag content, and not if set through other means.

@dummdidumm dummdidumm merged commit 3a238fe into sveltejs:master Apr 14, 2022
@ota-meshi ota-meshi deleted the pre2 branch April 14, 2022 20:58
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)
3 participants