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: use wholeText for only contenteditable for set_data #8394

Merged
merged 16 commits into from
Mar 21, 2023
Merged
Show file tree
Hide file tree
Changes from 10 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
37 changes: 20 additions & 17 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 @@ -868,13 +872,13 @@ export default class ElementWrapper extends Wrapper {
? x`@set_dynamic_element_data(${this.node.tag_expr.manipulate(block)})`
: x`@set_attributes`;

block.chunks.hydrate.push(
b`${fn}(${this.var}, ${data});`
block.chunks.hydrate.unshift(
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}, [
block.chunks.update.unshift(b`
Copy link
Member

@dummdidumm dummdidumm Mar 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to switch to unshift here and above? I switched it back to push and the tests all passed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, test case component-event-handler-contenteditable3, get_spread_update is added at ElementWrapper class, and set_data_maybe_contenteditable is added at MustacheTag class.
And MustacheTag is executed earlier than ElementWrapper.

If get_spread_update runs after set_data_maybe_contenteditable, set_data_maybe_contenteditable runs using div_data that is before updating.

To avoid this situation, I use unshift.
I think there are other ways to solve this issue, but I think this is reasonable and anyway, we will dynamically change the compiler in Svelte4, so we don't need to be particular about this.

set_attributes$(div$, div_data$ = get_spread_update$(div$_levels$, [dirty & /*spread*/ 2 && /*spread*/ ctx[1]]));
if (dirty & /*text*/ 1) set_data_maybe_contenteditable$(t$, /*text*/ ctx[0], div_data$['contenteditable']);

${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
30 changes: 27 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,32 @@ 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 is_contenteditable: Expression | undefined = undefined;
if (contenteditable_attributes.length > 0) {
const value = (contenteditable_attributes[0] as AttributeWrapper).get_value(block);
is_contenteditable = x`~@contenteditable_truthy_values.indexOf(${value})`;
} else if (spread_attributes.length > 0 && data.element_data_name) {
is_contenteditable = x`~@contenteditable_truthy_values.indexOf(${data.element_data_name}['contenteditable'])`;
}

const { init } = this.rename_this_method(
block,
value => x`@set_data(${this.var}, ${value})`
value => {
if (is_contenteditable) {
return x`@set_data_contenteditable(${this.var}, ${value}, ${is_contenteditable})`;
} 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');
}
}
12 changes: 9 additions & 3 deletions src/runtime/internal/dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,18 @@ 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 validate_each_argument(arg) {
Expand Down
11 changes: 9 additions & 2 deletions src/runtime/internal/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -581,9 +581,16 @@ 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_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
@@ -1,15 +1,12 @@
export default {
html: `
<div contenteditable=""></div>
`,
html: '<div contenteditable=""></div>',

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');
}
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export default {
html: '<div contenteditable="true"></div>',

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');
}
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<script>
let text = '';
const updater = (event) => {text = event.target.textContent}
</script>

<div contenteditable="true" on:input={updater}>{text}</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
export default {
html: '<div contenteditable="true"></div>',
ssrHtml: '<div contenteditable=""></div>',

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');
}
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<script>
let text = '';
const updater = (event) => {text = event.target.textContent}
$: spread = {
contenteditable: text !== undefined,
}
</script>

<div {...spread} on:input={updater}>{text}</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
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>
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>