Skip to content

feat(chart): dim non-hovered series rows in tooltip#447

Open
lkipke wants to merge 8 commits intocloudflare:mainfrom
lkipke:levi/add-dimming-to-chart-tooltip
Open

feat(chart): dim non-hovered series rows in tooltip#447
lkipke wants to merge 8 commits intocloudflare:mainfrom
lkipke:levi/add-dimming-to-chart-tooltip

Conversation

@lkipke
Copy link
Copy Markdown
Contributor

@lkipke lkipke commented Apr 24, 2026

When hovering a series on a bar or line chart, the tooltip now dims rows for other series to 50% opacity, matching the emphasis dimming that ECharts already applies to the chart elements themselves.

Screenshot 2026-04-24 at 1 43 00 PM Screenshot 2026-04-24 at 1 43 07 PM Screenshot 2026-04-24 at 1 43 16 PM
  • Reviews
    • bonk has reviewed the change
    • automated review not possible because:
  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows: Verified dimming works for both line and bar charts (see screenshots)
    • Additional testing not necessary because:

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Apr 24, 2026

npm i https://pkg.pr.new/@cloudflare/kumo@447

commit: d49b1cd

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 24, 2026

Docs Preview

View docs preview

Commit: d49b1cd

Copy link
Copy Markdown
Collaborator

@mattrothenberg mattrothenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, didn't mean to approve. Still reviewing

@mattrothenberg
Copy link
Copy Markdown
Collaborator

/bonk My other AI Agent wants to know: Why drive tooltip-row dimming from element-level mouseover/mouseout and a seriesName ref, when axis-trigger tooltips aren't element-activated. Does this silently no-op on line charts with showSymbol: false, flicker between adjacent bars of the same series, and read a ref from inside the formatter that ECharts has no way to keep in sync with tooltip re-renders? Would updateAxisPointer with series-index matching not be the correct signal here?

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 24, 2026

@mattrothenberg Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

@mattrothenberg
Copy link
Copy Markdown
Collaborator

/bonk pls review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

This PR adds tooltip row dimming when hovering chart series. The visual goal is good, but the implementation has a critical synchronization issue that will cause the dimming to not work reliably or at all.

Critical Issues

1. Stale Closure in Tooltip Formatter (Primary Issue)

The dangerousHtmlFormatter is created inside useMemo and captures hoveredSeriesRef.current in its closure. When mouseover/mouseout update the ref, the formatter function does not get re-created because refs don't trigger re-renders and the useMemo dependencies don't include the hovered state.

Result: The formatter always sees the initial null value, so dimming never actually works.

2. Event Mismatch with Axis-Triggered Tooltips

The tooltip uses trigger: 'axis', meaning tooltips appear on axis hover, not element hover. However, mouseover/mouseout events fire when hovering specific graphical elements.

Problems:

  • Line charts with showSymbol: false: No symbols to hover, so mouseover events may not fire for the tooltip to dim rows
  • Bar charts: Rapidly moving between adjacent bars of the same series can cause flicker (mouseout clears, then mouseover sets)
  • The tooltip renders before/without element mouseover, leaving rows undimmed during axis hover

3. Incomplete Incomplete-Series Config

The incomplete-series config adds triggerLineEvent: true but this doesn't work for bar series (bar charts don't have lines). The hover tracking won't work consistently across chart types.

Suggested Approach

Consider using updateAxisPointer event instead, which fires consistently for axis-triggered tooltips. Match the axis pointer's series index to dim the correct tooltip row:

handlers.updateAxisPointer = (params: any) => {
  // params.dataIndex gives the hovered data index
  // Compare with series data to determine which series is 'active'
  hoveredSeriesRef.current = findSeriesAtDataIndex(params.dataIndex);
};

Or use React state with a throttled update to ensure the formatter re-renders when hover changes.

Questions

  1. Have you verified this works on line charts with showSymbol: false?
  2. Does the dimming update when hovering between bars of different series?
  3. Would the emphasis.focus: 'series' state from ECharts be more reliable than tracking mouse events?

Comment thread packages/kumo/src/components/chart/TimeseriesChart.tsx
Comment thread packages/kumo/src/components/chart/TimeseriesChart.tsx
Comment thread packages/kumo/src/components/chart/TimeseriesChart.tsx Outdated
Comment thread packages/kumo/src/components/chart/TimeseriesChart.tsx Outdated
Comment thread packages/kumo/src/components/chart/TimeseriesChart.tsx
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 24, 2026

I've submitted a comprehensive review of PR #447. Here's a summary of the key findings:

Key Issues Identified

1. Stale Closure Bug (Critical)

The dangerousHtmlFormatter captures hoveredSeriesRef.current in its closure inside useMemo. Since ref updates don't trigger re-renders, the formatter always sees the initial null value, meaning the dimming never actually works.

2. Event Mismatch with Axis-Triggered Tooltips

