UI Charts: show/hide tooltips on touch - #32613
Merged
Merged
Conversation
Tooltips appear when the finger rests on the chart, follow it while inspecting and never flash during swipes or scrolling. Charts no longer receive raw touch events; a shared gate drives both chart libraries via synthetic mouse events. Works around WebKit reporting wrong offsetX/Y for synthetic events.
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The cleanup function returned by
attachTouchTooltipGateis never used in the echarts helper or Chart.js plugin, so touch listeners and theuser-select-noneclass will remain attached after charts are destroyed; consider wiring the cleanup into chart disposal/teardown. - Overriding
offsetX/offsetYviaObject.definePropertyon a syntheticMouseEventmay fail or be ignored in some browsers where these properties are non-configurable; it would be safer to guard this with a feature check or try/catch and fall back gracefully.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The cleanup function returned by `attachTouchTooltipGate` is never used in the echarts helper or Chart.js plugin, so touch listeners and the `user-select-none` class will remain attached after charts are destroyed; consider wiring the cleanup into chart disposal/teardown.
- Overriding `offsetX`/`offsetY` via `Object.defineProperty` on a synthetic `MouseEvent` may fail or be ignored in some browsers where these properties are non-configurable; it would be safer to guard this with a feature check or try/catch and fall back gracefully.
## Individual Comments
### Comment 1
<location path="assets/js/utils/swipe.ts" line_range="82-85" />
<code_context>
+ }
+ };
+
+ const onTouchEnd = (e: TouchEvent) => {
+ e.stopPropagation();
+ // suppress compat mouse events after touch, they would re-trigger the tooltip
+ if (e.cancelable) e.preventDefault();
+ if (timer) clearTimeout(timer);
+ shown = false;
</code_context>
<issue_to_address>
**issue (bug_risk):** Calling `preventDefault` on `touchend` will suppress all compat mouse events, including clicks.
While this prevents tooltip re-triggering, it also suppresses `click` and `mousedown/mouseup` events that other chart and UI interactions depend on (e.g., legend toggles, series selection). On touch devices, this can make the chart and clickable content within `el` non-interactive. Consider restricting `preventDefault` (e.g., only when the tooltip is currently shown or a dwell gesture is detected) or applying it to a more specific element so unrelated click behavior remains functional.
</issue_to_address>
### Comment 2
<location path="assets/js/utils/swipe.ts" line_range="17" />
<code_context>
+// the finger rests for a moment, then follows it; fast movement (swipes, scrolling)
+// keeps it hidden and a pause re-arms it. The tooltip is driven by a synthetic
+// mousemove at the finger position, which both chart libs handle natively.
+export function attachTouchTooltipGate(el: HTMLElement, hide: () => void): () => void {
+ let timer: ReturnType<typeof setTimeout> | null = null;
+ let shown = false;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the touch tooltip gate into clearer helpers and an explicit gesture state machine to reduce cognitive load while preserving behavior.
You can reduce the cognitive load without changing behavior by factoring out a few small helpers and making the gesture state explicit. This keeps all logic in one function but separates responsibilities.
**1. Encapsulate timer management**
Avoid repeating `if (timer) clearTimeout(timer)` and make timer behavior clearer:
```ts
const clearTimer = () => {
if (timer) {
clearTimeout(timer);
timer = null;
}
};
const restartDwell = () => {
clearTimer();
anchorX = x;
anchorY = y;
timer = setTimeout(show, DWELL_MS);
};
```
Then update call sites:
```ts
// onTouchMove / onTouchEnd
clearTimer();
```
**2. Extract movement threshold helpers**
Make the movement intent clearer and avoid repeating `Math.hypot` calls and magic numbers:
```ts
const hasScrolledOrSwiped = () =>
Math.hypot(x - gestureX, y - gestureY) > 30;
const hasBrokenDwellSlop = () =>
Math.hypot(x - anchorX, y - anchorY) > DWELL_SLOP_PX;
```
Then use them:
```ts
if (!shown && !latched && hasScrolledOrSwiped()) {
latched = true;
clearTimer();
}
if (latched) return;
if (shown) {
show();
} else if (hasBrokenDwellSlop()) {
restartDwell();
}
```
**3. Replace `shown`/`latched` booleans with an explicit gesture state**
This makes the tooltip/gesture lifecycle easier to reason about and documents the state machine in code:
```ts
type GestureState = 'idle' | 'armed' | 'latched' | 'tooltip';
let state: GestureState = 'idle';
const setState = (next: GestureState) => {
state = next;
};
const show = () => {
const target = document.elementFromPoint(x, y);
if (!target || !el.contains(target)) return;
setState('tooltip');
// ... existing synthetic mousemove dispatch ...
};
const onTouchStart = (e: TouchEvent) => {
// ...
setState('armed');
gestureX = x;
gestureY = y;
restartDwell();
};
const onTouchMove = (e: TouchEvent) => {
// ...
if (state !== 'tooltip' && state !== 'latched' && hasScrolledOrSwiped()) {
setState('latched');
clearTimer();
return;
}
if (state === 'latched') return;
if (state === 'tooltip') {
show();
} else if (hasBrokenDwellSlop()) {
restartDwell();
}
};
const onTouchEnd = (e: TouchEvent) => {
// ...
clearTimer();
setState('idle');
hide();
};
```
This keeps all functionality intact but makes the gesture/tooltip state machine more explicit and the handlers less intertwined, addressing most of the complexity concerns without a large restructuring.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
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.
Follow-up to #32603. Chart tooltips on touch devices now behave like a native scrubber (history, battery, sessions, optimizer, forecast).
📊 Rest your finger on a chart and the tooltip appears; slide slowly to inspect values; lift to dismiss.
🚫 No more tooltip flashing when swiping or scrolling across a chart.
🧹 No more stale tooltips or stuck bar highlights when touching several charts in a row.
📱 Fixes tooltips not appearing in the iOS app (WebKit reports wrong coordinates for synthetic events).
✋ Long-press no longer selects chart or tooltip text.
Sessions and optimizer still render with chart.js (legacy, to be replaced by echarts).
🤖 Generated with Claude Code