From 2db0fd46ed569eeb786efe103005096550bd8c87 Mon Sep 17 00:00:00 2001 From: Alek Petuskey Date: Sun, 26 Jul 2026 17:18:01 -0700 Subject: [PATCH] Cascade mark-level animation over the chart policy instead of replacing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #329. `Animation.to_spec()` emits every field, and both the Python merge in `_attach_keys` and the client's `_resolvedAnimation` are plain dict spreads. So a mark-level `xy.animation(duration=90)` was a complete spec spread over the chart's, resetting `match`, `easing`, `enter`, `update`, and `interpolate` to their defaults. Chart-level `match="key"` silently became index matching — no error, no fallback string — and the `match='key' requires key=` guard stopped firing in that configuration because it reads the same clobbered dict. `animation()` now records the caller's exact argument names behind sentinel defaults, so "set to the default" is distinguishable from "not set": a mark that passes `match="index"` against a chart's `match="key"` still wins. `to_override_spec()` returns just those fields. Direct `Animation(...)` construction is public too and falls back to diffing the defaults, which is right except for that same explicit-default case. The cascade resolves once, in Python, over a defaults base: {**defaults, **chart_spec, **mark_override} and a trace that carries an override ships the complete resolved policy. Shipping the sparse override instead would break a mark-level animation on a chart with no chart-level one — the client would read `undefined` for duration and easing. Resolving here means the client's spread stays a no-op rather than a second merge needing its own copy of the defaults. A trace with no override still ships no animation dict, and a chart with neither ships none, which is what keeps an unanimated chart static. Verified with a 59-combination cascade matrix (6 chart specs x 10 mark overrides including bools and None), each replaying the client's exact spread and checking all 8 fields plus whether identity planes ship. Identity-plane shipping tracks the resolved `match`, so this composes with #330: key matching that starts working again also gets its planes back. Three tests asserted the old behavior as a literal and now assert the cascade. test_mark_animation_spec_clobbers_chart_level_fields, added in #330 to pin the bug, becomes the regression test for the fix. Verified: 2578 passed / 4 skipped; ruff check and format clean; ty at the 14-diagnostic baseline with an identical diagnostic set; public API verification OK. --- CHANGELOG.md | 12 ++++ docs/components/marks.md | 6 +- docs/styling/animations.md | 4 +- python/xy/components.py | 131 +++++++++++++++++++++++++++---------- spec/design/animation.md | 16 ++++- tests/test_animation.py | 85 ++++++++++++++++++------ 6 files changed, 196 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8bd8f114..db802e90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,18 @@ in the README). same validated style properties, so an explicit `style=` still wins and specs that don't use them are byte-identical. +### Fixed +- A mark-level `animation=xy.animation(...)` no longer resets the chart-level + policy fields it does not mention. It was a complete spec spread over the + chart's, so `xy.animation(duration=90)` on a mark silently reset `match`, + `easing`, `enter`, `update`, and `interpolate` to their defaults — turning + off a chart-level `match="key"` with no error and no fallback, and + suppressing the `match='key' requires key=` validation entirely. Mark + overrides now cascade field by field, and passing a field explicitly counts + as setting it even when the value equals the default. A trace that carries an + override ships the complete resolved policy, so the browser's merge is no + longer a second, defaults-clobbering one. + ### Changed - Stable animation `key=` identity planes are now retained and shipped only when the resolved animation spec can actually key-match. `match` defaults to diff --git a/docs/components/marks.md b/docs/components/marks.md index d5fa037f..11842006 100644 --- a/docs/components/marks.md +++ b/docs/components/marks.md @@ -125,8 +125,10 @@ def marks_layer_demo(): - Children draw in declaration order, so broad fills normally come before lines and points. - `key=` supplies stable row identity for keyed browser data transitions, and - mark-level `animation=` overrides or disables the chart's `xy.animation()` - policy. See [Animations and data transitions](/docs/xy/styling/animations/). + mark-level `animation=` cascades over the chart's `xy.animation()` policy + field by field — `xy.animation(duration=90)` on a mark changes only the + duration — or disables that mark with `animation=False`. See + [Animations and data transitions](/docs/xy/styling/animations/). Canvas and WebGL marks are not DOM elements. CSS selectors and Tailwind classes cannot paint their geometry; use `style=` or the mark's paint props. In diff --git a/docs/styling/animations.md b/docs/styling/animations.md index af1ca07c..84ee5c68 100644 --- a/docs/styling/animations.md +++ b/docs/styling/animations.md @@ -373,7 +373,9 @@ before rendering. For a replacement without stable identity, choose | `append` | An ordered time series retains old x values and adds a tail | | `index` | Row order itself is the identity | -Each mark may override the chart policy or opt out: +Each mark may override the chart policy or opt out. The override cascades field +by field, so a mark only changes what it names — `xy.animation(duration=90)` on +a mark keeps the chart's `match`, `easing`, and everything else: ~~~python demo exec toggle preview-code id=animation-mark-override-demo rows = { diff --git a/python/xy/components.py b/python/xy/components.py index e2e899ca..5d30848e 100644 --- a/python/xy/components.py +++ b/python/xy/components.py @@ -38,6 +38,7 @@ import warnings from collections.abc import Callable, Mapping, Sequence from dataclasses import dataclass, field +from functools import lru_cache from os import PathLike from typing import Any, Literal, Optional, TypeAlias, Union @@ -356,9 +357,23 @@ def to_spec(self) -> dict[str, float | str]: } +_UNSET: Any = object() + + @dataclass class Animation(Component): - """Declarative browser transition policy built by :func:`animation`.""" + """Declarative browser transition policy built by :func:`animation`. + + A mark-level instance cascades over the chart-level one field by field. + ``to_spec`` is the complete policy; ``to_override_spec`` is only what this + instance actually sets, which is what makes the cascade work. + """ + + # Set by :func:`animation` to the exact argument names the caller passed. + # ``None`` means "not recorded" — direct construction falls back to + # comparing against the defaults, which is right except for the odd case + # of explicitly passing a default value. + _explicit_fields = None enabled: bool | Literal["auto"] = "auto" delay: float = 0.0 @@ -427,6 +442,26 @@ def to_spec(self) -> dict[str, Any]: "interpolate": list(policies), } + def to_override_spec(self) -> dict[str, Any]: + """Only the fields this instance sets, for cascading over a base spec. + + `to_spec` is complete, so spreading it over a chart-level spec resets + every field the caller did not mention — silently turning off a + chart-level ``match="key"``, among others. + """ + spec = self.to_spec() + explicit = self._explicit_fields + if explicit is None: + defaults = _animation_defaults() + explicit = {name for name, value in spec.items() if value != defaults[name]} + return {name: value for name, value in spec.items() if name in explicit} + + +@lru_cache(maxsize=1) +def _animation_defaults() -> dict[str, Any]: + """The complete default policy, and the base every cascade starts from.""" + return Animation().to_spec() + def _positive_animation_number(value: Any, label: str) -> float: number = _finite_number(value, label) @@ -463,43 +498,65 @@ def spring(*, stiffness: float = 170.0, damping: float = 26.0, mass: float = 1.0 def animation( *, - enabled: bool | Literal["auto"] = "auto", - delay: float = 0, - duration: float = 400, - easing: str | tuple[float, float, float, float] | Spring = "ease-out", - match: Literal["index", "append", "key"] = "index", - enter: str = "auto", - update: str = "interpolate", - interpolate: Sequence[str] = ("position", "size", "color", "domain"), + enabled: bool | Literal["auto"] = _UNSET, + delay: float = _UNSET, + duration: float = _UNSET, + easing: str | tuple[float, float, float, float] | Spring = _UNSET, + match: Literal["index", "append", "key"] = _UNSET, + enter: str = _UNSET, + update: str = _UNSET, + interpolate: Sequence[str] = _UNSET, on_start: Optional[Callable[[dict], None]] = None, on_end: Optional[Callable[[dict], None]] = None, ) -> Animation: """Configure entrance and data-update motion without per-frame callbacks. + A mark-level `animation=` cascades over the chart-level one: only the + arguments passed here override it, so `xy.animation(duration=90)` on a mark + keeps the chart's `match`, `easing`, and the rest. + Args: - enabled: ``"auto"`` honors reduced motion; a boolean explicitly enables or disables. - delay: Non-negative delay before motion begins, in milliseconds. - duration: Non-negative animation duration, in milliseconds. + enabled: ``"auto"`` (default) honors reduced motion; a boolean explicitly + enables or disables. + delay: Non-negative delay before motion begins, in milliseconds. Default ``0``. + duration: Non-negative animation duration, in milliseconds. Default ``400``. easing: Named easing, four-number cubic Bézier tuple, or ``spring()`` policy. - match: Row identity strategy: ``"index"``, ``"append"``, or ``"key"``. - enter: Entrance effect, such as ``"auto"``, ``"scale"``, or ``"reveal"``. - update: Update effect; use ``"interpolate"`` or ``"none"``. - interpolate: Unique channels to interpolate during updates. + Default ``"ease-out"``. + match: Row identity strategy: ``"index"`` (default), ``"append"``, or ``"key"``. + enter: Entrance effect, such as ``"auto"`` (default), ``"scale"``, or ``"reveal"``. + update: Update effect; use ``"interpolate"`` (default) or ``"none"``. + interpolate: Unique channels to interpolate during updates. Default + ``("position", "size", "color", "domain")``. on_start: Optional live-host callback receiving the animation-start event. on_end: Optional live-host callback receiving the animation-end event. """ - value = Animation( - enabled=enabled, - delay=delay, - duration=duration, - easing=easing, - match=match, - enter=enter, - update=update, - interpolate=tuple(interpolate), - on_start=on_start, - on_end=on_end, - ) + if interpolate is not _UNSET: + try: + interpolate = tuple(interpolate) + except TypeError as exc: + raise ValueError("animation interpolate must be a sequence of named policies") from exc + passed = { + name: value + for name, value in ( + ("enabled", enabled), + ("delay", delay), + ("duration", duration), + ("easing", easing), + ("match", match), + ("enter", enter), + ("update", update), + ("interpolate", interpolate), + ) + if value is not _UNSET + } + # Start from the dataclass defaults and apply only what was passed, so the + # default values live in exactly one place. + value = Animation(on_start=on_start, on_end=on_end) + for name, setting in passed.items(): + setattr(value, name, setting) + # Record the caller's exact argument names so the cascade can tell "set to + # the default" apart from "not set". Callbacks are host-side, not spec. + value._explicit_fields = frozenset(passed) value.to_spec() return value @@ -4054,14 +4111,19 @@ def _apply_mark_transition_metadata( ) -> None: override = mark.animation if override is None: - mark_spec = None + mark_override = None elif isinstance(override, bool): - mark_spec = {"enabled": override} + mark_override = {"enabled": override} elif isinstance(override, Animation): - mark_spec = override.to_spec() + mark_override = override.to_override_spec() else: raise ValueError(f"{mark.kind} animation must be xy.animation(...), bool, or None") - effective = {**(chart_spec or {}), **(mark_spec or {})} + # Cascade, do not replace. The mark contributes only the fields it set; + # defaults sit underneath so a mark-level spec with no chart-level one is + # still complete. Resolving here once is also what lets the client keep a + # plain spread — an incomplete `trace.animation` would leave it reading + # `undefined` for duration and easing. + effective = {**_animation_defaults(), **(chart_spec or {}), **(mark_override or {})} if ( effective.get("enabled") is not False and effective.get("match") == "key" @@ -4092,7 +4154,10 @@ def _apply_mark_transition_metadata( if positions is not None: keys = keys[np.argsort(positions, kind="stable")] for trace in traces: - trace.animation = None if mark_spec is None else dict(mark_spec) + # Ship the resolved policy, not the raw override: the client spreads + # this over the chart-level spec, and a sparse dict there would silently + # fall back to chart values the mark meant to change. + trace.animation = None if mark_override is None else dict(effective) if keys is not None: # The per-trace row check is contract too, so it runs whether or # not the planes are kept. diff --git a/spec/design/animation.md b/spec/design/animation.md index cd858d1d..cfc6f37c 100644 --- a/spec/design/animation.md +++ b/spec/design/animation.md @@ -7,8 +7,20 @@ motion-free by definition. ## 1. Public grammar Animation is a normal chart child. The last chart-level `Animation` child is -the default policy; a mark-level `animation=` value overrides it. `False` -disables that mark without disabling its siblings. +the default policy; a mark-level `animation=` value cascades over it **field by +field**, contributing only the arguments that were actually passed. So +`xy.animation(duration=90)` on a mark keeps the chart's `match`, `easing`, and +the rest — a whole-spec replacement would silently disable a chart-level +`match="key"`. Passing a field explicitly counts as setting it even when the +value equals the default. `False` disables that mark without disabling its +siblings. + +The resolution happens once, in Python: a trace that carries an override ships +the *complete* resolved policy, so the client's spread over the chart-level +spec is a no-op rather than a second merge that would need its own copy of the +defaults. A trace with no override ships no animation dict at all, and a chart +with neither ships none either — which is what keeps a chart without +`xy.animation()` static. ```python xy.chart( diff --git a/tests/test_animation.py b/tests/test_animation.py index 12b4c8dc..d63232c4 100644 --- a/tests/test_animation.py +++ b/tests/test_animation.py @@ -517,14 +517,12 @@ def test_key_matching_payload_is_unchanged_by_the_skip() -> None: np.testing.assert_array_equal(hi, expected[:, 1]) -def test_mark_animation_spec_clobbers_chart_level_fields() -> None: - """KNOWN BUG (reflex-dev/xy#329) — pinned so a fix is a deliberate change. - - `Animation.to_spec()` emits every field, and both the Python merge and the - client's `{...spec.animation, ...trace.animation}` are plain dict spreads. - So a mark-level `xy.animation(duration=90)` resets `match`, `easing`, - `enter`, `update`, and `interpolate` to their defaults, silently turning - off the chart-level `match="key"` the caller asked for. +def test_mark_animation_cascades_over_chart_level_fields() -> None: + """Regression for reflex-dev/xy#329. + + A mark-level spec used to be a complete dict spread over the chart-level + one, so setting any field reset every other field to its default — + silently turning off a chart-level ``match="key"``. """ figure = xy.scatter_chart( xy.scatter( @@ -536,17 +534,64 @@ def test_mark_animation_spec_clobbers_chart_level_fields() -> None: xy.animation(match="key", easing="linear"), ).figure() spec, _ = figure.build_payload_split() + # Exactly the merge the client performs in `_resolvedAnimation`. resolved = {**spec.get("animation", {}), **(spec["traces"][0].get("animation") or {})} - assert resolved["match"] == "index" # caller asked for "key" - assert resolved["easing"] == "ease-out" # caller asked for "linear" - assert resolved["duration"] == 90.0 # the one field they meant to set + assert resolved["duration"] == 90.0 # the mark's one override + assert resolved["match"] == "key" # kept from the chart + assert resolved["easing"] == "linear" # kept from the chart + assert "keys" in spec["traces"][0] # and key matching actually works + - # The same clobbering suppresses the match='key' requires key= guard. - xy.scatter_chart( - xy.scatter(x=[1.0, 2.0], y=[1.0, 2.0], animation=xy.animation(duration=90)), +def test_mark_animation_can_restate_a_default_to_override_the_chart() -> None: + """Passing a default explicitly is an override, not an absence.""" + figure = xy.scatter_chart( + xy.scatter( + x=[1.0, 2.0], + y=[1.0, 2.0], + key=["a", "b"], + animation=xy.animation(match="index"), + ), xy.animation(match="key"), ).figure() + spec, _ = figure.build_payload_split() + resolved = {**spec.get("animation", {}), **(spec["traces"][0].get("animation") or {})} + + assert resolved["match"] == "index" + assert "keys" not in spec["traces"][0] + + +def test_mark_animation_cascade_restores_the_match_key_guard() -> None: + """The clobbering also suppressed this validation entirely.""" + with pytest.raises(ValueError, match="animation match='key' requires key="): + xy.scatter_chart( + xy.scatter(x=[1.0, 2.0], y=[1.0, 2.0], animation=xy.animation(duration=90)), + xy.animation(match="key"), + ).figure() + + +def test_mark_animation_alone_still_resolves_to_a_complete_policy() -> None: + """No chart-level spec means the cascade base is the defaults, not {}.""" + figure = xy.scatter_chart( + xy.scatter(x=[1.0], y=[2.0], animation=xy.animation(duration=90)), + ).figure() + spec, _ = figure.build_payload_split() + resolved = {**spec.get("animation", {}), **(spec["traces"][0].get("animation") or {})} + + assert resolved["duration"] == 90.0 + assert set(resolved) == set(xy.animation().to_spec()) + assert resolved["easing"] == "ease-out" + assert resolved["match"] == "index" + + +def test_animation_override_spec_reports_only_what_was_set() -> None: + assert xy.animation(duration=90).to_override_spec() == {"duration": 90.0} + assert xy.animation(match="index").to_override_spec() == {"match": "index"} + assert xy.animation().to_override_spec() == {} + assert xy.animation().to_spec() == xy.Animation().to_spec() + # Direct construction is public too; it falls back to diffing the defaults. + assert xy.Animation(duration=90).to_override_spec() == {"duration": 90.0} + assert xy.Animation().to_override_spec() == {} def test_aggregate_tier_records_key_matching_fallback() -> None: @@ -556,10 +601,7 @@ def test_aggregate_tier_records_key_matching_fallback() -> None: y=[3.0, 4.0, 5.0], key=["a", "b", "c"], density=True, - # `match="key"` has to be restated here: a mark-level animation - # spec is a full dict, so it resets every field the chart set. - # See test_mark_animation_spec_clobbers_chart_level_fields. - animation=xy.animation(duration=90, match="key"), + animation=xy.animation(duration=90), ), xy.animation(match="key"), ) @@ -589,7 +631,10 @@ def test_mark_animation_overrides_chart_defaults() -> None: assert spec["animation"]["duration"] == 500.0 assert spec["traces"][0]["animation"]["duration"] == 80.0 assert spec["traces"][0]["animation"]["enter"] == "scale" - assert spec["traces"][1]["animation"] == {"enabled": False} + # A bool override resolves like any other: only `enabled` changes, and the + # trace carries the complete policy so the client needs no defaults. + assert spec["traces"][1]["animation"]["enabled"] is False + assert spec["traces"][1]["animation"]["duration"] == 500.0 def test_disabled_mark_does_not_require_chart_level_key_matching_identity() -> None: @@ -601,7 +646,7 @@ def test_disabled_mark_does_not_require_chart_level_key_matching_identity() -> N spec, _ = chart.figure().build_payload() - assert spec["traces"][1]["animation"] == {"enabled": False} + assert spec["traces"][1]["animation"]["enabled"] is False assert "keys" not in spec["traces"][1]