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] whitespace html entities lost in component slot #8464

Merged

Conversation

xxkl1
Copy link
Contributor

@xxkl1 xxkl1 commented Apr 8, 2023

closes: #8359

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

The reason is due to the following code

function not_whitespace_text(node) {
return !(node.type === 'Text' && regex_only_whitespaces.test(node.data));
}

export const regex_only_whitespaces = /^\s+$/;

  is non-breaking space characte, /\s/ match it and judging that it should not be retained.

&nbsp; in <span>&nbsp;</span> use [ \t\r\n] so it will not be delete.

<Component>
	<span>&nbsp;</span>
</Component>

if (should_trim && !child.keep_space()) {
data = trim_end(data);
if (!data) continue;

export function trim_end(str: string) {
return str.replace(regex_ends_with_whitespaces, '');
}

export const regex_ends_with_whitespaces = /[ \t\r\n]*$/;

/\s/ is looks equal with /[ \u00A0\u2009\u200C\u200D\u3000\t\n\r\f]/, \u00A0\u2009\u200C\u200D\u3000 is unicode code of \&nbsp; \&thinsp \&zwnj; \&zwj; &emsp;.

I think we can use [ \t\n\r\f] equals with [\u0020\t\n\r\f] to solve this problem, \u0020 is standard space character we need skip.

The test error of html space entities will be hided by /\s/ in normalizeHtml. The reason is html space entities be replace to '', so /\s/ need to replaced by [ \t\n\r\f] and save html space entities in test too.

svelte/test/helpers.ts

Lines 143 to 155 in d42ca04

export function normalizeHtml(window, html, preserveComments = false) {
try {
const node = window.document.createElement('div');
node.innerHTML = html
.replace(/(<!--.*?-->)/g, preserveComments ? '$1' : '')
.replace(/>[\s\r\n]+</g, '><')
.trim();
cleanChildren(node);
return node.innerHTML.replace(/<\/?noscript\/?>/g, '');
} catch (err) {
throw new Error(`Failed to normalize HTML:\n${html}`);
}
}

@vercel
Copy link

vercel bot commented Apr 8, 2023

@xxkl1 is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@xxkl1 xxkl1 force-pushed the fix/html-entities-inside-component-slot branch from 8b14f49 to 71aa09f Compare April 9, 2023 13:32
@dummdidumm dummdidumm added this to the 4.x milestone Apr 11, 2023
@dummdidumm dummdidumm changed the base branch from master to version-4 April 11, 2023 13:20
@dummdidumm dummdidumm merged commit 765023d into sveltejs:version-4 Apr 11, 2023
25 checks passed
@dummdidumm
Copy link
Member

Thank you!

dummdidumm pushed a commit that referenced this pull request Apr 18, 2023
@gtm-nayan gtm-nayan mentioned this pull request Jun 17, 2023
5 tasks
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.

If slot content consists only of &nbsp, it is not passed to component
2 participants