[FIX] selection: Altering selection should scroll the viewport

Commit b437f44 introduced an error. while fixing the viewport
scrolling while in autofill (it shouldn't happen) it also prevent an
change of selection (not an extension) to properly scroll the viewport.

The problem ultimately lies in the fact that the autofill was using the
selectionProcessor to override its selection instead of extending it.

In order to fix this, it was necessary to alter the API of the
selection processor to allow more granularity in the steps to extend a
zone selection: i.e. instead of a step of 1 or jump to the end of a zone,
we need to support all POSITIVE values of steps.

Task 3050101

closes #1824

X-original-commit: b167b3e
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>

src/components/composer/composer/composer.ts

@@ -267,7 +267,7 @@ export class Composer extends Component<Props, SpreadsheetChildEnv> {
 
     const direction = ev.shiftKey ? "left" : "right";
     this.env.model.dispatch("STOP_EDITION");
-    this.env.model.selection.moveAnchorCell(direction, "one");
+    this.env.model.selection.moveAnchorCell(direction, 1);
   }
 
   private processEnterKey(ev: KeyboardEvent) {
@@ -284,7 +284,7 @@ export class Composer extends Component<Props, SpreadsheetChildEnv> {
     }
     this.env.model.dispatch("STOP_EDITION");
     const direction = ev.shiftKey ? "up" : "down";
-    this.env.model.selection.moveAnchorCell(direction, "one");
+    this.env.model.selection.moveAnchorCell(direction, 1);
   }
 
   private processEscapeKey() {

src/components/grid/grid.ts

@@ -156,8 +156,8 @@ export class Grid extends Component<Props, SpreadsheetChildEnv> {
         ? this.props.onGridComposerCellFocused()
         : this.props.onComposerContentFocused();
     },
-    TAB: () => this.env.model.selection.moveAnchorCell("right", "one"),
-    "SHIFT+TAB": () => this.env.model.selection.moveAnchorCell("left", "one"),
+    TAB: () => this.env.model.selection.moveAnchorCell("right", 1),
+    "SHIFT+TAB": () => this.env.model.selection.moveAnchorCell("left", 1),
     F2: () => {
       const cell = this.env.model.getters.getActiveCell();
       cell.type === CellValueType.empty

src/components/helpers/selection_helpers.ts

@@ -13,8 +13,8 @@ export function updateSelectionWithArrowKeys(
 ) {
   const direction = arrowMap[ev.key];
   if (ev.shiftKey) {
-    selection.resizeAnchorZone(direction, ev.ctrlKey ? "end" : "one");
+    selection.resizeAnchorZone(direction, ev.ctrlKey ? "end" : 1);
   } else {
-    selection.moveAnchorCell(direction, ev.ctrlKey ? "end" : "one");
+    selection.moveAnchorCell(direction, ev.ctrlKey ? "end" : 1);
   }
 }

src/helpers/zones.ts

@@ -1,5 +1,5 @@
 import { _lt } from "../translation";
-import { Position, UnboundedZone, Viewport, Zone, ZoneDimension } from "../types";
+import { Position, UnboundedZone, Zone, ZoneDimension } from "../types";
 import { lettersToNumber, numberToLetters, toCartesian, toXC } from "./coordinates";
 import { range } from "./misc";
 import { isColReference, isRowReference } from "./references";
@@ -599,7 +599,7 @@ export function mergeOverlappingZones(zones: Zone[]) {
  * This function will compare the modifications of selection to determine
  * a cell that is part of the new zone and not the previous one.
  */
-export function findCellInNewZone(oldZone: Zone, currentZone: Zone, viewport: Viewport): Position {
+export function findCellInNewZone(oldZone: Zone, currentZone: Zone): Position {
   let col: number, row: number;
   const { left: oldLeft, right: oldRight, top: oldTop, bottom: oldBottom } = oldZone!;
   const { left, right, top, bottom } = currentZone;
@@ -609,15 +609,15 @@ export function findCellInNewZone(oldZone: Zone, currentZone: Zone, viewport: Vi
     col = right;
   } else {
     // left and right don't change
-    col = viewport.left > left || left > viewport.right ? viewport.left : left;
+    col = left;
   }
   if (top != oldTop) {
     row = top;
   } else if (bottom != oldBottom) {
     row = bottom;
   } else {
     // top and bottom don't change
-    row = viewport.top > top || top > viewport.bottom ? viewport.top : top;
+    row = top;
   }
   return { col, row };
 }

src/plugins/ui_core_views/sheetview.ts

@@ -152,13 +152,20 @@ export class SheetViewPlugin extends UIPlugin {
       case "AlterZoneCorner":
         break;
       case "ZonesSelected":
-        // altering a zone should not move the viewport
+        let { col, row } = findCellInNewZone(event.previousAnchor.zone, event.anchor.zone);
+        if (event.mode === "updateAnchor") {
+          const oldZone = event.previousAnchor.zone;
+          const newZone = event.anchor.zone;
+          // altering a zone should not move the viewport in a dimension that wasn't changed
+          const { top, bottom, left, right } = this.getters.getActiveMainViewport();
+          if (oldZone.left === newZone.left && oldZone.right === newZone.right) {
+            col = left > col || col > right ? left : col;
+          }
+          if (oldZone.top === newZone.top && oldZone.bottom === newZone.bottom) {
+            row = top > row || row > bottom ? top : row;
+          }
+        }
         const sheetId = this.getters.getActiveSheetId();
-        let { col, row } = findCellInNewZone(
-          event.previousAnchor.zone,
-          event.anchor.zone,
-          this.getters.getActiveMainViewport()
-        );
         col = Math.min(col, this.getters.getNumberCols(sheetId) - 1);
         row = Math.min(row, this.getters.getNumberRows(sheetId) - 1);
         this.refreshViewport(this.getters.getActiveSheetId(), { col, row });

src/plugins/ui_feature/autofill.ts

@@ -1,4 +1,4 @@
-import { clip, isInside, toCartesian, toXC, union } from "../../helpers/index";
+import { clip, isInside, toCartesian, toXC } from "../../helpers/index";
 import { autofillModifiersRegistry, autofillRulesRegistry } from "../../registries/index";
 import {
   AutofillData,
@@ -85,6 +85,7 @@ export class AutofillPlugin extends UIPlugin {
   static getters = ["getAutofillTooltip"] as const;
 
   private autofillZone: Zone | undefined;
+  private steps: number | undefined;
   private lastCellSelected: { col?: number; row?: number } = {};
   private direction: DIRECTION | undefined;
   private tooltip: Tooltip | undefined;
@@ -167,7 +168,7 @@ export class AutofillPlugin extends UIPlugin {
    *              useful to set it to false when we need to fill the tooltip
    */
   private autofill(apply: boolean) {
-    if (!this.autofillZone || this.direction === undefined) {
+    if (!this.autofillZone || !this.steps || this.direction === undefined) {
       this.tooltip = undefined;
       return;
     }
@@ -226,12 +227,12 @@ export class AutofillPlugin extends UIPlugin {
     }
 
     if (apply) {
-      const zone = union(this.getters.getSelectedZone(), this.autofillZone);
       this.autofillZone = undefined;
+      this.selection.resizeAnchorZone(this.direction, this.steps);
       this.lastCellSelected = {};
       this.direction = undefined;
+      this.steps = 0;
       this.tooltip = undefined;
-      this.selection.selectZone({ cell: { col: zone.left, row: zone.top }, zone });
     }
   }
 
@@ -248,15 +249,19 @@ export class AutofillPlugin extends UIPlugin {
     switch (this.direction) {
       case DIRECTION.UP:
         this.saveZone(row, source.top - 1, source.left, source.right);
+        this.steps = source.top - row;
         break;
       case DIRECTION.DOWN:
         this.saveZone(source.bottom + 1, row, source.left, source.right);
+        this.steps = row - source.bottom;
         break;
       case DIRECTION.LEFT:
         this.saveZone(source.top, source.bottom, col, source.left - 1);
+        this.steps = source.left - col;
         break;
       case DIRECTION.RIGHT:
         this.saveZone(source.top, source.bottom, source.right + 1, col);
+        this.steps = col - source.right;
         break;
     }
     this.autofill(false);

src/selection_stream/selection_stream_processor.ts

@@ -147,7 +147,10 @@ export class SelectionStreamProcessor
   /**
    * Set the selection to one of the cells adjacent to the current anchor cell.
    */
-  moveAnchorCell(direction: SelectionDirection, step: SelectionStep = "one"): DispatchResult {
+  moveAnchorCell(direction: SelectionDirection, step: SelectionStep = 1): DispatchResult {
+    if (step !== "end" && step <= 0) {
+      return new DispatchResult(CommandResult.InvalidSelectionStep);
+    }
     const { col, row } = this.getNextAvailablePosition(direction, step);
     return this.selectCell(col, row);
   }
@@ -193,7 +196,10 @@ export class SelectionStreamProcessor
    * The anchor cell remains where it is. It's the opposite side
    * of the anchor zone which moves.
    */
-  resizeAnchorZone(direction: SelectionDirection, step: SelectionStep = "one"): DispatchResult {
+  resizeAnchorZone(direction: SelectionDirection, step: SelectionStep = 1): DispatchResult {
+    if (step !== "end" && step <= 0) {
+      return new DispatchResult(CommandResult.InvalidSelectionStep);
+    }
     const sheetId = this.getters.getActiveSheetId();
     const anchor = this.anchor;
     const { col: anchorCol, row: anchorRow } = anchor.cell;
@@ -437,7 +443,7 @@ export class SelectionStreamProcessor
    */
   private getNextAvailablePosition(
     direction: SelectionDirection,
-    step: SelectionStep = "one"
+    step: SelectionStep = 1
   ): Position {
     const { col, row } = this.anchor.cell;
     const delta = this.deltaToTarget({ col, row }, direction, step);
@@ -536,20 +542,20 @@ export class SelectionStreamProcessor
   ): Delta {
     switch (direction) {
       case "up":
-        return step === "one"
-          ? [0, -1]
+        return step !== "end"
+          ? [0, -step]
           : [0, this.getEndOfCluster(position, "rows", -1) - position.row];
       case "down":
-        return step === "one"
-          ? [0, 1]
+        return step !== "end"
+          ? [0, step]
           : [0, this.getEndOfCluster(position, "rows", 1) - position.row];
       case "left":
-        return step === "one"
-          ? [-1, 0]
+        return step !== "end"
+          ? [-step, 0]
           : [this.getEndOfCluster(position, "cols", -1) - position.col, 0];
       case "right":
-        return step === "one"
-          ? [1, 0]
+        return step !== "end"
+          ? [step, 0]
           : [this.getEndOfCluster(position, "cols", 1) - position.col, 0];
     }
   }

src/types/commands.ts

@@ -1113,6 +1113,7 @@ export const enum CommandResult {
   MergeInFilter,
   NonContinuousTargets,
   DuplicatedFigureId,
+  InvalidSelectionStep,
 }
 
 export interface CommandHandler<T> {

src/types/misc.ts

@@ -232,10 +232,10 @@ export type BorderCommand =
 export type BorderDescription = { vertical?: BorderDescr; horizontal?: BorderDescr } | undefined;
 
 export const enum DIRECTION {
-  UP,
-  DOWN,
-  LEFT,
-  RIGHT,
+  UP = "up",
+  DOWN = "down",
+  LEFT = "left",
+  RIGHT = "right",
 }
 
 export type ChangeType = "REMOVE" | "RESIZE" | "MOVE" | "CHANGE" | "NONE";

src/types/selection.ts

@@ -1,3 +1,3 @@
 export type SelectionDirection = "up" | "down" | "left" | "right";
 
-export type SelectionStep = "one" | "end";
+export type SelectionStep = number | "end";

tests/plugins/navigation.test.ts

@@ -398,4 +398,25 @@ describe("Navigation starting from hidden cells", () => {
       expect(model.getters.getPosition()).toEqual(toCartesian(endPosition));
     }
   );
+
+  test("Cannot from zero or negative steps does nothing", () => {
+    const model = new Model({
+      sheets: [
+        {
+          colNumber: 5,
+          rowNumber: 2,
+        },
+      ],
+    });
+    selectCell(model, "C1");
+    hideColumns(model, ["C"]);
+    const move1 = moveAnchorCell(model, "down", 0);
+    expect(move1).toBeCancelledBecause(CommandResult.InvalidSelectionStep);
+    const move2 = moveAnchorCell(model, "up", -1);
+    expect(move2).toBeCancelledBecause(CommandResult.InvalidSelectionStep);
+    const move3 = moveAnchorCell(model, "right", -2);
+    expect(move3).toBeCancelledBecause(CommandResult.InvalidSelectionStep);
+    const move4 = moveAnchorCell(model, "left", -3);
+    expect(move4).toBeCancelledBecause(CommandResult.InvalidSelectionStep);
+  });
 });

tests/plugins/sheetview.test.ts

@@ -667,6 +667,26 @@ describe("Viewport of Simple sheet", () => {
     });
   });
 
+  describe("Cross Move Position with selection outside the viewport affects offset", () => {
+    test("Move horizontally a cell which row is outside the viewport", () => {
+      const { bottom } = model.getters.getActiveMainViewport();
+      selectCell(model, toXC(0, bottom + 3));
+      const viewport = { ...model.getters.getActiveMainViewport() };
+      model.dispatch("SET_VIEWPORT_OFFSET", { offsetX: 0, offsetY: 0 });
+      moveAnchorCell(model, "right");
+      expect(model.getters.getActiveMainViewport()).toMatchObject(viewport);
+    });
+
+    test("Move vertically a cell which col is outside the viewport", () => {
+      const { right } = model.getters.getActiveMainViewport();
+      selectCell(model, toXC(right + 3, 0));
+      const viewport = { ...model.getters.getActiveMainViewport() };
+      model.dispatch("SET_VIEWPORT_OFFSET", { offsetX: 0, offsetY: 0 });
+      moveAnchorCell(model, "down");
+      expect(model.getters.getActiveMainViewport()).toMatchObject(viewport);
+    });
+  });
+
   test("Move position on cells that are taller than the client's height", () => {
     const { height } = model.getters.getSheetViewDimensionWithHeaders();
     resizeRows(model, [0], height + 50);

tests/test_helpers/commands_helpers.ts

@@ -479,15 +479,15 @@ export function selectCell(model: Model, xc: string): DispatchResult {
 export function moveAnchorCell(
   model: Model,
   direction: SelectionDirection,
-  step: SelectionStep = "one"
+  step: SelectionStep = 1
 ): DispatchResult {
   return model.selection.moveAnchorCell(direction, step);
 }
 
 export function resizeAnchorZone(
   model: Model,
   direction: SelectionDirection,
-  step: SelectionStep = "one"
+  step: SelectionStep = 1
 ): DispatchResult {
   return model.selection.resizeAnchorZone(direction, step);
 }