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 render fallback slot content due to white space #4500

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

* In `vars` array, correctly indicate whether `module` variables are `mutated` or `reassigned` ([#3215](https://github.com/sveltejs/svelte/issues/3215))
* Fix spread props not updating in certain situations ([#3521](https://github.com/sveltejs/svelte/issues/3521), [#4480](https://github.com/sveltejs/svelte/issues/4480))
* Use the fallback content for slots if they are passed only whitespace ([#4092](https://github.com/sveltejs/svelte/issues/4092))
* In `dev` mode, check for unknown props even if the component has no writable props ([#4323](https://github.com/sveltejs/svelte/issues/4323))
* Exclude global variables from `$capture_state` ([#4463](https://github.com/sveltejs/svelte/issues/4463))
* Fix bitmask overflow for slots ([#4481](https://github.com/sveltejs/svelte/issues/4481))
Expand Down
29 changes: 29 additions & 0 deletions src/compiler/compile/nodes/Text.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,18 @@ import Component from '../Component';
import TemplateScope from './shared/TemplateScope';
import { INode } from './interfaces';

// Whitespace inside one of these elements will not result in
// a whitespace node being created in any circumstances. (This
// list is almost certainly very incomplete)
const elements_without_text = new Set([
'audio',
'datalist',
'dl',
'optgroup',
'select',
'video',
]);

export default class Text extends Node {
type: 'Text';
data: string;
Expand All @@ -13,4 +25,21 @@ export default class Text extends Node {
this.data = info.data;
this.synthetic = info.synthetic || false;
}

should_skip() {
if (/\S/.test(this.data)) return false;

const parent_element = this.find_nearest(/(?:Element|InlineComponent|Head)/);
if (!parent_element) return false;

if (parent_element.type === 'Head') return true;
if (parent_element.type === 'InlineComponent') return parent_element.children.length === 1 && this === parent_element.children[0];

// svg namespace exclusions
if (/svg$/.test(parent_element.namespace)) {
if (this.prev && this.prev.type === "Element" && this.prev.name === "tspan") return false;
}

return parent_element.namespace || elements_without_text.has(parent_element.name);
}
}
6 changes: 1 addition & 5 deletions src/compiler/compile/render_dom/wrappers/Fragment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { INode } from '../../nodes/interfaces';
import Renderer from '../Renderer';
import Block from '../Block';
import { trim_start, trim_end } from '../../../utils/trim';
import { link } from '../../../utils/link';
import { Identifier } from 'estree';

const wrappers = {
Expand All @@ -38,11 +39,6 @@ const wrappers = {
Window
};

function link(next: Wrapper, prev: Wrapper) {
prev.next = next;
if (next) next.prev = prev;
}

function trimmable_at(child: INode, next_sibling: Wrapper): boolean {
// Whitespace is trimmable if one of the following is true:
// The child and its sibling share a common nearest each block (not at an each block boundary)
Expand Down
25 changes: 16 additions & 9 deletions src/compiler/compile/render_dom/wrappers/InlineComponent/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,11 +138,27 @@ export default class InlineComponentWrapper extends Wrapper {
const statements: Array<Node | Node[]> = [];
const updates: Array<Node | Node[]> = [];

if (this.fragment) {
this.renderer.add_to_context('$$scope', true);
const default_slot = this.slots.get('default');

this.fragment.nodes.forEach((child) => {
child.render(default_slot.block, null, x`#nodes` as unknown as Identifier);
});
}

let props;
const name_changes = block.get_unique_name(`${name.name}_changes`);

const uses_spread = !!this.node.attributes.find(a => a.is_spread);

// removing empty slot
for (const slot of this.slots.keys()) {
if (!this.slots.get(slot).block.has_content()) {
this.slots.delete(slot);
}
}

const initial_props = this.slots.size > 0
? [
p`$$slots: {
Expand Down Expand Up @@ -172,15 +188,6 @@ export default class InlineComponentWrapper extends Wrapper {
}
}

if (this.fragment) {
this.renderer.add_to_context('$$scope', true);
const default_slot = this.slots.get('default');

this.fragment.nodes.forEach((child) => {
child.render(default_slot.block, null, x`#nodes` as unknown as Identifier);
});
}

if (component.compile_options.dev) {
// TODO this is a terrible hack, but without it the component
// will complain that options.target is missing. This would
Expand Down
32 changes: 1 addition & 31 deletions src/compiler/compile/render_dom/wrappers/Text.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,36 +5,6 @@ import Wrapper from './shared/Wrapper';
import { x } from 'code-red';
import { Identifier } from 'estree';

// Whitespace inside one of these elements will not result in
// a whitespace node being created in any circumstances. (This
// list is almost certainly very incomplete)
const elements_without_text = new Set([
'audio',
'datalist',
'dl',
'optgroup',
'select',
'video',
]);

// TODO this should probably be in Fragment
function should_skip(node: Text) {
if (/\S/.test(node.data)) return false;

const parent_element = node.find_nearest(/(?:Element|InlineComponent|Head)/);
if (!parent_element) return false;

if (parent_element.type === 'Head') return true;
if (parent_element.type === 'InlineComponent') return parent_element.children.length === 1 && node === parent_element.children[0];

// svg namespace exclusions
if (/svg$/.test(parent_element.namespace)) {
if (node.prev && node.prev.type === "Element" && node.prev.name === "tspan") return false;
}

return parent_element.namespace || elements_without_text.has(parent_element.name);
}

export default class TextWrapper extends Wrapper {
node: Text;
data: string;
Expand All @@ -50,7 +20,7 @@ export default class TextWrapper extends Wrapper {
) {
super(renderer, block, parent, node);

this.skip = should_skip(this.node);
this.skip = this.node.should_skip();
this.data = data;
this.var = (this.skip ? null : x`t`) as unknown as Identifier;
}
Expand Down
11 changes: 8 additions & 3 deletions src/compiler/compile/render_ssr/handlers/Element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,14 @@ import Renderer, { RenderOptions } from '../Renderer';
import Element from '../../nodes/Element';
import { x } from 'code-red';
import Expression from '../../nodes/shared/Expression';
import remove_whitespace_children from './utils/remove_whitespace_children';

export default function(node: Element, renderer: Renderer, options: RenderOptions & {
slot_scopes: Map<any, any>;
}) {

const children = remove_whitespace_children(node.children, node.next);

// awkward special case
let node_contents;

Expand Down Expand Up @@ -133,7 +137,7 @@ export default function(node: Element, renderer: Renderer, options: RenderOption
if (node_contents !== undefined) {
if (contenteditable) {
renderer.push();
renderer.render(node.children, options);
renderer.render(children, options);
const result = renderer.pop();

renderer.add_expression(x`($$value => $$value === void 0 ? ${result} : $$value)(${node_contents})`);
Expand All @@ -145,7 +149,7 @@ export default function(node: Element, renderer: Renderer, options: RenderOption
renderer.add_string(`</${node.name}>`);
}
} else if (slot && nearest_inline_component) {
renderer.render(node.children, options);
renderer.render(children, options);

if (!is_void(node.name)) {
renderer.add_string(`</${node.name}>`);
Expand All @@ -163,10 +167,11 @@ export default function(node: Element, renderer: Renderer, options: RenderOption
output: renderer.pop()
});
} else {
renderer.render(node.children, options);
renderer.render(children, options);

if (!is_void(node.name)) {
renderer.add_string(`</${node.name}>`);
}
}
}

23 changes: 18 additions & 5 deletions src/compiler/compile/render_ssr/handlers/InlineComponent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { string_literal } from '../../utils/stringify';
import Renderer, { RenderOptions } from '../Renderer';
import { get_slot_scope } from './shared/get_slot_scope';
import InlineComponent from '../../nodes/InlineComponent';
import remove_whitespace_children from './utils/remove_whitespace_children';
import { p, x } from 'code-red';

function get_prop_value(attribute) {
Expand Down Expand Up @@ -67,12 +68,14 @@ export default function(node: InlineComponent, renderer: Renderer, options: Rend

const slot_fns = [];

if (node.children.length) {
const children = remove_whitespace_children(node.children, node.next);

if (children.length) {
const slot_scopes = new Map();

renderer.push();

renderer.render(node.children, Object.assign({}, options, {
renderer.render(children, Object.assign({}, options, {
slot_scopes
}));

Expand All @@ -82,9 +85,11 @@ export default function(node: InlineComponent, renderer: Renderer, options: Rend
});

slot_scopes.forEach(({ input, output }, name) => {
slot_fns.push(
p`${name}: (${input}) => ${output}`
);
if (!is_empty_template_literal(output)) {
slot_fns.push(
p`${name}: (${input}) => ${output}`
);
}
});
}

Expand All @@ -94,3 +99,11 @@ export default function(node: InlineComponent, renderer: Renderer, options: Rend

renderer.add_expression(x`@validate_component(${expression}, "${node.name}").$$render($$result, ${props}, ${bindings}, ${slots})`);
}

function is_empty_template_literal(template_literal) {
return (
template_literal.expressions.length === 0 &&
template_literal.quasis.length === 1 &&
template_literal.quasis[0].value.raw === ""
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { INode } from '../../../nodes/interfaces';
import { trim_end, trim_start } from '../../../../utils/trim';
import { link } from '../../../../utils/link';

// similar logic from `compile/render_dom/wrappers/Fragment`
// We want to remove trailing whitespace inside an element/component/block,
// *unless* there is no whitespace between this node and its next sibling
export default function remove_whitespace_children(children: INode[], next?: INode): INode[] {
const nodes: INode[] = [];
let last_child: INode;
let i = children.length;
while (i--) {
const child = children[i];

if (child.type === 'Text') {
if (child.should_skip()) {
continue;
}

let { data } = child;

if (nodes.length === 0) {
const should_trim = next
? next.type === 'Text' &&
/^\s/.test(next.data) &&
trimmable_at(child, next)
: !child.has_ancestor('EachBlock');

if (should_trim) {
data = trim_end(data);
if (!data) continue;
}
}

// glue text nodes (which could e.g. be separated by comments) together
if (last_child && last_child.type === 'Text') {
last_child.data = data + last_child.data;
continue;
}

nodes.unshift(child);
link(last_child, last_child = child);
} else {
nodes.unshift(child);
link(last_child, last_child = child);
}
}

const first = nodes[0];
if (first && first.type === 'Text') {
first.data = trim_start(first.data);
if (!first.data) {
first.var = null;
nodes.shift();

if (nodes[0]) {
nodes[0].prev = null;
}
}
}

return nodes;
}

function trimmable_at(child: INode, next_sibling: INode): boolean {
// Whitespace is trimmable if one of the following is true:
// The child and its sibling share a common nearest each block (not at an each block boundary)
// The next sibling's previous node is an each block
return (
next_sibling.find_nearest(/EachBlock/) ===
child.find_nearest(/EachBlock/) || next_sibling.prev.type === 'EachBlock'
);
}
4 changes: 4 additions & 0 deletions src/compiler/utils/link.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export function link<T extends { next?: T; prev?: T }>(next: T, prev: T) {
prev.next = next;
if (next) next.prev = prev;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<div>
<slot><p class='default'>default fallback content</p></slot>
<slot name='bar'>bar fallback</slot>
</div>
13 changes: 13 additions & 0 deletions test/runtime/samples/component-slot-fallback-empty/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
export default {
html: `
<div>
<p class="default">default fallback content</p>
<input slot="bar">
</div>

<div>
<p class="default">default fallback content</p>
bar fallback
</div>
`
};
10 changes: 10 additions & 0 deletions test/runtime/samples/component-slot-fallback-empty/main.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<script>
import Nested from "./Nested.svelte";
</script>

<Nested>
<input slot="bar">
</Nested>

<Nested>
</Nested>