Skip to content

Commit 51860c0

Browse files
committed
[FIX] SheetView: dirtify sheet viewport at UPDATE_CELL
the issue was found through an autofill bug. To reproduce: - Add a data validation checkbox to A1:A2 - Add a format (like `%`) to A1:A2 - autofill A1 into A2 -> crash What happens? For starters, the plugin `SheetViewPlugin` is always listening to selection events and will update the viewports 'scroll' value accordingly. During the autofill handling of the command, a bunch of `UPDATE_CELL` commands are dispatched, which invalidates the whole state of `HeaderPositionsUIPlugin`. After that dispatch, the plugin also updates the selection, which triggers `SheetViewPlugin` to update its viewports but this manipulation relies on the state of `HeaderPositionsUIPlugin`, which was just invalidated! It was already known that during a command dispatch, the general state of plugins might be unstable and `SheetViewPlugin` accounted for it by *tagging* sheets that might have an unstable state and it would skip the viewport `scroll` update after selection events. However, we limited the *tagging* for specific payloads of `UPDATE_CELL` and the condition set on the style was incorrect. Indeed, a style object without the `fontSize` does not mean that we do not update the fontsize but rather that we reset it to its default value which has a direct impact on the header size and position (hence the invalidation of state in `HeaderPositionsUIPlugin`. All-in-all, The presence of the `style` key in the `UPDATE_CELL` command payload is enough to tag a sheet as invalid. A side note: While changing the content or format of a cell can have cross-sheet impact due to the evaluation process (a cell will take the format of its reference without a format on its own), it is not the case for the style. As such, we could make separate cases for commands that update the content/format, which might theoretically impact every sheet, and commands that only impact the style, which only impacts the targetted sheet. Such improvement can target the master dev branch. closes #8178 Task: 5953775 X-original-commit: cdc1728 Signed-off-by: Pierre Rousseau (pro) <pro@odoo.com> Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
1 parent e4ff47c commit 51860c0

File tree

4 files changed

+27
-11
lines changed

4 files changed

+27
-11
lines changed

packages/o-spreadsheet-engine/src/plugins/ui_stateful/header_positions.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ export class HeaderPositionsUIPlugin extends UIPlugin {
2121
this.headerPositions[sheetId] = this.computeHeaderPositionsOfSheet(sheetId);
2222
}
2323
break;
24+
// Either the content, format or style can impact the header sizes of a sheet
25+
// As such, every command can have a potential effect on the viewport
2426
case "UPDATE_CELL":
2527
case "SET_FORMATTING":
2628
case "CLEAR_FORMATTING":

packages/o-spreadsheet-engine/src/plugins/ui_stateful/sheetview.ts

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -251,16 +251,10 @@ export class SheetViewPlugin extends UIPlugin {
251251
break;
252252
case "UPDATE_CELL":
253253
case "SET_FORMATTING":
254-
// update cell content or format can change hidden rows because of data filters
255-
if (
256-
"content" in cmd ||
257-
"format" in cmd ||
258-
cmd.style?.fontSize !== undefined ||
259-
cmd.style?.wrapping !== undefined
260-
) {
261-
for (const sheetId of this.getters.getSheetIds()) {
262-
this.sheetsWithDirtyViewports.add(sheetId);
263-
}
254+
// Either the content, format or style can impact the header sizes of a sheet
255+
// As such, every command can have a potential effect on the viewport
256+
for (const sheetId of this.getters.getSheetIds()) {
257+
this.sheetsWithDirtyViewports.add(sheetId);
264258
}
265259
break;
266260
case "DELETE_SHEET":

tests/autofill/autofill_plugin.test.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
merge,
1313
selectCell,
1414
setCellContent,
15+
setFormat,
1516
setSelection,
1617
} from "../test_helpers/commands_helpers";
1718
import {
@@ -982,4 +983,12 @@ describe("Autofill", () => {
982983
setCellContent(model, "A1", "[label](url)");
983984
expect(autofillTooltip("A1", "A2")).toBe("label");
984985
});
986+
987+
test("Autofill that triggers no change does not crash", () => {
988+
addDataValidation(model, "A1:A2", "1", { type: "isBoolean", values: [] });
989+
setCellContent(model, "A1", "FALSE");
990+
setCellContent(model, "A2", "FALSE");
991+
setFormat(model, "A1:A2", "0.00%");
992+
autofill("A1", "A2"); // A1 and A2 are exactly the same
993+
});
985994
});

tests/sheet/sheetview_plugin.test.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -907,7 +907,18 @@ describe("Viewport of Simple sheet", () => {
907907
right: 10,
908908
top: 0,
909909
});
910-
setStyle(model, "A1:A20", { fontSize: 8 });
910+
setStyle(model, "A1:A20", { fontSize: 24 });
911+
expect(model.getters.getActiveMainViewport()).toEqual({
912+
bottom: 30,
913+
left: 0,
914+
right: 10,
915+
top: 0,
916+
});
917+
918+
const sheetId = model.getters.getActiveSheetId();
919+
for (let i = 0; i < 20; ++i) {
920+
model.dispatch("UPDATE_CELL", { sheetId, col: 0, row: i, style: {} });
921+
}
911922
expect(model.getters.getActiveMainViewport()).toEqual({
912923
bottom: 43,
913924
left: 0,

0 commit comments

Comments
 (0)