From a2170f5bd5312ed6a63cc1a465826705dc295cd9 Mon Sep 17 00:00:00 2001 From: Yuichiro Yamashita Date: Tue, 21 Mar 2023 22:30:25 +0900 Subject: [PATCH] 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 <1107178482@qq.com> Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> Co-authored-by: Simon Holthausen --- .../render_dom/wrappers/Element/index.ts | 33 ++++++++-------- .../compile/render_dom/wrappers/Fragment.ts | 2 +- .../render_dom/wrappers/MustacheTag.ts | 38 +++++++++++++++++-- .../render_dom/wrappers/shared/Wrapper.ts | 2 +- src/runtime/internal/dev.ts | 21 ++++++++-- src/runtime/internal/dom.ts | 21 ++++++++-- src/runtime/internal/utils.ts | 2 + .../_config.js | 13 +++++++ .../main.svelte | 6 +++ .../_config.js | 24 ++++++++++++ .../main.svelte | 11 ++++++ .../_config.js | 33 ++++++++++++++++ .../main.svelte | 0 .../_config.js | 15 -------- .../reactive-values-text-node/_config.js | 9 +++++ .../reactive-values-text-node/main.svelte | 8 ++++ 16 files changed, 197 insertions(+), 41 deletions(-) create mode 100644 test/runtime-puppeteer/samples/component-event-handler-contenteditable-false/_config.js create mode 100644 test/runtime-puppeteer/samples/component-event-handler-contenteditable-false/main.svelte create mode 100644 test/runtime-puppeteer/samples/component-event-handler-contenteditable-spread/_config.js create mode 100644 test/runtime-puppeteer/samples/component-event-handler-contenteditable-spread/main.svelte create mode 100644 test/runtime-puppeteer/samples/component-event-handler-contenteditable/_config.js rename test/{runtime => runtime-puppeteer}/samples/component-event-handler-contenteditable/main.svelte (100%) delete mode 100644 test/runtime/samples/component-event-handler-contenteditable/_config.js create mode 100644 test/runtime/samples/reactive-values-text-node/_config.js create mode 100644 test/runtime/samples/reactive-values-text-node/main.svelte diff --git a/src/compiler/compile/render_dom/wrappers/Element/index.ts b/src/compiler/compile/render_dom/wrappers/Element/index.ts index 1f80cee89cf0..7a0bdbaa060a 100644 --- a/src/compiler/compile/render_dom/wrappers/Element/index.ts +++ b/src/compiler/compile/render_dom/wrappers/Element/index.ts @@ -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, @@ -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) { @@ -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 } ); }); } @@ -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 = []; @@ -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]); } `); @@ -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} ])); `); @@ -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; } `); } @@ -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}`; } @@ -1232,7 +1236,6 @@ export default class ElementWrapper extends Wrapper { } `); } - }); } diff --git a/src/compiler/compile/render_dom/wrappers/Fragment.ts b/src/compiler/compile/render_dom/wrappers/Fragment.ts index 8c633aeb71fd..463c8db24a1b 100644 --- a/src/compiler/compile/render_dom/wrappers/Fragment.ts +++ b/src/compiler/compile/render_dom/wrappers/Fragment.ts @@ -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'; diff --git a/src/compiler/compile/render_dom/wrappers/MustacheTag.ts b/src/compiler/compile/render_dom/wrappers/MustacheTag.ts index ea6634ec803c..0bfa0198aea8 100644 --- a/src/compiler/compile/render_dom/wrappers/MustacheTag.ts +++ b/src/compiler/compile/render_dom/wrappers/MustacheTag.ts @@ -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' }; @@ -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 | 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( diff --git a/src/compiler/compile/render_dom/wrappers/shared/Wrapper.ts b/src/compiler/compile/render_dom/wrappers/shared/Wrapper.ts index 4bf8c20bd876..53847d887071 100644 --- a/src/compiler/compile/render_dom/wrappers/shared/Wrapper.ts +++ b/src/compiler/compile/render_dom/wrappers/shared/Wrapper.ts @@ -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 = undefined) { throw Error('Wrapper class is not renderable'); } } diff --git a/src/runtime/internal/dev.ts b/src/runtime/internal/dev.ts index be81a115147c..cd47138fefcc 100644 --- a/src/runtime/internal/dev.ts +++ b/src/runtime/internal/dev.ts @@ -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(type: string, detail?: T) { document.dispatchEvent(custom_event(type, { version: '__VERSION__', ...detail }, { bubbles: true })); @@ -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) { diff --git a/src/runtime/internal/dom.ts b/src/runtime/internal/dom.ts index 1f786d51b5b2..a7466131708e 100644 --- a/src/runtime/internal/dom.ts +++ b/src/runtime/internal/dom.ts @@ -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. @@ -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) { diff --git a/src/runtime/internal/utils.ts b/src/runtime/internal/utils.ts index b1c27a355de1..192f40140e4c 100644 --- a/src/runtime/internal/utils.ts +++ b/src/runtime/internal/utils.ts @@ -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']; diff --git a/test/runtime-puppeteer/samples/component-event-handler-contenteditable-false/_config.js b/test/runtime-puppeteer/samples/component-event-handler-contenteditable-false/_config.js new file mode 100644 index 000000000000..3763360fd82a --- /dev/null +++ b/test/runtime-puppeteer/samples/component-event-handler-contenteditable-false/_config.js @@ -0,0 +1,13 @@ +// A puppeteer test because JSDOM doesn't support contenteditable +export default { + html: '
', + + 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'); + } +}; diff --git a/test/runtime-puppeteer/samples/component-event-handler-contenteditable-false/main.svelte b/test/runtime-puppeteer/samples/component-event-handler-contenteditable-false/main.svelte new file mode 100644 index 000000000000..955592548748 --- /dev/null +++ b/test/runtime-puppeteer/samples/component-event-handler-contenteditable-false/main.svelte @@ -0,0 +1,6 @@ + + +
{text}
diff --git a/test/runtime-puppeteer/samples/component-event-handler-contenteditable-spread/_config.js b/test/runtime-puppeteer/samples/component-event-handler-contenteditable-spread/_config.js new file mode 100644 index 000000000000..de65bd662752 --- /dev/null +++ b/test/runtime-puppeteer/samples/component-event-handler-contenteditable-spread/_config.js @@ -0,0 +1,24 @@ +// A puppeteer test because JSDOM doesn't support contenteditable +export default { + html: '
', + ssrHtml: '
', + + 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'); + } +}; diff --git a/test/runtime-puppeteer/samples/component-event-handler-contenteditable-spread/main.svelte b/test/runtime-puppeteer/samples/component-event-handler-contenteditable-spread/main.svelte new file mode 100644 index 000000000000..ed301e8d2037 --- /dev/null +++ b/test/runtime-puppeteer/samples/component-event-handler-contenteditable-spread/main.svelte @@ -0,0 +1,11 @@ + + +
{text}
diff --git a/test/runtime-puppeteer/samples/component-event-handler-contenteditable/_config.js b/test/runtime-puppeteer/samples/component-event-handler-contenteditable/_config.js new file mode 100644 index 000000000000..1b4b3b0271e8 --- /dev/null +++ b/test/runtime-puppeteer/samples/component-event-handler-contenteditable/_config.js @@ -0,0 +1,33 @@ +// A puppeteer test because JSDOM doesn't support contenteditable +export default { + html: '
', + + // 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
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'); + } +}; diff --git a/test/runtime/samples/component-event-handler-contenteditable/main.svelte b/test/runtime-puppeteer/samples/component-event-handler-contenteditable/main.svelte similarity index 100% rename from test/runtime/samples/component-event-handler-contenteditable/main.svelte rename to test/runtime-puppeteer/samples/component-event-handler-contenteditable/main.svelte diff --git a/test/runtime/samples/component-event-handler-contenteditable/_config.js b/test/runtime/samples/component-event-handler-contenteditable/_config.js deleted file mode 100644 index 1628e22d017b..000000000000 --- a/test/runtime/samples/component-event-handler-contenteditable/_config.js +++ /dev/null @@ -1,15 +0,0 @@ -export default { - html: ` -
- `, - - async test({ assert, target, window }) { - 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'); - } -}; diff --git a/test/runtime/samples/reactive-values-text-node/_config.js b/test/runtime/samples/reactive-values-text-node/_config.js new file mode 100644 index 000000000000..fb859285ddda --- /dev/null +++ b/test/runtime/samples/reactive-values-text-node/_config.js @@ -0,0 +1,9 @@ +export default { + html:'
same text
', + async test({ assert, target }) { + await new Promise(f => setTimeout(f, 10)); + assert.htmlEqual(target.innerHTML, ` +
same text text
+ `); + } +}; diff --git a/test/runtime/samples/reactive-values-text-node/main.svelte b/test/runtime/samples/reactive-values-text-node/main.svelte new file mode 100644 index 000000000000..0982622b1a91 --- /dev/null +++ b/test/runtime/samples/reactive-values-text-node/main.svelte @@ -0,0 +1,8 @@ + + +
{text} text