Phase 10: Chart & Scrubber - #10
Merged
Merged
Conversation
- Test 8b: PointerExit during active drag must NOT clear selection (UAT-4) - Test 8c: PointerExit after drag end must still clear (D-05 companion) - Test 11b: tap-focus must paint the focus ring (UAT-5, UI-SPEC interaction-states) Tests 8b and 11b fail on current code; 8c passes (documents companion contract).
…tap-focus BUG A (UAT section 4 — scrub flickers/clears during drag): - Plumb gesture lifecycle through the fl_chart seam: buildScaffoldLineChart gains onScrubGestureStart/onScrubGestureEnd, fired on FlPanStart/FlTapDown and FlPanEnd/FlPanCancel/FlTapUp/FlTapCancel respectively. lib/utils/scaffold_chart_renderer.dart remains the ONLY fl_chart importer — the new surface is scaffold-neutral VoidCallbacks. - ScaffoldChart passes the two callbacks through unchanged. - ScaffoldChartScrubber tracks _dragActive via the new callbacks and _handleMouseExit skips the onPointSelected(null) clear while a drag is in progress. Hover-exit with no active drag still clears (Test 8 contract preserved). Tests 8b (no clear during drag) + 8c (clear after drag end) now pass. BUG B (UAT section 5 — no visible focus ring on tap): - ScaffoldFocusOutline gains an opt-in showRingWhenFocused parameter. When true, the ring paints on any primary focus regardless of FocusManager.highlightMode. Default false preserves the Phase 6 keyboard-only contract for all other atoms. - ScaffoldChartScrubber passes showRingWhenFocused: true, satisfying the Phase 10 UI-SPEC Interaction States row 'Focus'. Test 11b now passes. Files: - lib/utils/scaffold_chart_renderer.dart (+20/-2) - lib/components/scaffold_chart.dart (+17/-0) - lib/components/scaffold_chart_scrubber.dart (+95/-19) - lib/components/scaffold_focus_outline.dart (+15/-1) Gates: dart analyze --fatal-infos clean; flutter test 411 passed (was 408).
…cillation regression
UAT defect: mouse hover over the chart briefly shows the scrub line/dot,
then the readout oscillates between "Value: X" and "No selection" with a
completely STATIONARY mouse. Root cause (to be fixed in GREEN): fl_chart
fires FlPointerHoverEvent through touchCallback on every hover move; the
renderer forwards every event's spot; the atom's onSpotTouched toggles via
identical() on EVERY event — a second hover hit on the selected point
clears it, then the next hover hit re-selects it.
- Test 13: hover onto point selects; hover to another point updates;
re-hover of the selected point must NOT clear; stationary-hover probe
across 6 pump cycles must never emit null. FAILS on current code at the
stationary-hover probe (iteration 0) — the exact UAT oscillation.
- Test 14: discrete tap on the already-selected point must still toggle
off (tap-toggle contract preserved). PASSES — documents kept behavior.
- Spot pixel positions are discovered via a deterministic 1px calibration
sweep (fl_chart touchSpotThreshold is 10px; Semantics('Chart') box wraps
the whole atom, so spot pixels are not derivable from the box alone).
…ion at the fl_chart seam Root cause: fl_chart 1.2.0 fires FlPointerHoverEvent through touchCallback on EVERY hover move (render_base_chart.handleEvent:146), and re-fires for a stationary mouse whenever the chart rebuilds. The renderer forwarded every event's spot, and the atom's onSpotTouched toggled via identical() on EVERY event — a hover re-hit on the selected point cleared it, the rebuild re-fired hover, re-selecting it: the stationary-mouse oscillation between "Value: X" and "No selection" reported in UAT. Fix (minimal, at the D-02 seam): - scaffold_chart_renderer.dart: new ScaffoldSpotTouched typedef (spotIndex, isDiscreteTap). touchCallback now forwards a spot ONLY for intent-carrying events (FlTapDownEvent / FlPointerHoverEvent / FlPanDown|Start|UpdateEvent / FlLongPressStart|MoveUpdate) and reports isDiscreteTap = (event is FlTapDownEvent). Gesture-END events (FlTapUpEvent / FlPanEndEvent / cancels) no longer forward a spot — a tap-up forward would instantly re-select the point a tap-toggle just cleared (pointer is still over it at tap-up). - scaffold_chart.dart: toggle-clear applies only when isDiscreteTap AND identical(touched, selectedPoint). Hover/pan hits always SELECT. Behavior contract after this fix: - Hover over the plot continuously shows scrub line/dot + readout value; "No selection" is impossible while the pointer is over the plot. - Selection clears ONLY via discrete tap on the selected point, Escape, or hover-exit (D-05, unchanged — Test 8/8b/8c still green). - Discrete-tap toggle preserved (Test 14 green). Tests: 411 -> 414 (+ Test 13 hover-oscillation regression, Test 14 tap-toggle contract, renderer Test 6b hover/pan isDiscreteTap=false). dart analyze --fatal-infos clean; full flutter test green (414 passed).
- Test 14 rewritten to the new contract: discrete tap on the selected point must NOT fire null (tap ALWAYS selects; clearing is Escape / pointer-exit only per D-05 and 10-UI-SPEC). - Test 15 added: announceValue null<->value transitions must not drop primary focus (reproduces the focus-ring blink UAT defect). - Test 13 calibration keyed by value (o.dy) instead of Offset identity — fixes a latent misrecording when re-hovering calibrated spots.
… re-selects D-05 / 10-UI-SPEC never specified tap-to-clear. The toggle branch (isDiscreteTap && identical(...)) fired onPointSelected(null) when the user re-tapped the selected point, which UAT read as a bug. Tap, hover, and drag now ALWAYS select; clearing happens only via Escape or pointer-exit. The renderer's isDiscreteTap discrimination stays at the fl_chart seam (correct plumbing) but the atom no longer clears on it.
… longer blinks Root cause: build() returned Semantics(child: _ScrubberCore) when announceValue was null but ScaffoldLiveRegion(child: ...) when non-null. The type toggle directly above _ScrubberCore on every select/clear discarded the core's State (and its FocusNode), dropping primary focus — the ring blinked off until the next tap's Listener.onPointerDown re-focused the new node. Fix: always wrap in ScaffoldLiveRegion, passing value: null when there is nothing to announce (Semantics(value: null) is silent). The widget type above the core is now constant across announceValue transitions. Knock-on fix in ScaffoldLiveRegion: explicitChildNodes: true so the region's Semantics no longer merges (and hides) the descendant 'Chart scrubber' / 'Chart' labels now that the wrapper is always present. Test 10 updated to the new contract (wrapper always present, silent when null). Test 15 (RED, previous commit) now passes.
…zer clean) Replaces the tester.state<State<StatefulBuilder>>().setState cast, which tripped invalid_use_of_protected_member + unnecessary_cast.
…ector, D-10 gesture rule (UAT-driven)
… scrub, D-09 range selector, D-10 gesture rule)
- Snap-mode regression: bare LineChart return, dot painter intact - Smooth-mode: Stack wrapper, fl_chart dot hidden, continuous position forwarding on hover/pan/long-press via real pointer gestures - Overlay dot pixel placement + gesture-end clearing - Tap carries no continuous-position intent
…rlay Completes the RED from d294b8b. Continuous position flows via onScrubPositionChanged on hover/pan-move/long-press-move (touchChartCoordinate); dot rides a widget-space Stack overlay mirroring fl_chart's getPixelX/getPixelY usable-width mapping (fl_chart's own indicator is spot-index-locked). Snap mode (callback null) returns the bare LineChart unchanged. fl_chart still isolated. Test-harness corrections folded in: locate the smooth-mode Stack as the direct LineChart parent (MaterialApp/Scaffold Stacks are ambiguous); gutter-aware usable-plot rect (framed charts reserve kChartAxisGutter for right titles); real-gesture pumps (mouse hover / startGesture-moveBy) replace direct touchCallback invocation. Added Test 1b: stationary hover never re-fires onScrubPositionChanged across rebuilds (GlobalKey preserves the render object).
ScrubMode enum (snap default + smooth), onPositionChanged pass-through wiring (snap → null, smooth → wired), nearest-T onPointSelected in smooth mode, keyboard + hover-exit unchanged. Real PointerDeviceKind.mouse hovers.
…hartScrubber (D-08) Wire the D-08 smooth-scrub switch through the atom and its scrubber composition: - scaffold_chart.dart: add ScrubMode enum (snap default, smooth) at the base atom to avoid a chart->scrubber->chart circular import; add scrubMode + onPositionChanged params/fields; pass onScrubPositionChanged + smoothSpots to the renderer only in smooth mode. - scaffold_chart_scrubber.dart: re-export ScrubMode for scrubber-only importers; add scrubMode + onPositionChanged params/fields; pass through _ScrubberCore to ScaffoldChart. - scrubber test: drop now-redundant chart import (ScrubMode re-exported). gates: dart analyze --fatal-infos clean; flutter test 435 pass / 1 skip.
…om (D-09/D-10) D-10 deviation (Rule 1, plan bug): the plan's overlay used HitTestBehavior.opaque + a conditional Positioned.fill. opaque would starve the chart of tap/hover (breaking D-10's own pass-through contract and the conditional overlay could never capture the FIRST drag (chicken-and-egg). Implemented instead: always-present translucent GestureDetector + IgnorePointer on the CustomPaint — the horizontal-drag recognizer joins the arena first (suppressing pan-scrub) while tap/hover pass through to point-scrub. EOF )
A drag whose pixel->chart mapping fails (degenerate X window, collapsed layout mid-gesture, empty series) previously forwarded the null mapping to onRangeSelected, silently clearing the user's existing range. Only fire the callback when a real range is produced; document that null is reserved for the explicit Escape clear. Adds Test 29 regression: degenerate X window (viewMinX == viewMaxX) must NOT fire onRangeSelected.
…at equality The previous start.dx == current.dx check swallowed a drag that returned to its start pixel and was unreliable on platforms whose pointer quantization reports the up-coordinate as exactly the down-coordinate after a small out-and-back drag. Track whether any DragUpdateDetails arrived between drag-start and drag-end; that matches what the gesture arena actually decided. Adds Test 30 regression: drag out and back to the start pixel must fire onRangeSelected.
_stackWidth/_stackHeight are captured during layout and read at gesture time. A parent resize mid-drag (orientation change, split-view resize, window drag) previously left _dragStart anchored against the OLD width, producing a band whose left edge jumped and a mapped range whose startT was wrong. Drop _dragStart/_dragCurrent when the LayoutBuilder's constraints materially change so the next drag starts clean.
_chartXOfPixel/_pixelXOfChart read context.dimens at gesture time, which establishes an inherited-widget dependency outside the build phase (unsupported by Flutter) and resolves whatever the theme returns AT THAT MOMENT — so a theme swap mid-drag mixes old drag-start pixels with new padding. Resolve space8 once in build into a State field and read the cached value from the gesture handlers.
…ounds The early return on available <= 0 already guarantees available > 0 at the division site, so max(1, available) can never observe a value less than 1 — it is dead defensive code that masks intent. Drop it.
…hart The force-unwrap crashed a consumer whose ThemeData has labelSmall: null (legitimate for embedded surfaces). Degrade to the M3 default 11pt size via a named constant rather than throwing a null-check error.
Both tests now pass against the current implementation, but their bookkeeping comments still claimed 'FAILS against the current … — RED'. Rewrite to describe the asserted contract and drop the TDD bookkeeping — that belongs in commit messages, not in test source.
The captured down position is always overwritten before use, so this is not a bug fix — adding an explicit onPointerCancel handler makes the field's down → cancel → null lifecycle visible rather than implicit.
_handleSmoothPixel was called on every gesture-end even when _smoothDotPosition was already null (no smooth scrub ever began), scheduling a needless wrapper rebuild each time. Early-out when the new value equals the current one.
The top-level final list was shared mutable state across every smooth-mode test. No current test mutates it, but a future test that sorts, filters, or adds to the list would silently contaminate later tests in the same run. Mirror _spots() by returning a fresh list from a function.
The step * 1e-9 slack is relative — it scales with the step, absorbing representation error at the top of the ladder at normal axis steps. At pathologically small steps (~1e-12) the slack is far below double resolution and contributes nothing. Not a bug today; document the mismatch so a future reader is not misled by the constant.
Replace the magic 100.0 / 50.0 / 25.0 in the demo series expression with _kYBaseValue / _kYAmplitude / _kYOffset, matching the project's no-magic-numbers convention. Cosmetic — demo-only.
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2effb7fb3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…t dot clear Codex PR-10 P2 findings: - CX-1: scrubMode=smooth with no onPositionChanged never entered smooth mode (gated on callback presence) — test asserts the interpolated dot paints on hover with no callback supplied. - CX-2: pointer exit is not a gesture end, so the smooth overlay dot stuck after the mouse left — test asserts hover-exit clears the dot. Both fail against the current implementation (RED).
… clear Codex PR-10 P2 findings: - CX-1: smooth mode was gated on the consumer's optional onPositionChanged callback — smooth visuals now engage on scrubMode alone (internal no-op drives the overlay when no readout callback is supplied). - CX-2: FlPointerExitEvent is not a gesture end, so the interpolated dot stuck after the mouse left the chart. Pointer exit now clears the overlay and signals the wrapper via kScrubPositionExitSentinel (NaN) so the consumer callback is suppressed (exit carries no position intent).
…elector Codex PR-10 P2 finding: the file-level doc comment promises plain ArrowLeft/Right route to point scrub, but no scrubber is composed and no plain-arrow shortcuts exist. Test asserts ArrowRight from no selection fires onPointSelected(series.first) without mutating the range. Fails against the current implementation (RED).
…lector Codex PR-10 P2 finding: the file-level doc comment promised plain-arrow point scrub pass-through, but no scrubber is composed inside the selector and no plain-arrow shortcuts existed — keyboard users could adjust a range but not navigate its points. Adds _Prev/_Next point intents + handlers mirroring ScaffoldChartScrubber's contract (null selection → first/last, clamped steps, no-op when onPointSelected is unwired), and corrects the doc comment to describe the implemented routing.
…ambda The no-op needs two distinct params; wildcard '_' cannot repeat in one parameter list.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02794d5bcf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+623
to
+626
| final bool isGestureEnd = event is FlPanEndEvent || | ||
| event is FlPanCancelEvent || | ||
| event is FlTapUpEvent || | ||
| event is FlTapCancelEvent; |
There was a problem hiding this comment.
After a FlLongPressMoveUpdate positions the smooth overlay, lifting the pointer produces a long-press end event, but isGestureEnd recognizes only pan and tap completion. The smooth dot is therefore never cleared after a completed long-press scrub and can remain stuck on the chart; include long-press completion in the lifecycle mapping so it also invokes onScrubGestureEnd and clears onSmoothPositionPixel.
Useful? React with 👍 / 👎.
Comment on lines
+654
to
+656
| onHorizontalDragStart: _onDragStart, | ||
| onHorizontalDragUpdate: _onDragUpdate, | ||
| onHorizontalDragEnd: _onDragEnd, |
There was a problem hiding this comment.
If an accepted horizontal drag is canceled by the platform, pointer routing, or an interrupted gesture, none of these handlers resets _dragStart and _dragCurrent; Listener.onPointerCancel only clears _downPosition. The transient band consequently remains painted and masks the consumer-owned selectedRange until another drag or resize occurs, so wire onHorizontalDragCancel to clear all active drag state.
Useful? React with 👍 / 👎.
Comment on lines
+408
to
+412
| final Offset? dot = _smoothDotPosition; | ||
| return Stack( | ||
| children: <Widget>[ | ||
| chart, | ||
| if (dot != null) |
There was a problem hiding this comment.
While the pointer is stationary, rebuilding a smooth chart with new series data, Y bounds, view bounds, or dimensions retains _smoothDotPosition from the previous geometry and paints it over the newly rendered line. The preserved inner render object intentionally does not re-fire stationary hover events, so the position remains wrong until the pointer moves or exits; clear or recompute this transient position from didUpdateWidget when mapping inputs change.
Useful? React with 👍 / 👎.
Comment on lines
+695
to
+700
| /// The edge rules are the band's visual "handles". The hit area for the | ||
| /// edges is the full band extent — the overlay GestureDetector covers the | ||
| /// entire plot (translucent), and the whole interaction surface is wrapped | ||
| /// in `ScaffoldTouchTarget` to guarantee the 48x48 minimum interactive | ||
| /// dimension. No separate edge-handle geometry is needed because the band is | ||
| /// report-only: resizing happens via Shift+Arrow, not edge dragging. |
There was a problem hiding this comment.
When a selected band is already present, the two painted edge rules are not independent handles: the implementation has only one full-chart gesture target, so dragging an edge starts a replacement range instead of resizing that endpoint (for example, dragging the left edge of [20, 40] toward 10 produces [10, 20], not [10, 40]). This also does not provide the promised per-edge 48×48 interaction targets; add edge-specific hit regions and drag handling rather than treating the whole overlay as both handles.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 10: Chart & Scrubber
Goal: A neutral chart primitive renders any consumer-supplied series and supports point scrubbing — closing the Insight Cards gap without domain knowledge leaking into the scaffold.
Status: Verified ✓ (6/6 must-haves, goal-backward)
Ships a neutral, theme-token-driven chart stack for the scaffold: pure chart-geometry helpers, the single fl_chart renderer seam (D-02), the
ScaffoldChart<T>/ScaffoldChartScrubber<T>composed atoms, and the 10-06 gap-extension — D-08 smooth scrub (continuous dot riding the line), D-09ScaffoldChartRangeSelector<T>drag-band atom (report-only range selection), and the D-10 gesture-conflict rule. Barrel exports and example demos included; human UAT approved under both dark and light palettes.Changes
Plan 10-01: Pure chart geometry support part
chartTickStep/chartAxisLabel/chartXLabelCount/chartYTickCount/chartUsesFrame/chartBandedBounds/chartVisibleExtremes+ tests.Key files:
lib/utils/chart_geometry.dart,test/utils/chart_geometry_test.dartPlan 10-02: fl_chart renderer support part (the ONLY fl_chart seam)
Renderer translating the neutral contract into fl_chart; pubspec adds fl_chart ^1.2.0.
Key files:
lib/utils/scaffold_chart_renderer.dart,test/utils/scaffold_chart_renderer_test.dartPlan 10-03: ScaffoldChart<T> atom
Neutral series contract, framed/axis-free layouts, X-axis as Row of Texts.
Key files:
lib/components/scaffold_chart.dart,test/components/scaffold_chart_test.dartPlan 10-04: ScaffoldChartScrubber<T> atom
Keyboard a11y + focus outline + PointerExit + 48x48 touch target + live-region hook.
Key files:
lib/components/scaffold_chart_scrubber.dart,test/components/scaffold_chart_scrubber_test.dartPlan 10-05: Barrel exports + demos + UAT
Barrel exports for chart/scrubber/geometry/renderer; ChartDemo + ChartScrubberDemo registered; UAT approved.
Key files:
lib/frontend_scaffold.dart,example/lib/demos/chart_demo.dart,example/lib/demos/chart_scrubber_demo.dart,example/lib/main.dartPlan 10-06: D-08 smooth scrub + D-09 range selector + D-10 gesture rule
Renderer smooth seam + interpolated-dot overlay;
ScaffoldChartRangeSelector<T>report-only drag-band atom; gesture-arena-correct suppression of inner pan-scrub; new demo + smooth-mode section.Key files:
lib/components/scaffold_chart_range_selector.dart,test/components/scaffold_chart_range_selector_test.dart,example/lib/demos/chart_range_selector_demo.dart, plus modifications to the renderer/chart/scrubber/geometry and demo registration.Requirements Addressed
ScaffoldChartrenders a series via a neutral data contract (series,xAccessor,yAccessor) with no domain knowledge. ✓ScaffoldChartScrubberprovides point selection/scrubbing (selectedPoint,onPointSelected) composing withScaffoldChart; extended by D-08 smooth mode + D-09/D-10 range selection. ✓Verification
10-VERIFICATION.mddart analyze --fatal-infos— cleanflutter test— 451 passed, 1 skippedpackage:fl_chartimported only inlib/utils/scaffold_chart_renderer.dart10-REVIEW.md)Key Decisions
HitTestBehavior.translucent+IgnorePointer(not the plan'sopaque), so the horizontal-drag recognizer claims the arena first (suppressing pan-scrub) while tap/hover still pass through to point-scrub.onRangeSelected(null); an in-flight band drag cancels on resize mid-drag;dimens.space8is cached in build rather than read at gesture time.