Skip to content

Commit cdc1728

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 #8169 Task: 5953775 X-original-commit: b794e26 Signed-off-by: Pierre Rousseau (pro) <pro@odoo.com> Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
1 parent c98ebc5 commit cdc1728

File tree

4 files changed

+27
-6
lines changed

4 files changed

+27
-6
lines changed

src/plugins/ui_stateful/header_positions.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ export class HeaderPositionsUIPlugin extends UIPlugin {
2323
}
2424
break;
2525
case "UPDATE_CELL":
26+
// Either the content, format or style can impact the header sizes of a sheet
27+
// As such, every command can have a potential effect on the viewport
2628
this.headerPositions = {};
2729
this.isDirty = true;
2830
break;

src/plugins/ui_stateful/sheetview.ts

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -235,11 +235,10 @@ export class SheetViewPlugin extends UIPlugin {
235235
this.sheetsWithDirtyViewports.add(cmd.sheetId);
236236
break;
237237
case "UPDATE_CELL":
238-
// update cell content or format can change hidden rows because of data filters
239-
if ("content" in cmd || "format" in cmd || cmd.style?.fontSize !== undefined) {
240-
for (const sheetId of this.getters.getSheetIds()) {
241-
this.sheetsWithDirtyViewports.add(sheetId);
242-
}
238+
// Either the content, format or style can impact the header sizes of a sheet
239+
// As such, every command can have a potential effect on the viewport
240+
for (const sheetId of this.getters.getSheetIds()) {
241+
this.sheetsWithDirtyViewports.add(sheetId);
243242
}
244243
break;
245244
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 {
@@ -951,4 +952,12 @@ describe("Autofill", () => {
951952
setCellContent(model, "A1", "[label](url)");
952953
expect(autofillTooltip("A1", "A2")).toBe("label");
953954
});
955+
956+
test("Autofill that triggers no change does not crash", () => {
957+
addDataValidation(model, "A1:A2", "1", { type: "isBoolean", values: [] });
958+
setCellContent(model, "A1", "FALSE");
959+
setCellContent(model, "A2", "FALSE");
960+
setFormat(model, "A1:A2", "0.00%");
961+
autofill("A1", "A2"); // A1 and A2 are exactly the same
962+
});
954963
});

tests/sheet/sheetview_plugin.test.ts

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

0 commit comments

Comments
 (0)