From 7578af3a112ba0ad5695a9f5750f81e8a9c8b8a5 Mon Sep 17 00:00:00 2001 From: gtmnayan <50981692+gtm-nayan@users.noreply.github.com> Date: Tue, 14 Mar 2023 16:43:04 +0545 Subject: [PATCH] fix: retain style directive value after style attribute is updated (#7610) fixes #7475 --- .../render_dom/wrappers/Element/index.ts | 61 ++++++++++++++----- .../_config.js | 24 ++++++++ .../main.svelte | 15 +++++ .../_config.js | 1 - 4 files changed, 85 insertions(+), 16 deletions(-) create mode 100644 test/runtime-puppeteer/samples/inline-style-directive-precedence/_config.js create mode 100644 test/runtime-puppeteer/samples/inline-style-directive-precedence/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 eaef3a3b69..0af012777a 100644 --- a/src/compiler/compile/render_dom/wrappers/Element/index.ts +++ b/src/compiler/compile/render_dom/wrappers/Element/index.ts @@ -160,6 +160,7 @@ export default class ElementWrapper extends Wrapper { bindings: Binding[]; event_handlers: EventHandler[]; class_dependencies: string[]; + dynamic_style_dependencies: Set; has_dynamic_attribute: boolean; select_binding_dependencies?: Set; @@ -214,6 +215,8 @@ export default class ElementWrapper extends Wrapper { return; } + this.dynamic_style_dependencies = new Set(); + if (this.node.children.length) { this.node.lets.forEach(l => { extract_names(l.value || l.name).forEach(name => { @@ -801,11 +804,13 @@ export default class ElementWrapper extends Wrapper { } add_attributes(block: Block) { - // Get all the class dependencies first + // Get all the class and style dependencies first this.attributes.forEach((attribute) => { if (attribute.node.name === 'class') { const dependencies = attribute.node.get_dependencies(); push_array(this.class_dependencies, dependencies); + } else if (attribute.node.name === 'style') { + add_to_set(this.dynamic_style_dependencies, attribute.node.get_dependencies()); } }); @@ -1170,8 +1175,18 @@ export default class ElementWrapper extends Wrapper { add_styles(block: Block) { const has_spread = this.node.attributes.some(attr => attr.is_spread); + + let style_changed_var: Identifier | undefined; + const maybe_create_style_changed_var = () => { + if (!style_changed_var && this.dynamic_style_dependencies.size) { + style_changed_var = block.get_unique_name('style_changed'); + const style_attr_dirty = block.renderer.dirty([...this.dynamic_style_dependencies]); + block.chunks.update.push(b`const ${style_changed_var} = ${style_attr_dirty};`); + } + }; + this.node.styles.forEach((style_directive) => { - const { name, expression, should_cache, important } = style_directive; + const { name, expression, important, should_cache } = style_directive; const snippet = expression.manipulate(block); let cached_snippet: Identifier | undefined; @@ -1184,24 +1199,40 @@ export default class ElementWrapper extends Wrapper { block.chunks.hydrate.push(updater); - const dependencies = expression.dynamic_dependencies(); + // Assume that style has changed through the spread attribute if (has_spread) { block.chunks.update.push(updater); - } else if (dependencies.length > 0) { + } else { + const self_deps = expression.dynamic_dependencies(); + const all_deps = new Set([ + ...self_deps, + ...this.dynamic_style_dependencies + ]); + + if (all_deps.size === 0) return; + + let condition = block.renderer.dirty([...all_deps]); + if (should_cache) { - block.chunks.update.push(b` - if (${block.renderer.dirty(dependencies)} && (${cached_snippet} !== (${cached_snippet} = ${snippet}))) { - ${updater} - } - `); - } else { - block.chunks.update.push(b` - if (${block.renderer.dirty(dependencies)}) { - ${updater} - } - `); + condition = x`${condition} && ${cached_snippet} !== (${cached_snippet} = ${snippet})`; } + + 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 + ? style_changed_var + : x`${style_changed_var} || ${condition}`; + } + + block.chunks.update.push(b` + if (${condition}) { + ${updater} + } + `); } + }); } diff --git a/test/runtime-puppeteer/samples/inline-style-directive-precedence/_config.js b/test/runtime-puppeteer/samples/inline-style-directive-precedence/_config.js new file mode 100644 index 0000000000..a684f3e0d7 --- /dev/null +++ b/test/runtime-puppeteer/samples/inline-style-directive-precedence/_config.js @@ -0,0 +1,24 @@ +export default { + html: ` +

+ `, + + test({ assert, target, window, component }) { + const p = target.querySelector('p'); + const styles = window.getComputedStyle(p); + assert.equal(styles.color, 'rgb(255, 0, 0)'); + assert.equal(styles.fontSize, '32px'); + assert.equal(styles.backgroundColor, 'rgb(0, 128, 0)'); + assert.equal(styles.borderColor, 'rgb(0, 128, 0)'); + + component.foo = 'font-size: 50px; color: green;'; // Update style attribute + { + const p = target.querySelector('p'); + const styles = window.getComputedStyle(p); + assert.equal(styles.color, 'rgb(255, 0, 0)'); + assert.equal(styles.fontSize, '32px'); + assert.equal(styles.backgroundColor, 'rgb(0, 128, 0)'); + assert.equal(styles.borderColor, 'rgb(0, 128, 0)'); + } + } +}; diff --git a/test/runtime-puppeteer/samples/inline-style-directive-precedence/main.svelte b/test/runtime-puppeteer/samples/inline-style-directive-precedence/main.svelte new file mode 100644 index 0000000000..3e8b721a42 --- /dev/null +++ b/test/runtime-puppeteer/samples/inline-style-directive-precedence/main.svelte @@ -0,0 +1,15 @@ + + +

diff --git a/test/runtime/samples/inline-style-directive-and-style-attr-merged/_config.js b/test/runtime/samples/inline-style-directive-and-style-attr-merged/_config.js index f5780daf80..f2ab49b511 100644 --- a/test/runtime/samples/inline-style-directive-and-style-attr-merged/_config.js +++ b/test/runtime/samples/inline-style-directive-and-style-attr-merged/_config.js @@ -5,7 +5,6 @@ export default { test({ assert, target, window }) { const p = target.querySelector('p'); - const styles = window.getComputedStyle(p); assert.equal(styles.color, 'red'); assert.equal(styles.height, '40px');