Skip to content

Commit a2170f5

Browse files
baseballyamasuxin2017dummdidumm
authored
fix: use wholeText for only contenteditable for set_data (#8394)
- split logic up into "is this a contenteditable element" and depending on the outcome use either .wholeText or .data to check if an update is necessary - add to puppeteer because jsdom does not support contenteditable - one test is skipped it because it fails right now but helps test #5018 --------- Co-authored-by: suxin2017 <[email protected]> Co-authored-by: Simon H <[email protected]> Co-authored-by: Simon Holthausen <[email protected]>
1 parent 6ce6f14 commit a2170f5

File tree

16 files changed

+197
-41
lines changed

16 files changed

+197
-41
lines changed

‎src/compiler/compile/render_dom/wrappers/Element/index.ts

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,8 @@ export default class ElementWrapper extends Wrapper {
174174
child_dynamic_element_block?: Block = null;
175175
child_dynamic_element?: ElementWrapper = null;
176176

177+
element_data_name = null;
178+
177179
constructor(
178180
renderer: Renderer,
179181
block: Block,
@@ -287,6 +289,8 @@ export default class ElementWrapper extends Wrapper {
287289
}
288290

289291
this.fragment = new FragmentWrapper(renderer, block, node.children, this, strip_whitespace, next_sibling);
292+
293+
this.element_data_name = block.get_unique_name(`${this.var.name}_data`);
290294
}
291295

292296
render(block: Block, parent_node: Identifier, parent_nodes: Identifier) {
@@ -516,7 +520,8 @@ export default class ElementWrapper extends Wrapper {
516520
child.render(
517521
block,
518522
is_template ? x`${node}.content` : node,
519-
nodes
523+
nodes,
524+
{ element_data_name: this.element_data_name }
520525
);
521526
});
522527
}
@@ -824,7 +829,6 @@ export default class ElementWrapper extends Wrapper {
824829

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

829833
const initial_props = [];
830834
const updates = [];
@@ -855,9 +859,9 @@ export default class ElementWrapper extends Wrapper {
855859
block.chunks.init.push(b`
856860
let ${levels} = [${initial_props}];
857861
858-
let ${data} = {};
862+
let ${this.element_data_name} = {};
859863
for (let #i = 0; #i < ${levels}.length; #i += 1) {
860-
${data} = @assign(${data}, ${levels}[#i]);
864+
${this.element_data_name} = @assign(${this.element_data_name}, ${levels}[#i]);
861865
}
862866
`);
863867

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

871875
block.chunks.hydrate.push(
872-
b`${fn}(${this.var}, ${data});`
876+
b`${fn}(${this.var}, ${this.element_data_name});`
873877
);
874878

875879
if (this.has_dynamic_attribute) {
876880
block.chunks.update.push(b`
877-
${fn}(${this.var}, ${data} = @get_spread_update(${levels}, [
881+
${fn}(${this.var}, ${this.element_data_name} = @get_spread_update(${levels}, [
878882
${updates}
879883
]));
880884
`);
@@ -890,23 +894,23 @@ export default class ElementWrapper extends Wrapper {
890894
}
891895

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

896900
block.chunks.update.push(b`
897-
if (${block.renderer.dirty(Array.from(dependencies))} && 'value' in ${data}) (${data}.multiple ? @select_options : @select_option)(${this.var}, ${data}.value);
901+
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);
898902
`);
899903
} else if (this.node.name === 'input' && this.attributes.find(attr => attr.node.name === 'value')) {
900904
const type = this.node.get_static_attribute_value('type');
901905
if (type === null || type === '' || type === 'text' || type === 'email' || type === 'password') {
902906
block.chunks.mount.push(b`
903-
if ('value' in ${data}) {
904-
${this.var}.value = ${data}.value;
907+
if ('value' in ${this.element_data_name}) {
908+
${this.var}.value = ${this.element_data_name}.value;
905909
}
906910
`);
907911
block.chunks.update.push(b`
908-
if ('value' in ${data}) {
909-
${this.var}.value = ${data}.value;
912+
if ('value' in ${this.element_data_name}) {
913+
${this.var}.value = ${this.element_data_name}.value;
910914
}
911915
`);
912916
}
@@ -1220,8 +1224,8 @@ export default class ElementWrapper extends Wrapper {
12201224
if (this.dynamic_style_dependencies.size > 0) {
12211225
maybe_create_style_changed_var();
12221226
// If all dependencies are same as the style attribute dependencies, then we can skip the dirty check
1223-
condition =
1224-
all_deps.size === this.dynamic_style_dependencies.size
1227+
condition =
1228+
all_deps.size === this.dynamic_style_dependencies.size
12251229
? style_changed_var
12261230
: x`${style_changed_var} || ${condition}`;
12271231
}
@@ -1232,7 +1236,6 @@ export default class ElementWrapper extends Wrapper {
12321236
}
12331237
`);
12341238
}
1235-
12361239
});
12371240
}
12381241

‎src/compiler/compile/render_dom/wrappers/Fragment.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import Body from './Body';
44
import DebugTag from './DebugTag';
55
import Document from './Document';
66
import EachBlock from './EachBlock';
7-
import Element from './Element/index';
7+
import Element from './Element';
88
import Head from './Head';
99
import IfBlock from './IfBlock';
1010
import KeyBlock from './KeyBlock';

‎src/compiler/compile/render_dom/wrappers/MustacheTag.ts

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,9 @@ import Wrapper from './shared/Wrapper';
55
import MustacheTag from '../../nodes/MustacheTag';
66
import RawMustacheTag from '../../nodes/RawMustacheTag';
77
import { x } from 'code-red';
8-
import { Identifier } from 'estree';
8+
import { Identifier, Expression } from 'estree';
9+
import ElementWrapper from './Element';
10+
import AttributeWrapper from './Element/Attribute';
911

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

17-
render(block: Block, parent_node: Identifier, parent_nodes: Identifier) {
19+
render(block: Block, parent_node: Identifier, parent_nodes: Identifier, data: Record<string, unknown> | undefined) {
20+
const contenteditable_attributes =
21+
this.parent instanceof ElementWrapper &&
22+
this.parent.attributes.filter((a) => a.node.name === 'contenteditable');
23+
24+
const spread_attributes =
25+
this.parent instanceof ElementWrapper &&
26+
this.parent.attributes.filter((a) => a.node.is_spread);
27+
28+
let contenteditable_attr_value: Expression | true | undefined = undefined;
29+
if (contenteditable_attributes.length > 0) {
30+
const attribute = contenteditable_attributes[0] as AttributeWrapper;
31+
if ([true, 'true', ''].includes(attribute.node.get_static_value())) {
32+
contenteditable_attr_value = true;
33+
} else {
34+
contenteditable_attr_value = x`${attribute.get_value(block)}`;
35+
}
36+
} else if (spread_attributes.length > 0 && data.element_data_name) {
37+
contenteditable_attr_value = x`${data.element_data_name}['contenteditable']`;
38+
}
39+
1840
const { init } = this.rename_this_method(
1941
block,
20-
value => x`@set_data(${this.var}, ${value})`
42+
value => {
43+
if (contenteditable_attr_value) {
44+
if (contenteditable_attr_value === true) {
45+
return x`@set_data_contenteditable(${this.var}, ${value})`;
46+
} else {
47+
return x`@set_data_maybe_contenteditable(${this.var}, ${value}, ${contenteditable_attr_value})`;
48+
}
49+
} else {
50+
return x`@set_data(${this.var}, ${value})`;
51+
}
52+
}
2153
);
2254

2355
block.add_element(

‎src/compiler/compile/render_dom/wrappers/shared/Wrapper.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ export default class Wrapper {
8585
);
8686
}
8787

88-
render(_block: Block, _parent_node: Identifier, _parent_nodes: Identifier) {
88+
render(_block: Block, _parent_node: Identifier, _parent_nodes: Identifier, _data: Record<string, any> = undefined) {
8989
throw Error('Wrapper class is not renderable');
9090
}
9191
}

‎src/runtime/internal/dev.ts

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { custom_event, append, append_hydration, insert, insert_hydration, detach, listen, attr } from './dom';
22
import { SvelteComponent } from './Component';
33
import { is_void } from '../../shared/utils/names';
4+
import { contenteditable_truthy_values } from './utils';
45

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

86-
export function set_data_dev(text, data) {
87+
export function set_data_dev(text: Text, data: unknown) {
8788
data = '' + data;
88-
if (text.wholeText === data) return;
89+
if (text.data === data) return;
90+
dispatch_dev('SvelteDOMSetData', { node: text, data });
91+
text.data = (data as string);
92+
}
8993

94+
export function set_data_contenteditable_dev(text: Text, data: unknown) {
95+
data = '' + data;
96+
if (text.wholeText === data) return;
9097
dispatch_dev('SvelteDOMSetData', { node: text, data });
91-
text.data = data;
98+
text.data = (data as string);
99+
}
100+
101+
export function set_data_maybe_contenteditable_dev(text: Text, data: unknown, attr_value: string) {
102+
if (~contenteditable_truthy_values.indexOf(attr_value)) {
103+
set_data_contenteditable_dev(text, data);
104+
} else {
105+
set_data_dev(text, data);
106+
}
92107
}
93108

94109
export function validate_each_argument(arg) {

‎src/runtime/internal/dom.ts

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { has_prop } from './utils';
1+
import { contenteditable_truthy_values, has_prop } from './utils';
22

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

584-
export function set_data(text, data) {
584+
export function set_data(text: Text, data: unknown) {
585585
data = '' + data;
586-
if (text.wholeText !== data) text.data = data;
586+
if (text.data === data) return;
587+
text.data = (data as string);
588+
}
589+
590+
export function set_data_contenteditable(text: Text, data: unknown) {
591+
data = '' + data;
592+
if (text.wholeText === data) return;
593+
text.data = (data as string);
594+
}
595+
596+
export function set_data_maybe_contenteditable(text: Text, data: unknown, attr_value: string) {
597+
if (~contenteditable_truthy_values.indexOf(attr_value)) {
598+
set_data_contenteditable(text, data);
599+
} else {
600+
set_data(text, data);
601+
}
587602
}
588603

589604
export function set_input_value(input, value) {

‎src/runtime/internal/utils.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,3 +194,5 @@ export function split_css_unit(value: number | string): [number, string] {
194194
const split = typeof value === 'string' && value.match(/^\s*(-?[\d.]+)([^\s]*)\s*$/);
195195
return split ? [parseFloat(split[1]), split[2] || 'px'] : [value as number, 'px'];
196196
}
197+
198+
export const contenteditable_truthy_values = ['', true, 1, 'true', 'contenteditable'];

‎test/runtime-puppeteer/samples/component-event-handler-contenteditable-false/_config.js

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// A puppeteer test because JSDOM doesn't support contenteditable
2+
export default {
3+
html: '<div contenteditable="false"></div>',
4+
5+
async test({ assert, target, component, window }) {
6+
const div = target.querySelector('div');
7+
const text = window.document.createTextNode('a');
8+
div.insertBefore(text, null);
9+
assert.equal(div.textContent, 'a');
10+
component.text = 'bcde';
11+
assert.equal(div.textContent, 'bcdea');
12+
}
13+
};

‎test/runtime-puppeteer/samples/component-event-handler-contenteditable-false/main.svelte

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
<script>
2+
export let text = '';
3+
const updater = (event) => {text = event.target.textContent}
4+
</script>
5+
6+
<div contenteditable="false" on:input={updater}>{text}</div>

‎test/runtime-puppeteer/samples/component-event-handler-contenteditable-spread/_config.js

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// A puppeteer test because JSDOM doesn't support contenteditable
2+
export default {
3+
html: '<div contenteditable="true"></div>',
4+
ssrHtml: '<div contenteditable=""></div>',
5+
6+
async test({ assert, target, window }) {
7+
// this tests that by going from contenteditable=true to false, the
8+
// content is correctly updated before that. This relies on the order
9+
// of the updates: first updating the content, then setting contenteditable
10+
// to false, which means that `set_data_maybe_contenteditable` is used and not `set_data`.
11+
// If the order is reversed, https://github.com/sveltejs/svelte/issues/5018
12+
// would be happening. The caveat is that if we go from contenteditable=false to true
13+
// then we will have the same issue. To fix this reliably we probably need to
14+
// overhaul the way we handle text updates in general.
15+
// If due to some refactoring this test fails, it's probably fine to ignore it since
16+
// this is a very specific edge case and the behavior is unstable anyway.
17+
const div = target.querySelector('div');
18+
const text = window.document.createTextNode('a');
19+
div.insertBefore(text, null);
20+
const event = new window.InputEvent('input');
21+
await div.dispatchEvent(event);
22+
assert.equal(div.textContent, 'a');
23+
}
24+
};

‎test/runtime-puppeteer/samples/component-event-handler-contenteditable-spread/main.svelte

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<script>
2+
let text = "";
3+
const updater = (event) => {
4+
text = event.target.textContent;
5+
};
6+
$: spread = {
7+
contenteditable: text !== "a",
8+
};
9+
</script>
10+
11+
<div {...spread} on:input={updater}>{text}</div>

‎test/runtime-puppeteer/samples/component-event-handler-contenteditable/_config.js

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
// A puppeteer test because JSDOM doesn't support contenteditable
2+
export default {
3+
html: '<div contenteditable=""></div>',
4+
5+
// Failing test for https://github.com/sveltejs/svelte/issues/5018, fix pending
6+
// It's hard to fix this because in order to do that, we would need to change the
7+
// way the value is compared completely. Right now it compares the value of the
8+
// first text node, but it should compare the value of the whole content
9+
skip: true,
10+
11+
async test({ assert, target, window }) {
12+
const div = target.querySelector('div');
13+
14+
let text = window.document.createTextNode('a');
15+
div.insertBefore(text, null);
16+
let event = new window.InputEvent('input');
17+
await div.dispatchEvent(event);
18+
assert.equal(div.textContent, 'a');
19+
20+
// When a user types a newline, the browser inserts a <div> element
21+
const inner_div = window.document.createElement('div');
22+
div.insertBefore(inner_div, null);
23+
event = new window.InputEvent('input');
24+
await div.dispatchEvent(event);
25+
assert.equal(div.textContent, 'a');
26+
27+
text = window.document.createTextNode('b');
28+
inner_div.insertBefore(text, null);
29+
event = new window.InputEvent('input');
30+
await div.dispatchEvent(event);
31+
assert.equal(div.textContent, 'ab');
32+
}
33+
};

‎test/runtime/samples/component-event-handler-contenteditable/main.svelte renamed to ‎test/runtime-puppeteer/samples/component-event-handler-contenteditable/main.svelte

File renamed without changes.

‎test/runtime/samples/component-event-handler-contenteditable/_config.js

Lines changed: 0 additions & 15 deletions
This file was deleted.

‎test/runtime/samples/reactive-values-text-node/_config.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
export default {
2+
html:'<div>same text</div>',
3+
async test({ assert, target }) {
4+
await new Promise(f => setTimeout(f, 10));
5+
assert.htmlEqual(target.innerHTML, `
6+
<div>same text text</div>
7+
`);
8+
}
9+
};

‎test/runtime/samples/reactive-values-text-node/main.svelte

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<script>
2+
let text = 'same';
3+
setTimeout(() => {
4+
text = 'same text';
5+
}, 5);
6+
</script>
7+
8+
<div>{text} text</div>

0 commit comments

Comments
 (0)