Accessibility: last-move crosshair

[superkeil]

Fixes #

Draft / WIP — depends on goban PR online-go/goban#247.
The submodules/goban pointer currently references the head of that PR. Once goban#247 merges, the submodule will be re-pointed to the merged commit on goban:main before this leaves draft.

Proposed Changes

• Add an opt-in Accessibility settings section with a "Highlight the last move with a crosshair" toggle (disabled by default), plus color picker (default #1e6bff) and a relative thickness slider, with a live MiniGoban preview beside the controls.
• Add the preferences (accessibility.last-move-crosshair{,-color,-thickness}) and wire them into the goban via a new getLastMoveCrosshair() config callback in configure-goban.
• Suppress the crosshair on MiniGoban thumbnails/preview boards by default (via goban's new dont_draw_last_move_crosshair flag); the setting's own preview opts back in.
• Add the --z-goban-crosshair-layer z-index variable used by goban's crosshair layer.
• Design spec + implementation plan under docs/superpowers/.

The rendering itself (Canvas + SVG, under-stone crosshair) is in goban#247.

Notes for reviewers

• Manual testing done in both renderers: lines span the full board, pass under the stones, follow each move, erase cleanly on navigation; color/thickness work; absent on thumbnails and unrelated setting previews; disabled by default.
• Please test on mobile and desktop, logged-in and anonymous.

[github-actions]

Code Review Findings

Bug: props.json spread overrides lastMoveCrosshair prop

In MiniGoban.tsx, the new flag is assigned before ...props.json, so any caller that passes a json prop containing dont_draw_last_move_crosshair will silently override the new feature:

dont_draw_last_move_crosshair: \!props.lastMoveCrosshair,
...props.json,   // wins if json contains this key

props.json is typed as any. Once goban#247 exposes this field on GobanEngineConfig, a server-supplied game config that includes it would bypass lastMoveCrosshair entirely. The assignment should come after the spread:

...props.json,
dont_draw_last_move_crosshair: \!props.lastMoveCrosshair,

---

Wrong translation function: llm_pgettext used for permanent UI strings

Every user-visible string in AccessibilityPreferences.tsx uses llm_pgettext. Per project convention (and how every other Settings page is written — see ThemePreferences.tsx), permanent UI labels should use pgettext so they enter the volunteer translation pipeline. llm_pgettext is reserved for strings generated or summarised by an LLM at runtime. The preference titles, descriptions, and control labels here are static and belong in the volunteer pipeline.

---

Cosmetic: sample board preview always shows crosshair on a white stone

The four-move sequence in crosshair_sample_board is [{x:0,y:2},{x:4,y:2},{x:2,y:4},{x:2,y:2}]. Moves alternate starting from black, so the last move is white at center (2,2). The preview therefore always shows the crosshair on a white stone. Removing the fourth move so that black lands at center — or swapping moves 3 and 4 — would show the crosshair on a black stone, which is the more common last-move case and may contrast differently with the default blue colour.

[GreenAsJade]

(Note: anoek is away at the moment, please don't take lack of response as lack of interest. A screenshot would help with a quick appreciation of what this does, for the curious)

[superkeil]

Thanks for the review — all three addressed:

• ...props.json override: dont_draw_last_move_crosshair is now assigned after the ...props.json spread in MiniGoban, so a server-supplied config can't bypass the lastMoveCrosshair prop.
• Translation function: switched the static UI labels from llm_pgettext to pgettext.
• Sample preview: the board now ends on a black stone at the centre.

Also added a Playwright e2e test (e2e-tests/accessibility/crosshair.spec.ts) that drives the setting and asserts the crosshair renders with the chosen colour and thickness on both the SVG and the old Canvas renderer, and bumped the goban submodule to the updated goban#247 head.

[github-actions]

Reviewed for bugs and performance issues — nothing significant found.

The implementation is clean and follows established codebase patterns throughout: preference keys, callback wiring, the MiniGoban key-based remounting for the live preview (matches the same pattern already used in ThemePreferences for stone-font-scale and other sliders), and the goban config flag placement after the json spread to prevent server-supplied config from overriding the prop.

The getLastMoveCrosshair getter correctly reads fresh preference values on every draw without any caching hazard. No type safety gaps, no missing bounds checks that the HTML range input does not already enforce.

The only open item is the submodule pointer, which the PR description already flags (waiting for goban#247 to merge before leaving draft).

[superkeil]

(Note: anoek is away at the moment, please don't take lack of response as lack of interest. A screenshot would help with a quick appreciation of what this does, for the curious)

@GreenAsJade in attachment you will find 2 screenshots

I hope it would make this PR better understood

[github-actions]

Code review: two issues found - (1) E2E SVG selector likely broken: .crosshair-layer vs CrosshairLayer PascalCase mismatch; (2) MiniGoban re-mounts on every slider tick performance concern. See full comment below.

[github-actions]

Bug: E2E test SVG selector likely broken. In e2e-tests/accessibility/crosshair.spec.ts, SVG crosshair lines are queried via .crosshair-layer line. The Canvas renderer names its layer CrosshairLayer (PascalCase - see the submodule Goban.css and the canvas-phase assertion in the same test: canvas.CrosshairLayer). If the SVG layer follows the same PascalCase convention, the kebab-case selector .crosshair-layer will never match and the toBeAttached() assertion will fail. Please verify the actual class on the SVG layer and align the selector.

[github-actions]

Performance: MiniGoban re-mounts on every slider tick. The preview uses a key built from color and thickness to force re-mounts when preferences change. A range input fires onChange on every pixel during a drag (30-60x per second), so each slider position unmounts and remounts a full GobanController and writes to localStorage. For a 5x5 board the cost per mount is small, but a short debounce (~80ms) on the setThickness/setColor calls - keeping local controlled-input state separate from the persisted preference - would eliminate the churn without affecting perceived responsiveness.

[superkeil]

Thanks! Two clarifications on the latest automated review:

1. E2E SVG selector — not a mismatch (working as intended). The two renderers deliberately use different class conventions, matching their existing layers:

• Canvas: className = "CrosshairLayer" (PascalCase, like StoneLayer / ShadowLayer)
• SVG: class="crosshair-layer" (kebab-case, like the SVG shadow-layer / grid layers)

So .crosshair-layer (SVG) and .CrosshairLayer (Canvas) are both correct. The SVG-phase assertions in the test (.crosshair-layer line stroke / stroke-width) pass on every run — if the selector didn't match, those would fail.

2. Slider re-mount — consistent with the established pattern. The key-based MiniGoban re-mount mirrors what ThemePreferences already does for its slider previews (e.g. key={stone_font_scale + …} for the stone-font-scale sample). The preview is a tiny 5×5 board shown only on this settings page, so the per-tick cost is negligible. Debouncing only this one setting would diverge from the existing convention; optimising slider previews would be better as a separate, codebase-wide change.

Happy to revisit if you'd prefer the debounce here regardless.

[github-actions]

Code review: bugs and performance issues

AccessibilityPreferences.tsx — preview flickers and creates excessive goban instances during slider drag

key={\${color}-${thickness}`}forces a fullMiniGobanunmount+remount whenever either value changes. For the color picker (discrete interactions) the cost is low, but thethicknessrange slider firesonChangeon **every pointer-move step** during drag, so dragging from one end to the other triggers ~38 full mount/destroy cycles. Each cycle tears down the canvas element and creates a newGobanController`, producing a visible flash on every step.

The remount is also unnecessary: getLastMoveCrosshair() in configure-goban.tsx reads preferences at call time, so the running goban already picks up the new values at its next draw. Calling controller.goban.redraw() after the preference is set (via a useEffect with [color, thickness] deps, keeping a useRef to the controller) achieves the same result without tearing down the board.

AccessibilityPreferences.tsx — preference written to storage on every slider event

setThickness(parseFloat(ev.target.value)) (and similarly setColor) calls preferences.set() on every pointer-move event. For a single drag across the full range that is 30–40 storage writes. A common pattern is to track the in-flight value with local useState (for immediate UI feedback) and commit to preferences.set only on onPointerUp / onBlur. This also eliminates the need to remount the board: update local state on every event to keep the slider position responsive, then trigger one preference write + one goban.redraw() on pointer release.

These two issues share the same root cause and fixing either one partially addresses both; fixing both together gives a flicker-free, storage-efficient preview.

[superkeil]

On the slider re-mount / per-tick preference write: this is the established convention, not new behaviour. ThemePreferences' "Stone font scale" range slider does exactly the same — onChange={setStoneFontScale} writes the preference on every tick, and its preview MiniGoban re-mounts via key={stone_font_scale + …} on every tick (ThemePreferences.tsx ~L378/L386). The custom-colour pickers there behave the same way.

So the per-tick remount + write isn't introduced by this PR; it matches the surrounding code. Moving slider previews to local controlled state + a single redraw()/preferences.set on pointer-release is a reasonable improvement, but it'd be better as a separate, codebase-wide change so all the settings previews stay consistent — happy to do that separately if the team wants it.

[github-actions]

Code Review: main-repo changes

Reviewed: preferences.ts, configure-goban.tsx, AccessibilityPreferences.tsx, MiniGoban.tsx, Settings.tsx, 01_variables.css, e2e-tests.

No bugs found. One performance concern:

MiniGoban preview remounts on every slider tick

The key prop (key=color-thickness) on the MiniGoban preview changes on every onChange from the thickness range input, which fires continuously while dragging. With step=0.01 over the range 0.02-0.4, a single drag creates and destroys ~38 goban instances in quick succession.

Since getLastMoveCrosshair() reads from preferences.get() at draw time, a debounce on the key update would avoid the churn. The current approach works on a 5x5/150px board on modern hardware, but may cause visible jank on lower-end devices.

---

Everything else looks correct: the dont_draw_last_move_crosshair override in MiniGoban is placed after the json spread (preventing server config from overriding it), the callback in configure-goban.tsx follows the established getter pattern, and the Settings registration matches all other preference pages.

[github-actions]

Performance: Excessive MiniGoban remounting during settings adjustments

In src/views/Settings/AccessibilityPreferences.tsx, the preview MiniGoban uses key={${color}-${thickness}} to force a remount whenever color or thickness changes. Both <input type="color"> and <input type="range"> fire React's onChange continuously while the user drags, so this causes a full canvas teardown + reallocation on every frame of slider/picker movement — potentially 60+ times per second.

The architecture already avoids this: getLastMoveCrosshair() reads from the preference store at draw time (see configure-goban.tsx), so after setColor/setThickness updates the store the board only needs a redraw() call, not a full remount. Something like:

const gobanRef = useRef<Goban | null>(null);

// in the MiniGoban, expose a ref and call gobanRef.current?.redraw(true)
// whenever color or thickness changes, instead of changing the key

The canvas allocation limit is a known concern in this codebase (see canvasAllocationErrorHandler in configure-goban.tsx); the rapid create/destroy cycle on drag puts unnecessary pressure on that limit.

[github-actions]

Code review

src/components/MiniGoban/MiniGoban.tsx, line 357 -- props.lastMoveCrosshair missing from useEffect dependency array

The PR adds dont_draw_last_move_crosshair: \!props.lastMoveCrosshair inside the useEffect that constructs the GobanController, but props.lastMoveCrosshair is not in the effect dependency array:

}, [props.game_id, props.review_id, props.width, props.height]);

If a caller changes the lastMoveCrosshair prop after initial mount without also changing game_id, review_id, width, or height, the stale closure keeps the original dont_draw_last_move_crosshair value and the GobanController is never re-created to reflect the updated flag. Current call sites happen to be safe (the settings preview uses a key-based remount; all other MiniGoban usages omit the prop entirely), but the missing dep is a correctness gap that will silently misbehave if the prop is ever toggled dynamically.

Fix: add props.lastMoveCrosshair to the dependency array.

[superkeil]

On the missing useEffect dependency (props.lastMoveCrosshair): this is intentional, and consistent with the surrounding code rather than an oversight.

dont_draw_last_move_crosshair: !props.lastMoveCrosshair is a construction-time flag, in the same object literal as connect_to_chat: !!props.chat, enable_sounds, draw_*_labels, last_move_opacity, stone_font_scale — all read once when the GobanController is built, and none of them are in the dependency array. That array is deliberately scoped to board identity ([game_id, review_id, width, height]): the controller is expensive to construct (chat/socket wiring, etc.), so it's only rebuilt when the board itself changes. By the review's logic connect_to_chat on the line above would be an identical 'gap'.

On the dynamic-toggle scenario: the only caller that passes the prop is AccessibilityPreferences.tsx, with a constant lastMoveCrosshair={true}, and that preview already remounts via a key. Every other MiniGoban usage omits the prop entirely. So adding it to the deps is a provable no-op for all current call sites, and react-hooks/exhaustive-deps (which we run in CI) does not flag the current array.

I've added a short comment at the flag making this explicit (construction-time, why it's not a dep) so the intent is clear to future readers. If the team would rather move all settings previews to controlled local state + a single redraw() (which would also address the slider-remount thread above), that'd be a good separate, codebase-wide change.

[github-actions]

Code Review

Summary: Adds an opt-in accessibility setting to draw a crosshair through the last-played stone, with color/thickness controls and a live MiniGoban preview.

---

Performance: preview board re-mounts on every slider step

src/views/Settings/AccessibilityPreferences.tsx — the MiniGoban uses key={${color}-${thickness}} to force a remount whenever color or thickness changes. For the color picker this fires once per pick, which is fine. For the range slider (step=0.01, range 0.02–0.40) a single full drag fires ~38 separate onChange events, each one tearing down and re-initializing the entire goban (canvas/SVG setup, layout, event wiring). On slower devices this will cause visible flicker and may stall the UI thread mid-drag.

A simple debounce on the key — or on just the thickness preference write — would reduce this to a handful of re-mounts per drag rather than one per step, without requiring any goban API change. The color picker can keep the direct key since it only fires on commit.

[superkeil]

Code Review

Summary: Adds an opt-in accessibility setting to draw a crosshair through the last-played stone, with color/thickness controls and a live MiniGoban preview.

Performance: preview board re-mounts on every slider step

src/views/Settings/AccessibilityPreferences.tsx — the MiniGoban uses key={${color}-${thickness}} to force a remount whenever color or thickness changes. For the color picker this fires once per pick, which is fine. For the range slider (step=0.01, range 0.02–0.40) a single full drag fires ~38 separate onChange events, each one tearing down and re-initializing the entire goban (canvas/SVG setup, layout, event wiring). On slower devices this will cause visible flicker and may stall the UI thread mid-drag.

A simple debounce on the key — or on just the thickness preference write — would reduce this to a handful of re-mounts per drag rather than one per step, without requiring any goban API change. The color picker can keep the direct key since it only fires on commit.

already answered at #3584 (comment)