Skip to content

Commit c039286

Browse files
committed
[FIX] side panel: close selection input when switching tab
When focusing the selection input in the chart side panel, changing tab would not close the selection input, even though the input isn't visible anymore. It's because since 91c0583, changing the tab would keep the component mounted, only hiding it with `d-none` (to keep its state). Relying on the selection input `onWillUnmount` to reset the state is not enough. We now rely on a `useEffect` that checks the `input.offsetParent` property, which is `null` when an ancestor has `display: none`. Task: 5249165
1 parent d59fcbf commit c039286

File tree

3 files changed

+42
-0
lines changed

3 files changed

+42
-0
lines changed

src/components/selection_input/selection_input.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,13 @@ export class SelectionInput extends Component<Props, SpreadsheetChildEnv> {
129129
() => this.focusedInput.el?.focus(),
130130
() => [this.focusedInput.el]
131131
);
132+
useEffect(() => {
133+
// Check the offsetParent to know if the input or an ancestor is `display: none` (eg. when changing side panel tab)
134+
if (this.store.hasFocus && this.selectionRef.el?.offsetParent === null) {
135+
this.reset();
136+
}
137+
});
138+
132139
this.store = useLocalStore(
133140
SelectionInputStore,
134141
this.props.ranges,

tests/figures/chart/charts_component.test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
import { toHex, toZone } from "../../../src/helpers";
1212
import { ScorecardChart } from "../../../src/helpers/figures/charts";
1313
import { getChartColorsGenerator } from "../../../src/helpers/figures/charts/runtime";
14+
import { HighlightStore } from "../../../src/stores/highlight_store";
1415
import {
1516
CHART_TYPES,
1617
ChartDefinition,
@@ -1128,6 +1129,22 @@ describe("charts", () => {
11281129
expect(chartPanel.scrollTop).toBe(100);
11291130
});
11301131

1132+
test("selection input is closed when switching tab", async () => {
1133+
createTestChart("basicChart");
1134+
await mountChartSidePanel();
1135+
const highlightStore = env.getStore(HighlightStore);
1136+
1137+
await simulateClick(".o-selection-input input");
1138+
expect(".o-selection-ok").toHaveCount(1);
1139+
expect(highlightStore.highlights.length).toBe(1);
1140+
1141+
await openChartDesignSidePanel(model, env, fixture, chartId);
1142+
await nextTick(); // the check is done in a `useEffect`, we need to wait for the next render
1143+
1144+
expect(".o-selection-ok").toHaveCount(0);
1145+
expect(highlightStore.highlights.length).toBe(0);
1146+
});
1147+
11311148
describe.each(TEST_CHART_TYPES)("selecting other chart will adapt sidepanel", (chartType) => {
11321149
test.each(["click", "SELECT_FIGURE command"])("when using %s", async (selectMethod: string) => {
11331150
createTestChart(chartType);

tests/setup/jest.setup.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,24 @@ beforeEach(() => {
8787
const blob = new window.Blob([data], { type });
8888
setTimeout(() => callback(blob), 0);
8989
});
90+
91+
// offsetParent should return the nearest positioned ancestor, or null if an ancestor has `display: none`
92+
jest
93+
.spyOn(HTMLElement.prototype, "offsetParent", "get")
94+
.mockImplementation(function (this: HTMLElement) {
95+
let parentEl: HTMLElement | null = this.parentElement;
96+
while (parentEl) {
97+
if (
98+
getComputedStyle(parentEl).display === "none" ||
99+
parentEl.classList.contains("d-none")
100+
) {
101+
return null;
102+
}
103+
parentEl = parentEl.parentElement;
104+
}
105+
return this.parentElement; // should be nearest positioned ancestor, but simplified for tests
106+
});
107+
90108
patchSessionMove();
91109
});
92110

0 commit comments

Comments
 (0)