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
9 changes: 9 additions & 0 deletions src/compiler/compile/nodes/Text.ts
Expand Up @@ -49,6 +49,15 @@ export default class Text extends Node {
return this.within_pre();
}

/**
* @returns If true, the leading newline character should be stripped.
* @see https://html.spec.whatwg.org/multipage/grouping-content.html#the-pre-element
*/
should_strip_leading_newline(): boolean {
const parent = this.parent;
return parent.type === 'Element' && parent.name === 'pre' && parent.children[0] === this;
}
benmccann marked this conversation as resolved.
Show resolved Hide resolved

within_pre(): boolean {
let node = this.parent;
while (node) {
Expand Down
13 changes: 11 additions & 2 deletions src/compiler/compile/render_dom/wrappers/Element/index.ts
Expand Up @@ -12,7 +12,7 @@ import { namespaces } from '../../../../utils/namespaces';
import AttributeWrapper from './Attribute';
import StyleAttributeWrapper from './StyleAttribute';
import SpreadAttributeWrapper from './SpreadAttribute';
import { dimensions } from '../../../../utils/patterns';
import { dimensions, start_newline } from '../../../../utils/patterns';
import Binding from './Binding';
import add_to_set from '../../../utils/add_to_set';
import { add_event_handler } from '../shared/add_event_handlers';
Expand Down Expand Up @@ -939,7 +939,7 @@ export default class ElementWrapper extends Wrapper {
if (should_cache) {
block.chunks.update.push(b`
if (${block.renderer.dirty(dependencies)} && (${cached_snippet} !== (${cached_snippet} = ${snippet}))) {
${updater}
${updater}
}
`);
} else {
Expand Down Expand Up @@ -1015,6 +1015,15 @@ function to_html(wrappers: Array<ElementWrapper | TextWrapper | MustacheTagWrapp
if (!wrapper.void) {
state.quasi.value.raw += '>';

if (wrapper.node.name === 'pre') {
// Two or more leading newlines are required to restore the leading newline immediately after `<pre>`.
// see https://html.spec.whatwg.org/multipage/grouping-content.html#the-pre-element
tanhauhau marked this conversation as resolved.
Show resolved Hide resolved
const first = wrapper.fragment.nodes[0];
if (first && first.node.type === 'Text' && start_newline.test(first.node.data)) {
state.quasi.value.raw += '\n';
}
}

to_html(wrapper.fragment.nodes as Array<ElementWrapper | TextWrapper>, block, literal, state);

state.quasi.value.raw += `</${wrapper.node.name}>`;
Expand Down
9 changes: 7 additions & 2 deletions src/compiler/compile/render_dom/wrappers/Fragment.ts
Expand Up @@ -21,6 +21,7 @@ import Block from '../Block';
import { trim_start, trim_end } from '../../../utils/trim';
import { link } from '../../../utils/link';
import { Identifier } from 'estree';
import { start_newline } from '../../../utils/patterns';

const wrappers = {
AwaitBlock,
Expand Down Expand Up @@ -127,8 +128,12 @@ export default class FragmentWrapper {
if (strip_whitespace) {
const first = this.nodes[0] as Text;

if (first && first.node.type === 'Text' && !first.node.keep_space()) {
first.data = trim_start(first.data);
if (first && first.node.type === 'Text') {
if (!first.node.keep_space()) {
first.data = trim_start(first.data);
} else if (first.node.should_strip_leading_newline()) {
first.data = first.data.replace(start_newline, '');
}
benmccann marked this conversation as resolved.
Show resolved Hide resolved
if (!first.data) {
first.var = null;
this.nodes.shift();
Expand Down
1 change: 1 addition & 0 deletions src/compiler/utils/patterns.ts
@@ -1,5 +1,6 @@
export const whitespace = /[ \t\r\n]/;
export const start_whitespace = /^[ \t\r\n]*/;
export const end_whitespace = /[ \t\r\n]*$/;
export const start_newline = /^\r?\n/;

export const dimensions = /^(?:offset|client)(?:Width|Height)$/;
2 changes: 2 additions & 0 deletions test/runtime/samples/pre-tag/.editorconfig
@@ -0,0 +1,2 @@
[main.svelte]
trim_trailing_whitespace = unset
tanhauhau marked this conversation as resolved.
Show resolved Hide resolved
26 changes: 12 additions & 14 deletions test/runtime/samples/pre-tag/_config.js
Expand Up @@ -6,23 +6,14 @@ export default {
const elementDiv = target.querySelector('#div');
// Test for <pre> tag in non <pre> tag
const elementDivWithPre = target.querySelector('#div-with-pre');

// There is a slight difference in innerHTML because there is a difference in HTML optimization (in jsdom)
// depending on how the innerHTML is set.
// (There is no difference in the display.)
// Reassign innerHTML to add the same optimizations to innerHTML.

// eslint-disable-next-line no-self-assign
elementPre.innerHTML = elementPre.innerHTML;
// eslint-disable-next-line no-self-assign
elementDiv.innerHTML = elementDiv.innerHTML;
// eslint-disable-next-line no-self-assign
elementDivWithPre.innerHTML = elementDivWithPre.innerHTML;
// Test for <pre> tag with leading newline
const elementPreWithLeadingNewline = target.querySelector('#pre-with-leading-newline');
const elementPreWithoutLeadingNewline = target.querySelector('#pre-without-leading-newline');
const elementPreWithMultipleLeadingNewline = target.querySelector('#pre-with-multiple-leading-newlines');

assert.equal(
elementPre.innerHTML,
`
A
` A
B
<span>
C
Expand Down Expand Up @@ -53,5 +44,12 @@ export default {
F
</pre>`
);
assert.equal(elementPreWithLeadingNewline.children[0].innerHTML, 'leading newline');
assert.equal(elementPreWithLeadingNewline.children[1].innerHTML, ' leading newline and spaces');
assert.equal(elementPreWithLeadingNewline.children[2].innerHTML, '\nleading newlines');
assert.equal(elementPreWithoutLeadingNewline.children[0].innerHTML, 'without spaces');
assert.equal(elementPreWithoutLeadingNewline.children[1].innerHTML, ' with spaces ');
assert.equal(elementPreWithoutLeadingNewline.children[2].innerHTML, ' \nnewline after leading space');
assert.equal(elementPreWithMultipleLeadingNewline.innerHTML, '\n\nmultiple leading newlines');
}
};
22 changes: 22 additions & 0 deletions test/runtime/samples/pre-tag/main.svelte
Expand Up @@ -32,3 +32,25 @@
F
</pre>
</div>

<div id="pre-with-leading-newline">
<pre>
leading newline</pre>
<pre>
leading newline and spaces</pre>
<pre>

leading newlines</pre>
</div>

<div id="pre-without-leading-newline">
<pre>without spaces</pre>
<pre> with spaces </pre>
<pre>
newline after leading space</pre>
</div>

<pre id="pre-with-multiple-leading-newlines">


multiple leading newlines</pre>
15 changes: 1 addition & 14 deletions test/runtime/samples/preserve-whitespaces/_config.js
Expand Up @@ -10,22 +10,9 @@ export default {
// Test for <pre> tag in non <pre> tag
const elementDivWithPre = target.querySelector('#div-with-pre');

// There is a slight difference in innerHTML because there is a difference in HTML optimization (in jsdom)
// depending on how the innerHTML is set.
// (There is no difference in the display.)
// Reassign innerHTML to add the same optimizations to innerHTML.

// eslint-disable-next-line no-self-assign
elementPre.innerHTML = elementPre.innerHTML;
// eslint-disable-next-line no-self-assign
elementDiv.innerHTML = elementDiv.innerHTML;
// eslint-disable-next-line no-self-assign
elementDivWithPre.innerHTML = elementDivWithPre.innerHTML;

assert.equal(
elementPre.innerHTML,
`
A
` A
B
<span>
C
Expand Down
2 changes: 2 additions & 0 deletions test/server-side-rendering/samples/pre-tag/.editorconfig
@@ -0,0 +1,2 @@
[{main.svelte,_expected.html}]
trim_trailing_whitespace = unset
tanhauhau marked this conversation as resolved.
Show resolved Hide resolved
13 changes: 13 additions & 0 deletions test/server-side-rendering/samples/pre-tag/_expected.html
Expand Up @@ -28,3 +28,16 @@
E
F
</pre></div>

<div id="pre-with-leading-newline"><pre>
leading newline</pre>
<pre>
leading newline and spaces</pre>
<pre>

leading newlines</pre></div>

<div id="pre-without-leading-newline"><pre>without spaces</pre>
<pre> with spaces </pre>
<pre>
newline after leading space</pre></div>
17 changes: 17 additions & 0 deletions test/server-side-rendering/samples/pre-tag/main.svelte
Expand Up @@ -32,3 +32,20 @@
F
</pre>
</div>

<div id="pre-with-leading-newline">
<pre>
leading newline</pre>
<pre>
leading newline and spaces</pre>
<pre>

leading newlines</pre>
</div>

<div id="pre-without-leading-newline">
<pre>without spaces</pre>
<pre> with spaces </pre>
<pre>
newline after leading space</pre>
</div>