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
8 changes: 7 additions & 1 deletion src/compiler/compile/nodes/Element.ts
Expand Up @@ -11,7 +11,7 @@ import StyleDirective from './StyleDirective';
import Text from './Text';
import { namespaces } from '../../utils/namespaces';
import map_children from './shared/map_children';
import { dimensions } from '../../utils/patterns';
import { dimensions, start_newline } from '../../utils/patterns';
import fuzzymatch from '../../utils/fuzzymatch';
import list from '../../utils/list';
import Let from './Let';
Expand Down Expand Up @@ -208,6 +208,12 @@ export default class Element extends Node {

// this is an egregious hack, but it's the easiest way to get <textarea>
// children treated the same way as a value attribute
const first = info.children[0];
if (first && first.type === 'Text') {
// The leading newline character should be stripped.
// see https://html.spec.whatwg.org/multipage/syntax.html#element-restrictions
first.data = first.data.replace(start_newline, '');
}
info.attributes.push({
type: 'Attribute',
name: 'value',
Expand Down
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
45 changes: 43 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 @@ -992,7 +992,13 @@ function to_html(wrappers: Array<ElementWrapper | TextWrapper | MustacheTagWrapp
// element
state.quasi.value.raw += `<${wrapper.node.name}`;

const is_empty_textarea = wrapper.node.name === 'textarea' && wrapper.fragment.nodes.length === 0;

(wrapper as ElementWrapper).attributes.forEach((attr: AttributeWrapper) => {
if (is_empty_textarea && attr.node.name === 'value') {
// The value attribute of <textarea> renders as content.
return;
}
state.quasi.value.raw += ` ${fix_attribute_casing(attr.node.name)}="`;

attr.node.chunks.forEach(chunk => {
Expand All @@ -1015,6 +1021,41 @@ 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';
}
}

if (is_empty_textarea) {
// The <textarea> renders the value attribute as content because the content is stored in the value attribute.
const value_attribute = wrapper.attributes.find(attr => attr.node.name === 'value');
if (value_attribute) {
// Two or more leading newlines are required to restore the leading newline immediately after `<textarea>`.
// see https://html.spec.whatwg.org/multipage/syntax.html#element-restrictions
const first = value_attribute.node.chunks[0];
if (first && first.type === 'Text' && start_newline.test(first.data)) {
state.quasi.value.raw += '\n';
}
value_attribute.node.chunks.forEach(chunk => {
if (chunk.type === 'Text') {
state.quasi.value.raw += escape_html(chunk.data);
} else {
literal.quasis.push(state.quasi);
literal.expressions.push(chunk.manipulate(block));

state.quasi = {
type: 'TemplateElement',
value: { raw: '' }
};
}
});
}
tanhauhau marked this conversation as resolved.
Show resolved Hide resolved
}

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
7 changes: 6 additions & 1 deletion src/compiler/compile/render_ssr/handlers/Element.ts
Expand Up @@ -40,7 +40,7 @@ export default function(node: Element, renderer: Renderer, options: RenderOption
const { name, expression: { node: expression } } = style_directive;
return p`"${name}": ${expression}`;
});

const style_expression =
style_expression_list.length > 0 &&
x`{ ${style_expression_list} }`;
Expand Down Expand Up @@ -164,6 +164,11 @@ export default function(node: Element, renderer: Renderer, options: RenderOption

renderer.add_expression(x`($$value => $$value === void 0 ? ${result} : $$value)(${node_contents})`);
} else {
if (node.name === 'textarea') {
// Restore the leading newline immediately after `<textarea>`.
// see https://html.spec.whatwg.org/multipage/syntax.html#element-restrictions
renderer.add_string('\n');
benmccann marked this conversation as resolved.
Show resolved Hide resolved
}
renderer.add_expression(node_contents);
}

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
4 changes: 2 additions & 2 deletions test/runtime/samples/textarea-children/_config.js
Expand Up @@ -9,9 +9,9 @@ export default {

test({ assert, component, target }) {
const textarea = target.querySelector( 'textarea' );
assert.strictEqual( textarea.value, '\n\t<p>not actually an element. 42</p>\n' );
assert.strictEqual( textarea.value, '\t<p>not actually an element. 42</p>\n' );

component.foo = 43;
assert.strictEqual( textarea.value, '\n\t<p>not actually an element. 43</p>\n' );
assert.strictEqual( textarea.value, '\t<p>not actually an element. 43</p>\n' );
}
};
2 changes: 2 additions & 0 deletions test/runtime/samples/textarea-content/.editorconfig
@@ -0,0 +1,2 @@
[main.svelte]
trim_trailing_whitespace = unset
34 changes: 34 additions & 0 deletions test/runtime/samples/textarea-content/_config.js
@@ -0,0 +1,34 @@
export default {
test({ assert, target }) {
// Test for <textarea> tag
const elementTextarea = target.querySelector('#textarea');
// Test for <textarea> tag in non <textarea> tag
const elementDivWithTextarea = target.querySelector('#div-with-textarea');
// Test for <textarea> tag with leading newline
const elementTextareaWithLeadingNewline = target.querySelector('#textarea-with-leading-newline');
const elementTextareaWithoutLeadingNewline = target.querySelector('#textarea-without-leading-newline');
const elementTextareaWithMultipleLeadingNewline = target.querySelector('#textarea-with-multiple-leading-newlines');
const elementDivWithTextareaWithMultipleLeadingNewline = target.querySelector('#div-with-textarea-with-multiple-leading-newlines');

assert.equal(
elementTextarea.value,
` A
B
`
);
assert.equal(
elementDivWithTextarea.children[0].value,
` A
B
`
);
assert.equal(elementTextareaWithLeadingNewline.children[0].value, 'leading newline');
assert.equal(elementTextareaWithLeadingNewline.children[1].value, ' leading newline and spaces');
assert.equal(elementTextareaWithLeadingNewline.children[2].value, '\nleading newlines');
assert.equal(elementTextareaWithoutLeadingNewline.children[0].value, 'without spaces');
assert.equal(elementTextareaWithoutLeadingNewline.children[1].value, ' with spaces ');
assert.equal(elementTextareaWithoutLeadingNewline.children[2].value, ' \nnewline after leading space');
assert.equal(elementTextareaWithMultipleLeadingNewline.value, '\n\nmultiple leading newlines');
assert.equal(elementDivWithTextareaWithMultipleLeadingNewline.children[0].value, '\n\nmultiple leading newlines');
}
};
40 changes: 40 additions & 0 deletions test/runtime/samples/textarea-content/main.svelte
@@ -0,0 +1,40 @@
<textarea id="textarea">
A
B
</textarea>

<div id="div-with-textarea">
<textarea>
A
B
</textarea>
</div>

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

leading newlines</textarea>
</div>

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

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


multiple leading newlines</textarea>

<div id="div-with-textarea-with-multiple-leading-newlines">
<textarea>


multiple leading newlines</textarea>
</div>
2 changes: 1 addition & 1 deletion test/server-side-rendering/index.ts
Expand Up @@ -84,7 +84,7 @@ describe('ssr', () => {

try {
if (config.withoutNormalizeHtml) {
assert.strictEqual(html.trim(), expectedHtml.trim().replace(/\r\n/g, '\n'));
assert.strictEqual(html.trim().replace(/\r\n/g, '\n'), expectedHtml.trim().replace(/\r\n/g, '\n'));
} else {
(compileOptions.preserveComments
? assert.htmlEqualWithComments
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>