The tooltip uses trigger: 'axis' but the implementation relies on mouseover/mouseout events which fire on graphical elements. This creates problems:

  • Line charts with showSymbol: false: No symbols to hover, so events don't fire
  • Bar charts: Flickering when moving between adjacent bars
  • Axis hover: Tooltip can show without any element mouseover

3. triggerLineEvent Doesn't Work for Bar Charts

This configuration only applies to line series, so hover tracking won't work consistently across chart types.

Suggested Solution

Use the updateAxisPointer event (which fires consistently for axis-triggered tooltips) instead of mouseover/mouseout, and either:

  • Use React state with throttling to ensure the formatter re-renders
  • Or manually trigger chart.setOption() when hover changes

The review has been posted with inline comments on the relevant lines.

github run

@lkipke lkipke force-pushed the levi/add-dimming-to-chart-tooltip branch from 8882aa0 to ed7246e Compare April 24, 2026 14:27
@lkipke
Copy link
Copy Markdown
Contributor Author

lkipke commented Apr 28, 2026

/bonk My other AI Agent wants to know: Why drive tooltip-row dimming from element-level mouseover/mouseout and a seriesName ref, when axis-trigger tooltips aren't element-activated. Does this silently no-op on line charts with showSymbol: false, flicker between adjacent bars of the same series, and read a ref from inside the formatter that ECharts has no way to keep in sync with tooltip re-renders? Would updateAxisPointer with series-index matching not be the correct signal here?

The feedback suggested using updateAxisPointer with params.seriesIndex to identify the hovered series. The problem is that seriesIndex on updateAxisPointer doesn't work the way the reviewer assumed:

  • What the reviewer expected: params.seriesIndex tells you which series the cursor is closest to vertically (i.e., "the user is hovering near the P99 line, not the P50 line")
  • What actually happens: With axisPointer: { type: "shadow" } (which we use), seriesIndex is either undefined, always 0, or just indicates the first series at that axis position — it doesn't reflect vertical proximity to a specific line

The seriesIndex field on updateAxisPointer is really only meaningful when axisPointer.type is "cross" (crosshair mode), where ECharts actively tracks which series the crosshair is snapped to. With "shadow" or "line" axis pointer types, the pointer just highlights a time column — it doesn't distinguish between series vertically.

So the suggestion was directionally correct (use an axis-level event instead of element-level mouseover), but the specific mechanism (params.seriesIndex) doesn't provide the data needed for line charts.

For bar charts, the original mouseover/mouseout approach was always the right call since each stacked segment is a distinct hoverable element.

@lkipke
Copy link
Copy Markdown
Contributor Author

lkipke commented May 1, 2026

Note on line chart hover hit area: The hover target for line charts is the existing 2px line stroke — this is an ECharts limitation where symbolSize, lineStyle.width, and hit detection area are all coupled. This PR just adds tooltip dimming that activates whenever ECharts' own emphasis system fires, without changing the hit area.

(There is a related, but not quite the same, upstream issue for ECharts: apache/echarts#21280)

When hovering a series on a bar or line chart, the tooltip now dims
rows for other series to 50% opacity, matching the emphasis dimming
that ECharts already applies to the chart elements themselves.
Address review feedback:
- Replace mouseover/mouseout with updateAxisPointer + globalout for
  reliable hover tracking with axis-triggered tooltips
- Move tooltip value formatter to a ref to avoid stale closure in
  the useMemo'd dangerousHtmlFormatter
- Dispatch highlight/downplay actions to keep chart emphasis in sync
- Add updateAxisPointer event type to ChartEvents interface
Bar charts have distinct hoverable segments so mouseover/mouseout work
reliably. Line charts with showSymbol:false lack hover targets, so
updateAxisPointer (which provides the nearest seriesIndex) is used
instead. Both share globalout to clear state when leaving the chart.
Remove line chart hover tracking (ZRender mousemove, updateAxisPointer)
since line series with showSymbol: false lack reliable hover targets,
causing the tooltip dimming to be out of sync with the chart emphasis.
Bar chart dimming via mouseover/mouseout works reliably since each
stacked segment is a distinct hoverable element.
Expose ECharts' extraCssText and enterable tooltip options as
TimeseriesChart props. When tooltipEnterable is true, the tooltip
is positioned close to the cursor so users can mouse into it to
scroll or interact with the content.

Includes a docs demo with 8 series and a scrollable tooltip.
This event type was added during development but is no longer used —
bar chart tooltip dimming uses mouseover/mouseout instead.
Tooltip row dimming now works for all chart types via mouseover/mouseout.
When ECharts triggers emphasis on a series element, the tooltip dims
non-hovered rows to match. The hit area for line charts is unchanged
(tied to the 2px line stroke) — improving that is a separate concern.
@lkipke lkipke force-pushed the levi/add-dimming-to-chart-tooltip branch from def1616 to 0b73923 Compare May 1, 2026 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants