Skip to content

Commit b794e26

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 #7972 Task: 5953775 Signed-off-by: Pierre Rousseau (pro) <pro@odoo.com>
1 parent 5a46aed commit b794e26

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
@@ -228,11 +228,10 @@ export class SheetViewPlugin extends UIPlugin {
228228
this.sheetsWithDirtyViewports.add(cmd.sheetId);
229229
break;
230230
case "UPDATE_CELL":
231-
// update cell content or format can change hidden rows because of data filters
232-
if ("content" in cmd || "format" in cmd || cmd.style?.fontSize !== undefined) {
233-
for (const sheetId of this.getters.getSheetIds()) {
234-
this.sheetsWithDirtyViewports.add(sheetId);
235-
}
231+
// Either the content, format or style can impact the header sizes of a sheet
232+
// As such, every command can have a potential effect on the viewport
233+
for (const sheetId of this.getters.getSheetIds()) {
234+
this.sheetsWithDirtyViewports.add(sheetId);
236235
}
237236
break;
238237
case "DELETE_SHEET":

tests/autofill/autofill_plugin.test.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
merge,
1414
selectCell,
1515
setCellContent,
16+
setFormat,
1617
setSelection,
1718
} from "../test_helpers/commands_helpers";
1819
import {
@@ -716,4 +717,12 @@ describe("Autofill", () => {
716717
setCellContent(model, "A1", "[label](url)");
717718
expect(autofillTooltip("A1", "A2")).toBe("label");
718719
});
720+
721+
test("Autofill that triggers no change does not crash", () => {
722+
addDataValidation(model, "A1:A2", "1", { type: "isBoolean", values: [] });
723+
setCellContent(model, "A1", "FALSE");
724+
setCellContent(model, "A2", "FALSE");
725+
setFormat(model, "0.00%", [toZone("A1:A2")]);
726+
autofill("A1", "A2"); // A1 and A2 are exactly the same
727+
});
719728
});

tests/sheet/sheetview_plugin.test.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -895,7 +895,18 @@ describe("Viewport of Simple sheet", () => {
895895
right: 10,
896896
top: 0,
897897
});
898-
setStyle(model, "A1:A20", { fontSize: 8 });
898+
setStyle(model, "A1:A20", { fontSize: 24 });
899+
expect(model.getters.getActiveMainViewport()).toEqual({
900+
bottom: 30,
901+
left: 0,
902+
right: 10,
903+
top: 0,
904+
});
905+
906+
const sheetId = model.getters.getActiveSheetId();
907+
for (let i = 0; i < 20; ++i) {
908+
model.dispatch("UPDATE_CELL", { sheetId, col: 0, row: i, style: {} });
909+
}
899910
expect(model.getters.getActiveMainViewport()).toEqual({
900911
bottom: 43,
901912
left: 0,

0 commit comments

Comments
 (0)