[FIX] viewport: infinite loop with hidden headers

When we move the selection we scroll the viewport so that new selection is
visible. Previously, the strategy to do this was (when moving up):

1) set the viewport offset to the row above the current topmost row
2) adapt the viewport top/bottom to the new offset
3) repeat while the selection is not visible

This doesn't work if the row above the current topmost row is hidden, since
it have the same position has the current row. This led to an infinite loop
with the structure of the code.

With this commit, the hidden rows are skipped in step 1). The loop scrolling
the viewport was also rewritten, so it's impossible to have an
infinite loop even if there is a bug somewhere.

Odoo task 3244431

closes #2342

X-original-commit: e2c9659
Signed-off-by: Rémi Rahir (rar) <[email protected]>
Signed-off-by: Minne Adrien (adrm) <[email protected]>

src/helpers/internal_viewport.ts

@@ -139,53 +139,79 @@ export class InternalViewport {
     }
   }
 
-  adjustPositionX(col: HeaderIndex) {
+  adjustPositionX(targetCol: HeaderIndex) {
     const sheetId = this.sheetId;
-    const { start, end } = this.getters.getColDimensions(sheetId, col);
-    while (
-      end > this.offsetX + this.offsetCorrectionX + this.viewportWidth &&
-      this.offsetX + this.offsetCorrectionX < start
-    ) {
-      this.offsetX = this.getters.getColDimensions(sheetId, this.left).end - this.offsetCorrectionX;
-      this.offsetScrollbarX = this.offsetX;
-      this.adjustViewportZoneX();
-    }
-    while (col < this.left) {
-      let leftCol: HeaderIndex;
-      for (leftCol = this.left; leftCol >= 0; leftCol--) {
-        if (!this.getters.isColHidden(sheetId, leftCol)) {
+    const { end } = this.getters.getColDimensions(sheetId, targetCol);
+    const maxCol = this.getters.getNumberCols(sheetId);
+
+    if (this.offsetX + this.offsetCorrectionX + this.viewportWidth < end) {
+      for (
+        let col = this.left;
+        this.offsetX + this.offsetCorrectionX + this.viewportWidth < end;
+        col++
+      ) {
+        if (col > maxCol) {
           break;
         }
+        if (this.getters.isColHidden(sheetId, col)) {
+          continue;
+        }
+
+        this.offsetX = this.getters.getColDimensions(sheetId, col).end - this.offsetCorrectionX;
+        this.offsetScrollbarX = this.offsetX;
+        this.adjustViewportZoneX();
+      }
+    } else if (this.left > targetCol) {
+      for (let col = this.left; col >= targetCol; col--) {
+        if (col < 0) {
+          break;
+        }
+        if (this.getters.isColHidden(sheetId, col)) {
+          continue;
+        }
+
+        this.offsetX = this.getters.getColDimensions(sheetId, col).start - this.offsetCorrectionX;
+        this.offsetScrollbarX = this.offsetX;
+        this.adjustViewportZoneX();
       }
-      this.offsetX =
-        this.getters.getColDimensions(sheetId, leftCol - 1).start - this.offsetCorrectionX;
-      this.offsetScrollbarX = this.offsetX;
-      this.adjustViewportZoneX();
     }
   }
 
-  adjustPositionY(row: HeaderIndex) {
+  adjustPositionY(targetRow: HeaderIndex) {
     const sheetId = this.sheetId;
-    while (
-      this.getters.getRowDimensions(sheetId, row).end >
-        this.offsetY + this.viewportHeight + this.offsetCorrectionY &&
-      this.offsetY + this.offsetCorrectionY < this.getters.getRowDimensions(sheetId, row).start
-    ) {
-      this.offsetY = this.getters.getRowDimensions(sheetId, this.top).end - this.offsetCorrectionY;
-      this.offsetScrollbarY = this.offsetY;
-      this.adjustViewportZoneY();
-    }
-    while (row < this.top) {
-      let topRow: HeaderIndex;
-      for (topRow = this.top; topRow >= 0; topRow--) {
-        if (!this.getters.isRowHidden(sheetId, topRow)) {
+    const { end } = this.getters.getRowDimensions(sheetId, targetRow);
+    const maxRow = this.getters.getNumberRows(sheetId);
+
+    if (this.offsetY + this.viewportHeight + this.offsetCorrectionY < end) {
+      for (
+        let row = this.top;
+        this.offsetY + this.viewportHeight + this.offsetCorrectionY < end;
+        row++
+      ) {
+        if (row > maxRow) {
           break;
         }
+        if (this.getters.isRowHidden(sheetId, row)) {
+          continue;
+        }
+
+        this.offsetY = this.getters.getRowDimensions(sheetId, row).end - this.offsetCorrectionY;
+        this.offsetScrollbarY = this.offsetY;
+        this.adjustViewportZoneY();
+      }
+    } else if (this.top > targetRow) {
+      for (let row = this.top; row >= targetRow; row--) {
+        if (row < 0) {
+          break;
+        }
+        if (this.getters.isRowHidden(sheetId, row)) {
+          continue;
+        }
+
+        this.offsetY = this.getters.getRowDimensions(sheetId, row).start - this.offsetCorrectionY;
+        this.offsetScrollbarY = this.offsetY;
+        this.adjustViewportZoneY();
       }
-      this.offsetY =
-        this.getters.getRowDimensions(sheetId, topRow - 1).start - this.offsetCorrectionY;
-      this.offsetScrollbarY = this.offsetY;
-      this.adjustViewportZoneY();
     }
   }
 

tests/plugins/selection.test.ts

@@ -1,5 +1,12 @@
 import { DEFAULT_CELL_HEIGHT, DEFAULT_CELL_WIDTH } from "../../src/constants";
-import { positionToZone, toCartesian, toZone, zoneToXc } from "../../src/helpers";
+import {
+  numberToLetters,
+  positionToZone,
+  toCartesian,
+  toXC,
+  toZone,
+  zoneToXc,
+} from "../../src/helpers";
 import { Model } from "../../src/model";
 import { CommandResult } from "../../src/types";
 import { SelectionDirection } from "../../src/types/selection";
@@ -25,6 +32,7 @@ import {
   selectRow,
   setAnchorCorner,
   setSelection,
+  setViewportOffset,
   undo,
 } from "../test_helpers/commands_helpers";
 import { getActivePosition, getSelectionAnchorCellXc } from "../test_helpers/getters_helpers";
@@ -290,6 +298,58 @@ describe("simple selection", () => {
     expect(model.getters.getSelectedZone()).toEqual(toZone("A5"));
   });
 
+  test("move selection left through hidden cols, with scrolling", () => {
+    const model = new Model({ sheets: [{ colNumber: 100, rowNumber: 1 }] });
+    hideColumns(model, ["B"]);
+    selectCell(model, "C1");
+    setViewportOffset(model, DEFAULT_CELL_WIDTH, 0);
+
+    moveAnchorCell(model, "left");
+    expect(model.getters.getSelectedZone()).toEqual(toZone("A1"));
+    expect(model.getters.getActiveSheetScrollInfo()).toEqual({ scrollX: 0, scrollY: 0 });
+  });
+
+  test("move selection right through hidden cols, with scrolling", () => {
+    const model = new Model({ sheets: [{ colNumber: 100, rowNumber: 1 }] });
+    const visibleCols = model.getters.getSheetViewVisibleCols();
+    const lastVisibleCol = visibleCols[visibleCols.length - 1];
+    hideColumns(model, [numberToLetters(lastVisibleCol + 1), numberToLetters(lastVisibleCol + 2)]);
+    selectCell(model, toXC(lastVisibleCol, 0));
+    moveAnchorCell(model, "right");
+
+    expect(model.getters.getSelectedZone()).toEqual(toZone(toXC(lastVisibleCol + 3, 0)));
+    expect(model.getters.getActiveSheetScrollInfo()).toEqual({
+      scrollX: DEFAULT_CELL_WIDTH * 2,
+      scrollY: 0,
+    });
+  });
+
+  test("move selection up through hidden rows, with scrolling", () => {
+    const model = new Model({ sheets: [{ colNumber: 1, rowNumber: 100 }] });
+    hideRows(model, [1]);
+    selectCell(model, "A3");
+    setViewportOffset(model, 0, DEFAULT_CELL_HEIGHT);
+
+    moveAnchorCell(model, "up");
+    expect(model.getters.getSelectedZone()).toEqual(toZone("A1"));
+    expect(model.getters.getActiveSheetScrollInfo()).toEqual({ scrollX: 0, scrollY: 0 });
+  });
+
+  test("move selection down through hidden cols, with scrolling", () => {
+    const model = new Model({ sheets: [{ colNumber: 1, rowNumber: 100 }] });
+    const visibleRows = model.getters.getSheetViewVisibleRows();
+    const lastVisibleRow = visibleRows[visibleRows.length - 1];
+    hideRows(model, [lastVisibleRow + 1, lastVisibleRow + 2]);
+    selectCell(model, toXC(0, lastVisibleRow));
+
+    moveAnchorCell(model, "down");
+    expect(model.getters.getSelectedZone()).toEqual(toZone(toXC(0, lastVisibleRow + 3)));
+    expect(model.getters.getActiveSheetScrollInfo()).toEqual({
+      scrollX: 0,
+      scrollY: DEFAULT_CELL_HEIGHT * 2,
+    });
+  });
+
   test("can select a whole column", () => {
     const model = new Model({
       sheets: [