From 6eb1742021bf00b29f94245f76f9852aca5ab7ed Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 8 Jan 2024 17:21:42 +0000 Subject: [PATCH 1/9] fix: improve animation heuristics better fix better fix --- packages/svelte/src/internal/client/each.js | 27 +++++++++------------ 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/packages/svelte/src/internal/client/each.js b/packages/svelte/src/internal/client/each.js index c51af7857212..ba0bd789db96 100644 --- a/packages/svelte/src/internal/client/each.js +++ b/packages/svelte/src/internal/client/each.js @@ -25,7 +25,6 @@ import { mutable_source, push_destroy_fn, render_effect, - schedule_task, set_signal_value, source } from './runtime.js'; @@ -486,6 +485,17 @@ function reconcile_tracked_array( key = is_computed_key ? keys[a] : item; map_set(item_index, key, a); } + // If keys are animated, we need to do updates before actual moves + if (is_animated) { + for (b = start; b <= a_end; ++b) { + a = map_get(item_index, /** @type {V} */ (a_blocks[b].k)); + if (a !== undefined) { + item = array[a]; + block = a_blocks[b]; + update_each_item_block(block, item, a, flags); + } + } + } for (b = start; b <= a_end; ++b) { a = map_get(item_index, /** @type {V} */ (a_blocks[b].k)); block = a_blocks[b]; @@ -501,22 +511,9 @@ function reconcile_tracked_array( if (pos === MOVED_BLOCK) { mark_lis(sources); } - // If keys are animated, we need to do updates before actual moves - var should_create; - if (is_animated) { - var i = b_length; - while (i-- > 0) { - b_end = i + start; - a = sources[i]; - if (pos === MOVED_BLOCK) { - block = b_blocks[b_end]; - item = array[b_end]; - update_each_item_block(block, item, b_end, flags); - } - } - } var last_block; var last_sibling; + var should_create; while (b_length-- > 0) { b_end = b_length + start; a = sources[b_length]; From 316e98674f594994c044b403d1983cd657a8dbbe Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 9 Jan 2024 12:57:42 +0000 Subject: [PATCH 2/9] improve-animation --- .changeset/soft-tigers-wink.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/soft-tigers-wink.md diff --git a/.changeset/soft-tigers-wink.md b/.changeset/soft-tigers-wink.md new file mode 100644 index 000000000000..561fbb69a6a8 --- /dev/null +++ b/.changeset/soft-tigers-wink.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: improve animation transition heuristics From 32e6dca0d7230bcf8fb34690a69d8d4582bc45ab Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 9 Jan 2024 13:49:09 +0000 Subject: [PATCH 3/9] more fixes --- packages/svelte/src/internal/client/runtime.js | 10 +++++----- packages/svelte/src/internal/client/transitions.js | 3 +-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 8631529e62a0..90043bdb9078 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -585,11 +585,6 @@ function flush_queued_effects(effects) { function process_microtask() { is_micro_task_queued = false; - if (current_queued_microtasks.length > 0) { - const tasks = current_queued_microtasks.slice(); - current_queued_microtasks = []; - run_all(tasks); - } if (flush_count > 101) { return; } @@ -602,6 +597,11 @@ function process_microtask() { if (!is_micro_task_queued) { flush_count = 0; } + if (current_queued_microtasks.length > 0) { + const tasks = current_queued_microtasks.slice(); + current_queued_microtasks = []; + run_all(tasks); + } } /** diff --git a/packages/svelte/src/internal/client/transitions.js b/packages/svelte/src/internal/client/transitions.js index 3317520e6d13..d1bb511cb7d4 100644 --- a/packages/svelte/src/internal/client/transitions.js +++ b/packages/svelte/src/internal/client/transitions.js @@ -672,8 +672,7 @@ function each_item_animate(block, transitions, index, index_is_reactive) { if (index_is_reactive) { prev_index = /** @type {import('./types.js').Signal} */ (prev_index).v; } - const items = block.p.v; - if (prev_index !== index && /** @type {number} */ (index) < items.length) { + if (prev_index !== index) { const from_dom = /** @type {Element} */ (get_first_element(block)); const from = from_dom.getBoundingClientRect(); // Cancel any existing key transitions From f75b46b1813839df13ec1815b9c2e2c32f7dad96 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 9 Jan 2024 14:45:13 +0000 Subject: [PATCH 4/9] use rAF --- .../svelte/src/internal/client/runtime.js | 29 ++++++++++--------- .../svelte/src/internal/client/transitions.js | 11 ++++--- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 90043bdb9078..a483999ec83d 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -37,6 +37,7 @@ let current_scheduler_mode = FLUSH_MICROTASK; // Used for handling scheduling let is_micro_task_queued = false; let is_task_queued = false; +let is_frame_queued = false; // Used for $inspect export let is_batching_effect = false; @@ -51,7 +52,7 @@ let current_queued_effects = []; /** @type {Array<() => void>} */ let current_queued_tasks = []; /** @type {Array<() => void>} */ -let current_queued_microtasks = []; +let current_queued_frames = []; let flush_count = 0; // Handle signal reactivity tree dependencies and consumer @@ -597,11 +598,6 @@ function process_microtask() { if (!is_micro_task_queued) { flush_count = 0; } - if (current_queued_microtasks.length > 0) { - const tasks = current_queued_microtasks.slice(); - current_queued_microtasks = []; - run_all(tasks); - } } /** @@ -636,6 +632,13 @@ function process_task() { run_all(tasks); } +function process_frames() { + is_frame_queued = false; + const frames = current_queued_frames.slice(); + current_queued_frames = []; + run_all(frames); +} + /** * @param {() => void} fn * @returns {void} @@ -652,12 +655,12 @@ export function schedule_task(fn) { * @param {() => void} fn * @returns {void} */ -export function schedule_microtask(fn) { - if (!is_micro_task_queued) { - is_micro_task_queued = true; - queueMicrotask(process_microtask); +export function schedule_frame(fn) { + if (!is_frame_queued) { + is_frame_queued = true; + requestAnimationFrame(process_frames); } - current_queued_microtasks.push(fn); + current_queued_frames.push(fn); } /** @@ -720,8 +723,8 @@ export function flushSync(fn) { if (current_queued_pre_and_render_effects.length > 0 || effects.length > 0) { flushSync(); } - if (is_micro_task_queued) { - process_microtask(); + if (is_frame_queued) { + process_frames(); } if (is_task_queued) { process_task(); diff --git a/packages/svelte/src/internal/client/transitions.js b/packages/svelte/src/internal/client/transitions.js index d1bb511cb7d4..5a426d314303 100644 --- a/packages/svelte/src/internal/client/transitions.js +++ b/packages/svelte/src/internal/client/transitions.js @@ -21,7 +21,7 @@ import { managed_effect, managed_pre_effect, mark_subtree_inert, - schedule_microtask, + schedule_frame, untrack } from './runtime.js'; import { raf } from './timing.js'; @@ -646,7 +646,8 @@ function each_item_transition(transition) { transitions.delete(transition); if (transition.r !== 'key') { for (let other of transitions) { - if (other.r === 'key' || other.r === 'in') { + const type = other.r; + if (type === 'key' || type === 'in') { transitions.delete(other); } } @@ -675,13 +676,15 @@ function each_item_animate(block, transitions, index, index_is_reactive) { if (prev_index !== index) { const from_dom = /** @type {Element} */ (get_first_element(block)); const from = from_dom.getBoundingClientRect(); + let deferred = false; // Cancel any existing key transitions for (const transition of transitions) { - if (transition.r === 'key') { + const type = transition.r; + if (type === 'key') { transition.c(); } } - schedule_microtask(() => { + schedule_frame(() => { trigger_transitions(transitions, 'key', from); }); } From e3dd5eb69ceef4c7c4a4a8df43f01e35825546d1 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 9 Jan 2024 15:05:12 +0000 Subject: [PATCH 5/9] feedback --- .../svelte/src/internal/client/runtime.js | 28 +++++++++---------- .../svelte/src/internal/client/transitions.js | 4 +-- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index a483999ec83d..df802c94b29f 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -37,7 +37,7 @@ let current_scheduler_mode = FLUSH_MICROTASK; // Used for handling scheduling let is_micro_task_queued = false; let is_task_queued = false; -let is_frame_queued = false; +let is_raf_queued = false; // Used for $inspect export let is_batching_effect = false; @@ -52,7 +52,7 @@ let current_queued_effects = []; /** @type {Array<() => void>} */ let current_queued_tasks = []; /** @type {Array<() => void>} */ -let current_queued_frames = []; +let current_raf_tasks = []; let flush_count = 0; // Handle signal reactivity tree dependencies and consumer @@ -632,11 +632,11 @@ function process_task() { run_all(tasks); } -function process_frames() { - is_frame_queued = false; - const frames = current_queued_frames.slice(); - current_queued_frames = []; - run_all(frames); +function process_raf_task() { + is_raf_queued = false; + const tasks = current_raf_tasks.slice(); + current_raf_tasks = []; + run_all(tasks); } /** @@ -655,12 +655,12 @@ export function schedule_task(fn) { * @param {() => void} fn * @returns {void} */ -export function schedule_frame(fn) { - if (!is_frame_queued) { - is_frame_queued = true; - requestAnimationFrame(process_frames); +export function schedule_raf_task(fn) { + if (!is_raf_queued) { + is_raf_queued = true; + requestAnimationFrame(process_raf_task); } - current_queued_frames.push(fn); + current_raf_tasks.push(fn); } /** @@ -723,8 +723,8 @@ export function flushSync(fn) { if (current_queued_pre_and_render_effects.length > 0 || effects.length > 0) { flushSync(); } - if (is_frame_queued) { - process_frames(); + if (is_raf_queued) { + process_raf_task(); } if (is_task_queued) { process_task(); diff --git a/packages/svelte/src/internal/client/transitions.js b/packages/svelte/src/internal/client/transitions.js index 5a426d314303..cdafc0e19018 100644 --- a/packages/svelte/src/internal/client/transitions.js +++ b/packages/svelte/src/internal/client/transitions.js @@ -21,7 +21,7 @@ import { managed_effect, managed_pre_effect, mark_subtree_inert, - schedule_frame, + schedule_raf_task, untrack } from './runtime.js'; import { raf } from './timing.js'; @@ -684,7 +684,7 @@ function each_item_animate(block, transitions, index, index_is_reactive) { transition.c(); } } - schedule_frame(() => { + schedule_raf_task(() => { trigger_transitions(transitions, 'key', from); }); } From 95aa3ac10a4a27eeaeda222c8837f5532a472b4a Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 9 Jan 2024 17:27:00 +0000 Subject: [PATCH 6/9] fix absolute positioning --- .../svelte/src/internal/client/transitions.js | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/packages/svelte/src/internal/client/transitions.js b/packages/svelte/src/internal/client/transitions.js index cdafc0e19018..2fd8f8ea6fe7 100644 --- a/packages/svelte/src/internal/client/transitions.js +++ b/packages/svelte/src/internal/client/transitions.js @@ -369,6 +369,22 @@ function create_transition(dom, init, direction, effect) { }, // out o() { + // @ts-ignore + const has_keyed_transition = dom.__animate; + + // If we're outroing an element that has an animation, then we need to fix + // its position to ensure it behaves nicely without causing layout shift. + if (has_keyed_transition) { + const style = getComputedStyle(dom); + const position = style.position; + + if (position !== 'absolute' && position !== 'fixed') { + const { width, height } = style; + dom.style.position = 'absolute'; + dom.style.width = width; + dom.style.height = height; + } + } const needs_reverse = direction === 'both' && curr_direction !== 'out'; curr_direction = 'out'; if (animation === null || cancelled) { @@ -432,10 +448,16 @@ function is_transition_block(block) { export function bind_transition(dom, get_transition_fn, props_fn, direction, global) { const transition_effect = /** @type {import('./types.js').EffectSignal} */ (current_effect); const block = current_block; + const is_keyed_transition = direction === 'key'; let can_show_intro_on_mount = true; let can_apply_lazy_transitions = false; + if (is_keyed_transition) { + // @ts-ignore + dom.__animate = true; + } + /** @type {import('./types.js').Block | null} */ let transition_block = block; while (transition_block !== null) { @@ -479,7 +501,7 @@ export function bind_transition(dom, get_transition_fn, props_fn, direction, glo const init = (from) => untrack(() => { const props = props_fn === null ? {} : props_fn(); - return direction === 'key' + return is_keyed_transition ? /** @type {import('./types.js').AnimateFn} */ (transition_fn)( dom, { from: /** @type {DOMRect} */ (from), to: dom.getBoundingClientRect() }, @@ -676,7 +698,6 @@ function each_item_animate(block, transitions, index, index_is_reactive) { if (prev_index !== index) { const from_dom = /** @type {Element} */ (get_first_element(block)); const from = from_dom.getBoundingClientRect(); - let deferred = false; // Cancel any existing key transitions for (const transition of transitions) { const type = transition.r; From 73542facffa9408251dcbeb08a718396a3bdf6f4 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 9 Jan 2024 20:23:31 +0000 Subject: [PATCH 7/9] fix more --- .../svelte/src/internal/client/transitions.js | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/packages/svelte/src/internal/client/transitions.js b/packages/svelte/src/internal/client/transitions.js index 2fd8f8ea6fe7..fbd99883ebad 100644 --- a/packages/svelte/src/internal/client/transitions.js +++ b/packages/svelte/src/internal/client/transitions.js @@ -372,19 +372,6 @@ function create_transition(dom, init, direction, effect) { // @ts-ignore const has_keyed_transition = dom.__animate; - // If we're outroing an element that has an animation, then we need to fix - // its position to ensure it behaves nicely without causing layout shift. - if (has_keyed_transition) { - const style = getComputedStyle(dom); - const position = style.position; - - if (position !== 'absolute' && position !== 'fixed') { - const { width, height } = style; - dom.style.position = 'absolute'; - dom.style.width = width; - dom.style.height = height; - } - } const needs_reverse = direction === 'both' && curr_direction !== 'out'; curr_direction = 'out'; if (animation === null || cancelled) { @@ -401,6 +388,28 @@ function create_transition(dom, init, direction, effect) { /** @type {Animation | TickAnimation} */ (animation).play(); } } + // If we're outroing an element that has an animation, then we need to fix + // its position to ensure it behaves nicely without causing layout shift. + if (has_keyed_transition) { + const style = getComputedStyle(dom); + const position = style.position; + + if (position !== 'absolute' && position !== 'fixed') { + const { width, height } = style; + const a = dom.getBoundingClientRect(); + dom.style.position = 'absolute'; + dom.style.width = width; + dom.style.height = height; + const b = dom.getBoundingClientRect(); + if (a.left !== b.left || a.top !== b.top) { + const style = getComputedStyle(dom); + const transform = style.transform === 'none' ? '' : style.transform; + dom.style.transform = `${transform} translate(${a.left - b.left}px, ${ + a.top - b.top + }px)`; + } + } + } }, // cancel c() { From 877a996e34e6c564f77c3e726cfc63e51a7ab3aa Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 9 Jan 2024 21:00:43 +0000 Subject: [PATCH 8/9] revert --- .../svelte/src/internal/client/transitions.js | 33 +------------------ 1 file changed, 1 insertion(+), 32 deletions(-) diff --git a/packages/svelte/src/internal/client/transitions.js b/packages/svelte/src/internal/client/transitions.js index fbd99883ebad..06aa521a69fb 100644 --- a/packages/svelte/src/internal/client/transitions.js +++ b/packages/svelte/src/internal/client/transitions.js @@ -369,9 +369,6 @@ function create_transition(dom, init, direction, effect) { }, // out o() { - // @ts-ignore - const has_keyed_transition = dom.__animate; - const needs_reverse = direction === 'both' && curr_direction !== 'out'; curr_direction = 'out'; if (animation === null || cancelled) { @@ -388,28 +385,6 @@ function create_transition(dom, init, direction, effect) { /** @type {Animation | TickAnimation} */ (animation).play(); } } - // If we're outroing an element that has an animation, then we need to fix - // its position to ensure it behaves nicely without causing layout shift. - if (has_keyed_transition) { - const style = getComputedStyle(dom); - const position = style.position; - - if (position !== 'absolute' && position !== 'fixed') { - const { width, height } = style; - const a = dom.getBoundingClientRect(); - dom.style.position = 'absolute'; - dom.style.width = width; - dom.style.height = height; - const b = dom.getBoundingClientRect(); - if (a.left !== b.left || a.top !== b.top) { - const style = getComputedStyle(dom); - const transform = style.transform === 'none' ? '' : style.transform; - dom.style.transform = `${transform} translate(${a.left - b.left}px, ${ - a.top - b.top - }px)`; - } - } - } }, // cancel c() { @@ -457,16 +432,10 @@ function is_transition_block(block) { export function bind_transition(dom, get_transition_fn, props_fn, direction, global) { const transition_effect = /** @type {import('./types.js').EffectSignal} */ (current_effect); const block = current_block; - const is_keyed_transition = direction === 'key'; let can_show_intro_on_mount = true; let can_apply_lazy_transitions = false; - if (is_keyed_transition) { - // @ts-ignore - dom.__animate = true; - } - /** @type {import('./types.js').Block | null} */ let transition_block = block; while (transition_block !== null) { @@ -510,7 +479,7 @@ export function bind_transition(dom, get_transition_fn, props_fn, direction, glo const init = (from) => untrack(() => { const props = props_fn === null ? {} : props_fn(); - return is_keyed_transition + return direction === 'key' ? /** @type {import('./types.js').AnimateFn} */ (transition_fn)( dom, { from: /** @type {DOMRect} */ (from), to: dom.getBoundingClientRect() }, From b04c5c7dcc03b900966cc07282cd95cef08a5970 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 9 Jan 2024 21:45:18 +0000 Subject: [PATCH 9/9] more fixes --- packages/svelte/src/internal/client/each.js | 2 +- .../svelte/src/internal/client/transitions.js | 30 +++++++------------ .../svelte/src/internal/client/types.d.ts | 9 +----- .../dynamic-element-animation/_config.js | 6 ++-- 4 files changed, 16 insertions(+), 31 deletions(-) diff --git a/packages/svelte/src/internal/client/each.js b/packages/svelte/src/internal/client/each.js index ba0bd789db96..e66574fba0e7 100644 --- a/packages/svelte/src/internal/client/each.js +++ b/packages/svelte/src/internal/client/each.js @@ -712,7 +712,7 @@ function update_each_item_block(block, item, index, type) { // Handle each item animations const each_animation = block.a; if (transitions !== null && (type & EACH_KEYED) !== 0 && each_animation !== null) { - each_animation(block, transitions, index, index_is_reactive); + each_animation(block, transitions); } if (index_is_reactive) { set_signal_value(/** @type {import('./types.js').Signal} */ (block.i), index); diff --git a/packages/svelte/src/internal/client/transitions.js b/packages/svelte/src/internal/client/transitions.js index 06aa521a69fb..6ce0f2f40345 100644 --- a/packages/svelte/src/internal/client/transitions.js +++ b/packages/svelte/src/internal/client/transitions.js @@ -665,26 +665,18 @@ function each_item_transition(transition) { * * @param {import('./types.js').EachItemBlock} block * @param {Set} transitions - * @param {number} index - * @param {boolean} index_is_reactive */ -function each_item_animate(block, transitions, index, index_is_reactive) { - let prev_index = block.i; - if (index_is_reactive) { - prev_index = /** @type {import('./types.js').Signal} */ (prev_index).v; - } - if (prev_index !== index) { - const from_dom = /** @type {Element} */ (get_first_element(block)); - const from = from_dom.getBoundingClientRect(); - // Cancel any existing key transitions - for (const transition of transitions) { - const type = transition.r; - if (type === 'key') { - transition.c(); - } +function each_item_animate(block, transitions) { + const from_dom = /** @type {Element} */ (get_first_element(block)); + const from = from_dom.getBoundingClientRect(); + // Cancel any existing key transitions + for (const transition of transitions) { + const type = transition.r; + if (type === 'key') { + transition.c(); } - schedule_raf_task(() => { - trigger_transitions(transitions, 'key', from); - }); } + schedule_raf_task(() => { + trigger_transitions(transitions, 'key', from); + }); } diff --git a/packages/svelte/src/internal/client/types.d.ts b/packages/svelte/src/internal/client/types.d.ts index af21817c69f2..31dd4f1162d1 100644 --- a/packages/svelte/src/internal/client/types.d.ts +++ b/packages/svelte/src/internal/client/types.d.ts @@ -288,14 +288,7 @@ export type EachBlock = { export type EachItemBlock = { /** transition */ - a: - | null - | (( - block: EachItemBlock, - transitions: Set, - index: number, - index_is_reactive: boolean - ) => void); + a: null | ((block: EachItemBlock, transitions: Set) => void); /** dom */ d: null | TemplateNode | Array; /** effect */ diff --git a/packages/svelte/tests/runtime-legacy/samples/dynamic-element-animation/_config.js b/packages/svelte/tests/runtime-legacy/samples/dynamic-element-animation/_config.js index 5a6f0818fbaf..6315a32d861d 100644 --- a/packages/svelte/tests/runtime-legacy/samples/dynamic-element-animation/_config.js +++ b/packages/svelte/tests/runtime-legacy/samples/dynamic-element-animation/_config.js @@ -53,9 +53,9 @@ export default test({ divs = target.querySelectorAll('div'); assert.ok(~divs[0].style.transform); - assert.equal(divs[1].style.transform, ''); - assert.equal(divs[2].style.transform, ''); - assert.equal(divs[3].style.transform, ''); + assert.equal(divs[1].style.transform, 'translate(1px, 0px)'); + assert.equal(divs[2].style.transform, 'translate(1px, 0px)'); + assert.equal(divs[3].style.transform, 'translate(1px, 0px)'); assert.ok(~divs[4].style.transform); raf.tick(100);