Skip to content

fix: use wholeText for only contenteditable for set_data #8394

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

Merged
merged 16 commits into from
Mar 21, 2023
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
33 changes: 18 additions & 15 deletions src/compiler/compile/render_dom/wrappers/Element/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,8 @@ export default class ElementWrapper extends Wrapper {
child_dynamic_element_block?: Block = null;
child_dynamic_element?: ElementWrapper = null;

element_data_name = null;

constructor(
renderer: Renderer,
block: Block,
Expand Down Expand Up @@ -287,6 +289,8 @@ export default class ElementWrapper extends Wrapper {
}

this.fragment = new FragmentWrapper(renderer, block, node.children, this, strip_whitespace, next_sibling);

this.element_data_name = block.get_unique_name(`${this.var.name}_data`);
}

render(block: Block, parent_node: Identifier, parent_nodes: Identifier) {
Expand Down Expand Up @@ -516,7 +520,8 @@ export default class ElementWrapper extends Wrapper {
child.render(
block,
is_template ? x`${node}.content` : node,
nodes
nodes,
{ element_data_name: this.element_data_name }
);
});
}
Expand Down Expand Up @@ -824,7 +829,6 @@ export default class ElementWrapper extends Wrapper {

add_spread_attributes(block: Block) {
const levels = block.get_unique_name(`${this.var.name}_levels`);
const data = block.get_unique_name(`${this.var.name}_data`);

const initial_props = [];
const updates = [];
Expand Down Expand Up @@ -855,9 +859,9 @@ export default class ElementWrapper extends Wrapper {
block.chunks.init.push(b`
let ${levels} = [${initial_props}];

let ${data} = {};
let ${this.element_data_name} = {};
for (let #i = 0; #i < ${levels}.length; #i += 1) {
${data} = @assign(${data}, ${levels}[#i]);
${this.element_data_name} = @assign(${this.element_data_name}, ${levels}[#i]);
}
`);

Expand All @@ -869,12 +873,12 @@ export default class ElementWrapper extends Wrapper {
: x`@set_attributes`;

block.chunks.hydrate.push(
b`${fn}(${this.var}, ${data});`
b`${fn}(${this.var}, ${this.element_data_name});`
);

if (this.has_dynamic_attribute) {
block.chunks.update.push(b`
${fn}(${this.var}, ${data} = @get_spread_update(${levels}, [
${fn}(${this.var}, ${this.element_data_name} = @get_spread_update(${levels}, [
${updates}
]));
`);
Expand All @@ -890,23 +894,23 @@ export default class ElementWrapper extends Wrapper {
}

block.chunks.mount.push(b`
'value' in ${data} && (${data}.multiple ? @select_options : @select_option)(${this.var}, ${data}.value);
'value' in ${this.element_data_name} && (${this.element_data_name}.multiple ? @select_options : @select_option)(${this.var}, ${this.element_data_name}.value);
`);

block.chunks.update.push(b`
if (${block.renderer.dirty(Array.from(dependencies))} && 'value' in ${data}) (${data}.multiple ? @select_options : @select_option)(${this.var}, ${data}.value);
if (${block.renderer.dirty(Array.from(dependencies))} && 'value' in ${this.element_data_name}) (${this.element_data_name}.multiple ? @select_options : @select_option)(${this.var}, ${this.element_data_name}.value);
`);
} else if (this.node.name === 'input' && this.attributes.find(attr => attr.node.name === 'value')) {
const type = this.node.get_static_attribute_value('type');
if (type === null || type === '' || type === 'text' || type === 'email' || type === 'password') {
block.chunks.mount.push(b`
if ('value' in ${data}) {
${this.var}.value = ${data}.value;
if ('value' in ${this.element_data_name}) {
${this.var}.value = ${this.element_data_name}.value;
}
`);
block.chunks.update.push(b`
if ('value' in ${data}) {
${this.var}.value = ${data}.value;
if ('value' in ${this.element_data_name}) {
${this.var}.value = ${this.element_data_name}.value;
}
`);
}
Expand Down Expand Up @@ -1220,8 +1224,8 @@ export default class ElementWrapper extends Wrapper {
if (this.dynamic_style_dependencies.size > 0) {
maybe_create_style_changed_var();
// If all dependencies are same as the style attribute dependencies, then we can skip the dirty check
condition =
all_deps.size === this.dynamic_style_dependencies.size
condition =
all_deps.size === this.dynamic_style_dependencies.size
? style_changed_var
: x`${style_changed_var} || ${condition}`;
}
Expand All @@ -1232,7 +1236,6 @@ export default class ElementWrapper extends Wrapper {
}
`);
}

});
}

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/compile/render_dom/wrappers/Fragment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import Body from './Body';
import DebugTag from './DebugTag';
import Document from './Document';
import EachBlock from './EachBlock';
import Element from './Element/index';
import Element from './Element';
import Head from './Head';
import IfBlock from './IfBlock';
import KeyBlock from './KeyBlock';
Expand Down
38 changes: 35 additions & 3 deletions src/compiler/compile/render_dom/wrappers/MustacheTag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ import Wrapper from './shared/Wrapper';
import MustacheTag from '../../nodes/MustacheTag';
import RawMustacheTag from '../../nodes/RawMustacheTag';
import { x } from 'code-red';
import { Identifier } from 'estree';
import { Identifier, Expression } from 'estree';
import ElementWrapper from './Element';
import AttributeWrapper from './Element/Attribute';

export default class MustacheTagWrapper extends Tag {
var: Identifier = { type: 'Identifier', name: 't' };
Expand All @@ -14,10 +16,40 @@ export default class MustacheTagWrapper extends Tag {
super(renderer, block, parent, node);
}

render(block: Block, parent_node: Identifier, parent_nodes: Identifier) {
render(block: Block, parent_node: Identifier, parent_nodes: Identifier, data: Record<string, unknown> | undefined) {
const contenteditable_attributes =
this.parent instanceof ElementWrapper &&
this.parent.attributes.filter((a) => a.node.name === 'contenteditable');

const spread_attributes =
this.parent instanceof ElementWrapper &&
this.parent.attributes.filter((a) => a.node.is_spread);

let contenteditable_attr_value: Expression | true | undefined = undefined;
if (contenteditable_attributes.length > 0) {
const attribute = contenteditable_attributes[0] as AttributeWrapper;
if ([true, 'true', ''].includes(attribute.node.get_static_value())) {
contenteditable_attr_value = true;
} else {
contenteditable_attr_value = x`${attribute.get_value(block)}`;
}
} else if (spread_attributes.length > 0 && data.element_data_name) {
contenteditable_attr_value = x`${data.element_data_name}['contenteditable']`;
}

const { init } = this.rename_this_method(
block,
value => x`@set_data(${this.var}, ${value})`
value => {
if (contenteditable_attr_value) {
if (contenteditable_attr_value === true) {
return x`@set_data_contenteditable(${this.var}, ${value})`;
} else {
return x`@set_data_maybe_contenteditable(${this.var}, ${value}, ${contenteditable_attr_value})`;
}
} else {
return x`@set_data(${this.var}, ${value})`;
}
}
);

block.add_element(
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/compile/render_dom/wrappers/shared/Wrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export default class Wrapper {
);
}

render(_block: Block, _parent_node: Identifier, _parent_nodes: Identifier) {
render(_block: Block, _parent_node: Identifier, _parent_nodes: Identifier, _data: Record<string, any> = undefined) {
throw Error('Wrapper class is not renderable');
}
}
21 changes: 18 additions & 3 deletions src/runtime/internal/dev.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { custom_event, append, append_hydration, insert, insert_hydration, detach, listen, attr } from './dom';
import { SvelteComponent } from './Component';
import { is_void } from '../../shared/utils/names';
import { contenteditable_truthy_values } from './utils';

export function dispatch_dev<T=any>(type: string, detail?: T) {
document.dispatchEvent(custom_event(type, { version: '__VERSION__', ...detail }, { bubbles: true }));
Expand Down Expand Up @@ -83,12 +84,26 @@ export function dataset_dev(node: HTMLElement, property: string, value?: any) {
dispatch_dev('SvelteDOMSetDataset', { node, property, value });
}

export function set_data_dev(text, data) {
export function set_data_dev(text: Text, data: unknown) {
data = '' + data;
if (text.wholeText === data) return;
if (text.data === data) return;
dispatch_dev('SvelteDOMSetData', { node: text, data });
text.data = (data as string);
}

export function set_data_contenteditable_dev(text: Text, data: unknown) {
data = '' + data;
if (text.wholeText === data) return;
dispatch_dev('SvelteDOMSetData', { node: text, data });
text.data = data;
text.data = (data as string);
}

export function set_data_maybe_contenteditable_dev(text: Text, data: unknown, attr_value: string) {
if (~contenteditable_truthy_values.indexOf(attr_value)) {
set_data_contenteditable_dev(text, data);
} else {
set_data_dev(text, data);
}
}

export function validate_each_argument(arg) {
Expand Down
21 changes: 18 additions & 3 deletions src/runtime/internal/dom.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { has_prop } from './utils';
import { contenteditable_truthy_values, has_prop } from './utils';

// Track which nodes are claimed during hydration. Unclaimed nodes can then be removed from the DOM
// at the end of hydration without touching the remaining nodes.
Expand Down Expand Up @@ -581,9 +581,24 @@ export function claim_html_tag(nodes, is_svg: boolean) {
return new HtmlTagHydration(claimed_nodes, is_svg);
}

export function set_data(text, data) {
export function set_data(text: Text, data: unknown) {
data = '' + data;
if (text.wholeText !== data) text.data = data;
if (text.data === data) return;
text.data = (data as string);
}

export function set_data_contenteditable(text: Text, data: unknown) {
data = '' + data;
if (text.wholeText === data) return;
text.data = (data as string);
}

export function set_data_maybe_contenteditable(text: Text, data: unknown, attr_value: string) {
if (~contenteditable_truthy_values.indexOf(attr_value)) {
set_data_contenteditable(text, data);
} else {
set_data(text, data);
}
}

export function set_input_value(input, value) {
Expand Down
2 changes: 2 additions & 0 deletions src/runtime/internal/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,3 +194,5 @@ export function split_css_unit(value: number | string): [number, string] {
const split = typeof value === 'string' && value.match(/^\s*(-?[\d.]+)([^\s]*)\s*$/);
return split ? [parseFloat(split[1]), split[2] || 'px'] : [value as number, 'px'];
}

export const contenteditable_truthy_values = ['', true, 1, 'true', 'contenteditable'];
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// A puppeteer test because JSDOM doesn't support contenteditable
export default {
html: '<div contenteditable="false"></div>',

async test({ assert, target, component, window }) {
const div = target.querySelector('div');
const text = window.document.createTextNode('a');
div.insertBefore(text, null);
assert.equal(div.textContent, 'a');
component.text = 'bcde';
assert.equal(div.textContent, 'bcdea');
}
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<script>
export let text = '';
const updater = (event) => {text = event.target.textContent}
</script>

<div contenteditable="false" on:input={updater}>{text}</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// A puppeteer test because JSDOM doesn't support contenteditable
export default {
html: '<div contenteditable="true"></div>',
ssrHtml: '<div contenteditable=""></div>',

async test({ assert, target, window }) {
// this tests that by going from contenteditable=true to false, the
// content is correctly updated before that. This relies on the order
// of the updates: first updating the content, then setting contenteditable
// to false, which means that `set_data_maybe_contenteditable` is used and not `set_data`.
// If the order is reversed, https://github.com/sveltejs/svelte/issues/5018
// would be happening. The caveat is that if we go from contenteditable=false to true
// then we will have the same issue. To fix this reliably we probably need to
// overhaul the way we handle text updates in general.
// If due to some refactoring this test fails, it's probably fine to ignore it since
// this is a very specific edge case and the behavior is unstable anyway.
const div = target.querySelector('div');
const text = window.document.createTextNode('a');
div.insertBefore(text, null);
const event = new window.InputEvent('input');
await div.dispatchEvent(event);
assert.equal(div.textContent, 'a');
}
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<script>
let text = "";
const updater = (event) => {
text = event.target.textContent;
};
$: spread = {
contenteditable: text !== "a",
};
</script>

<div {...spread} on:input={updater}>{text}</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// A puppeteer test because JSDOM doesn't support contenteditable
export default {
html: '<div contenteditable=""></div>',

// Failing test for https://github.com/sveltejs/svelte/issues/5018, fix pending
// It's hard to fix this because in order to do that, we would need to change the
// way the value is compared completely. Right now it compares the value of the
// first text node, but it should compare the value of the whole content
skip: true,

async test({ assert, target, window }) {
const div = target.querySelector('div');

let text = window.document.createTextNode('a');
div.insertBefore(text, null);
let event = new window.InputEvent('input');
await div.dispatchEvent(event);
assert.equal(div.textContent, 'a');

// When a user types a newline, the browser inserts a <div> element
const inner_div = window.document.createElement('div');
div.insertBefore(inner_div, null);
event = new window.InputEvent('input');
await div.dispatchEvent(event);
assert.equal(div.textContent, 'a');

text = window.document.createTextNode('b');
inner_div.insertBefore(text, null);
event = new window.InputEvent('input');
await div.dispatchEvent(event);
assert.equal(div.textContent, 'ab');
}
};

This file was deleted.

9 changes: 9 additions & 0 deletions test/runtime/samples/reactive-values-text-node/_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export default {
html:'<div>same text</div>',
async test({ assert, target }) {
await new Promise(f => setTimeout(f, 10));
assert.htmlEqual(target.innerHTML, `
<div>same text text</div>
`);
}
};
8 changes: 8 additions & 0 deletions test/runtime/samples/reactive-values-text-node/main.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<script>
let text = 'same';
setTimeout(() => {
text = 'same text';
}, 5);
</script>

<div>{text} text</div>