Repository navigation
fix: Skip redundant point setState calls when highlighting column series - #277
Conversation
cd6fa33 to
f5132c8
Compare
f5132c8 to
89ae89e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Pie-chart highlighting can leave the shared halo on the wrong item when returning to a previously highlighted point, requiring unconditional state application and a regression test.
1 open issue
What changed in this PR
This PR reduces redundant point-state updates in chart highlighting while preserving state restoration after data changes and re-renders.
Changes:
- Skip state applications when both Highcharts and stored point states match the target.
- Force state reapplication when restoring highlights after re-render.
- Add tests for call counts, rendered highlights, data changes, pinned tooltips, and legend interactions.
| File | Description |
|---|---|
src/core/chart-api/chart-extra-highlight.ts |
Adds redundant-state checks and forced highlight restoration. |
src/core/__tests__/chart-core-highlight.test.tsx |
Adds column highlight performance and visual-state regression tests. |
Review effort: Balanced
89ae89e to
88375e3
Compare
| // The setState(state, true) call re-applies the state in the DOM even when it is unchanged, so for column series | ||
| // it is skipped for points that already have the target state, both in Highcharts and in the stored state for the | ||
| // current point key. A missing or different stored state means the point might have been redrawn (e.g. after its | ||
| // data changed). Other series types are always updated, because shared series graphics (such as the pie halo) |
There was a problem hiding this comment.
Do we have other examples than pie halo? If only pie chart is an outlier - should we aim to make the optimisation for every other series - so not just "column"?
| expect(rendered()).toEqual(highlighted(1, 0)); | ||
| }); | ||
|
|
||
| test("re-applies rendered point state after re-render", () => { |
There was a problem hiding this comment.
How is this different from "restores point highlight after re-render"?
| }, | ||
| ); | ||
|
|
||
| describe("column point state", () => { |
There was a problem hiding this comment.
Can we have less new tests? These go very deep into implementation detail and can be tricky to maintain. I am not even sure if we need one that validates highlight for the column series as there is an existing "highlights a single group/stack, stacked=$stacked" test. As a minimal regression cov - we can include a test that validates that setState is only called once per point - which is exactly the change introduced by the PR.
88375e3 to
a7bbf94
Compare
| // the state after re-render, and for pie charts, where the shared halo follows the last setState call. | ||
| private setPointState(point: Highcharts.Point, state: "" | Highcharts.PointStateValue) { | ||
| (point.setState as PointSetStateWithLock)[SET_STATE_LOCK] = false; | ||
| point.setState(state, true); |
There was a problem hiding this comment.
For column series it is not enough to change the state on the series - so we have to go over many points. I noticed that if we change the 2nd argument here to false - the performance greatly improves: Highcharts avoids some extra work in this case. I believe we can use it to our advantage for column series, e.g.:
public onChartRender() {
this.columnStatesSynced = false;
//...
}
public highlightChartGroup = (points: readonly Highcharts.Point[]) => {
// ...
for (const s of getChartSeries(this.context.chart())) {
// ...
if (s.type === "column") {
// ...
this.setPointState(d, includedPoints.has(d) ? "" : "inactive", !this.columnStatesSynced);
}
this.columnStatesSynced = true;
};
public highlightChartPoint = (point: Highcharts.Point) => {
// same changes
};
private setPointState(point: Highcharts.Point, state: "" | Highcharts.PointStateValue, move = true) {
(point.setState as PointSetStateWithLock)[SET_STATE_LOCK] = false;
point.setState(state, move);
this.updateStoredPointState(point, state);
(point.setState as PointSetStateWithLock)[SET_STATE_LOCK] = true;
}
Could you please check if that or a similar approach works? This introduces less complexity and seems to produce about the same result.
e0cf3d5 to
c87d944
Compare
| private columnStatesSynced = false; | ||
|
|
||
| public onChartRender() { | ||
| this.columnStatesSynced = false; |
| private setPointState(point: Highcharts.Point, state: "" | Highcharts.PointStateValue, move = true) { | ||
| (point.setState as PointSetStateWithLock)[SET_STATE_LOCK] = false; | ||
| point.setState(state, true); | ||
| point.setState(state, move); |
Description
Hovering a column chart re-applies the highlight state to every column point on each hover. On a 24 × 180 stacked column chart (4,320 points) that is 4,320 forced
point.setStatecalls per hover, ~100 ms per hover.Root cause: why columns only
highlightChartPointandhighlightChartGrouptherefore looped over every point of every column series on each hover.setState(state, true). Themove = trueargument bypasses Highcharts' own "already has this state" early return, so every unchanged point was redrawn.series.setState): no per-point loop, and 0 pointsetStatecalls per hover. Pie charts do loop per point, but have few points, and they need the forced call because the shared halo follows the lastsetState.So the cost comes from our implementation (a full loop and a forced re-apply), not from a Highcharts bug. The only Highcharts-side item is the missing column-group highlight option (#23076), which is what made the workaround necessary. That is a possible upstream follow-up, not a bug report.
Fix
Approach suggested in review.
highlightChartPointandhighlightChartGroupstill visit every column point. After the first highlight following a render, they callsetState(state, false)instead ofsetState(state, true), so Highcharts returns early for points whose state is unchanged and only redraws the points that actually change.columnStatesSyncedflag tracks this. It is reset inonChartRender, on every render, so the next highlight re-applies every column point withmove = true. This is needed aftersetDataand other updates, where Highcharts may have reset or recreated points.move = trueand the existing code paths.Tests
One new unit test, "forces column point state updates only on the first highlight after render", in
src/core/__tests__/chart-core-highlight.test.tsx. It spies onPoint.prototype.setStateon a chart with two column series and checks themoveargument:setStatewithmove = trueonly. The next highlight usesmove = falseonly.move = trueagain, and the following one usesmove = false.The test fails when the reset in
onChartRenderis removed, whenmoveis alwaysfalse, and onmain.The column group highlight rendering is already covered by the existing "highlights a single group/stack, stacked=$stacked" test. There is no separate re-render restore test on
main; the new test covers that the first highlight after a re-render re-applies every column point. Pie behaviour is unchanged, so this PR adds no pie tests.Verification
mainon Highcharts 12.5.0, for column (single, stacked, grouped), line, area, scatter, pie and donut, plus core API scenarios for column and pie: 760 steps, 0 mismatches. Control: with theonChartRenderreset removed, the same harness reports 28 mismatches.mainagainst this PR:setStatetime per hover: 36.2 ms against 1.2 ms.How has this been tested?
Unit tests (full suite),
tsc, eslint, prettier, and the equivalence and perf runs above.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Review checklist