diff --git a/src/components/composer/composer/composer.ts b/src/components/composer/composer/composer.ts index a2ecb0f55..89f44268f 100644 --- a/src/components/composer/composer/composer.ts +++ b/src/components/composer/composer/composer.ts @@ -267,7 +267,7 @@ export class Composer extends Component { 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 { } 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() { diff --git a/src/components/grid/grid.ts b/src/components/grid/grid.ts index 031aeaa62..dc8ea0bfe 100644 --- a/src/components/grid/grid.ts +++ b/src/components/grid/grid.ts @@ -156,8 +156,8 @@ export class Grid extends Component { ? 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 diff --git a/src/components/helpers/selection_helpers.ts b/src/components/helpers/selection_helpers.ts index 8c7fd09da..04e2dc2d5 100644 --- a/src/components/helpers/selection_helpers.ts +++ b/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); } } diff --git a/src/helpers/zones.ts b/src/helpers/zones.ts index 5af142500..6f7f5e6cb 100644 --- a/src/helpers/zones.ts +++ b/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,7 +609,7 @@ 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; @@ -617,7 +617,7 @@ export function findCellInNewZone(oldZone: Zone, currentZone: Zone, viewport: Vi row = bottom; } else { // top and bottom don't change - row = viewport.top > top || top > viewport.bottom ? viewport.top : top; + row = top; } return { col, row }; } diff --git a/src/plugins/ui_core_views/sheetview.ts b/src/plugins/ui_core_views/sheetview.ts index 1762cc59b..2c0ef1a5e 100644 --- a/src/plugins/ui_core_views/sheetview.ts +++ b/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 }); diff --git a/src/plugins/ui_feature/autofill.ts b/src/plugins/ui_feature/autofill.ts index 9b880ddea..83e750f28 100644 --- a/src/plugins/ui_feature/autofill.ts +++ b/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); diff --git a/src/selection_stream/selection_stream_processor.ts b/src/selection_stream/selection_stream_processor.ts index aba5e925f..cbe14b63c 100644 --- a/src/selection_stream/selection_stream_processor.ts +++ b/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]; } } diff --git a/src/types/commands.ts b/src/types/commands.ts index cb0bbd5b9..9fac25e33 100644 --- a/src/types/commands.ts +++ b/src/types/commands.ts @@ -1113,6 +1113,7 @@ export const enum CommandResult { MergeInFilter, NonContinuousTargets, DuplicatedFigureId, + InvalidSelectionStep, } export interface CommandHandler { diff --git a/src/types/misc.ts b/src/types/misc.ts index 36ffdfa22..97af2e746 100644 --- a/src/types/misc.ts +++ b/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"; diff --git a/src/types/selection.ts b/src/types/selection.ts index 9ba67c324..66bef5fde 100644 --- a/src/types/selection.ts +++ b/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"; diff --git a/tests/plugins/navigation.test.ts b/tests/plugins/navigation.test.ts index 8bd3da2c7..5d3737453 100644 --- a/tests/plugins/navigation.test.ts +++ b/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); + }); }); diff --git a/tests/plugins/sheetview.test.ts b/tests/plugins/sheetview.test.ts index 2bc8b18fb..fa757c437 100644 --- a/tests/plugins/sheetview.test.ts +++ b/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); diff --git a/tests/test_helpers/commands_helpers.ts b/tests/test_helpers/commands_helpers.ts index ac58f74fb..114d3ca1b 100644 --- a/tests/test_helpers/commands_helpers.ts +++ b/tests/test_helpers/commands_helpers.ts @@ -479,7 +479,7 @@ 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); } @@ -487,7 +487,7 @@ export function moveAnchorCell( export function resizeAnchorZone( model: Model, direction: SelectionDirection, - step: SelectionStep = "one" + step: SelectionStep = 1 ): DispatchResult { return model.selection.resizeAnchorZone(direction, step); }