From f90e06b6cd08f3539af3e425802ea93b1868423c Mon Sep 17 00:00:00 2001 From: icecream17 Date: Thu, 7 Mar 2024 15:02:22 +0000 Subject: [PATCH] 0.35.1 fix keyboard navigation --- CHANGELOG.md | 6 ++++++ Todo | 2 ++ package.json | 2 +- src/Api/Solver.test.tsx | 2 +- src/App.test.tsx | 2 +- src/Elems/Aside.test.tsx | 2 +- src/Elems/Aside.tsx | 2 +- src/Elems/AsideElems/StrategyItem.css | 5 ++--- src/Elems/AsideElems/StrategyList.css | 18 ++++++++++++------ src/Elems/AsideElems/Tabs.test.tsx | 6 +++--- src/Elems/MainElems/Sudoku.css | 1 + src/Elems/MainElems/Sudoku.test.tsx | 25 ++++++++++++++++++------- src/Elems/MainElems/Sudoku.tsx | 14 ++++++++++++-- src/Elems/Version.tsx | 2 +- 14 files changed, 62 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b92e2f90..fb8aa494 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ Note: Many earlier versions are not specified, that's too much work. When a `@types` dependency updates, they almost always don't affect anything. +## v0.35.1 + +- (use) Fix massive bug in cell keyboard nagivation, where pressing an arrow key would focus the next cell and select the next next cell +- (a11y) Expand abbrevation (`strats` ⇒ `strategies`) +- (css) Fix some incorrect rtl display + ## v0.35.0 - (use) Multiselectable cells! Select multiple cells at the same time with Ctrl+Click diff --git a/Todo b/Todo index f6c79b4c..b26b66dd 100644 --- a/Todo +++ b/Todo @@ -19,6 +19,8 @@ ## Code [ ] Migrate undo functionality completely to the solver +[ ] Make rtl handling consistent in the css files +[ ] rtl keyboard navigation should be inverted? ## Features diff --git a/package.json b/package.json index 625cf253..28e67fe2 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "solver", - "version": "0.35.0", + "version": "0.35.1", "private": true, "homepage": "https://icecream17.github.io/solver", "dependencies": { diff --git a/src/Api/Solver.test.tsx b/src/Api/Solver.test.tsx index be440d63..d9d084ae 100644 --- a/src/Api/Solver.test.tsx +++ b/src/Api/Solver.test.tsx @@ -10,7 +10,7 @@ jest.setTimeout(17000); beforeEach(() => { render(); - switchTab("strats"); + switchTab("strategies"); }) test("Import board", async () => { diff --git a/src/App.test.tsx b/src/App.test.tsx index f5d32a84..7505bc86 100644 --- a/src/App.test.tsx +++ b/src/App.test.tsx @@ -19,7 +19,7 @@ test('a header exists', () => { }) test("Strategy sections exist", () => { - switchTab('strats') + switchTab('strategies') expect(screen.getByRole('group', { name: 'strategies' })).toBeInTheDocument() expect(screen.getByRole('group', { name: 'controls' })).toBeInTheDocument() }) diff --git a/src/Elems/Aside.test.tsx b/src/Elems/Aside.test.tsx index 43d4cb18..e3d32b3e 100644 --- a/src/Elems/Aside.test.tsx +++ b/src/Elems/Aside.test.tsx @@ -11,7 +11,7 @@ beforeEach(() => { }) function getTogglers () { - switchTab('strats') + switchTab('strategies') try { return screen.getAllByRole('checkbox') diff --git a/src/Elems/Aside.tsx b/src/Elems/Aside.tsx index a573e317..06c3b025 100644 --- a/src/Elems/Aside.tsx +++ b/src/Elems/Aside.tsx @@ -23,7 +23,7 @@ type AsideState = Readonly<{ }> -const tabNames = ["solving tools", "strats"] +const tabNames = ["solving tools", "strategies"] /** * Currently a window of tabs diff --git a/src/Elems/AsideElems/StrategyItem.css b/src/Elems/AsideElems/StrategyItem.css index 6bb5b41b..465543c1 100644 --- a/src/Elems/AsideElems/StrategyItem.css +++ b/src/Elems/AsideElems/StrategyItem.css @@ -34,7 +34,6 @@ position: absolute; left: -1rem; margin: 0; /* Now sanitize.css works! */ - vertical-align: middle; text-align: center; line-height: inherit; @@ -55,7 +54,7 @@ .StrategyToggler:dir(rtl) { left: initial; - right: -1rem; + right: 0; } } @@ -72,7 +71,7 @@ [dir=rtl] .StrategyToggler { left: initial; - right: -1rem; + right: 0; } } diff --git a/src/Elems/AsideElems/StrategyList.css b/src/Elems/AsideElems/StrategyList.css index 54ef92e0..62faed9d 100644 --- a/src/Elems/AsideElems/StrategyList.css +++ b/src/Elems/AsideElems/StrategyList.css @@ -9,12 +9,18 @@ border-right: 5px solid rgb(183, 193, 194); } -.StrategyItem.isCurrent[dir=rtl] { - border-left: 5px solid rgb(183, 193, 194); -} - -.StrategyItem.isCurrent:dir(rtl) { - border-left: 5px solid rgb(183, 193, 194); +@supports selector(:dir(rtl)) { + .StrategyItem.isCurrent:dir(rtl) { + border-right: initial; + border-left: 5px solid rgb(183, 193, 194); + } +} + +@supports not selector(:dir(rtl)) { + .StrategyItem.isCurrent[dir=rtl] { + border-right: initial; + border-left: 5px solid rgb(183, 193, 194); + } } /* Alternating colors - note that the markers are not in color */ diff --git a/src/Elems/AsideElems/Tabs.test.tsx b/src/Elems/AsideElems/Tabs.test.tsx index 8872c341..cc5ff8be 100644 --- a/src/Elems/AsideElems/Tabs.test.tsx +++ b/src/Elems/AsideElems/Tabs.test.tsx @@ -22,7 +22,7 @@ beforeEach(() => { test('Tab: When focus moves into the tab list, focus on the active tab element', () => { const dummyTextarea = screen.getByTestId("dummy-focusable") - const stratsTab = screen.getByRole("tab", { name: "strats" }) + const stratsTab = screen.getByRole("tab", { name: "strategies" }) userEvent.click(stratsTab) dummyTextarea.focus() userEvent.tab() @@ -39,7 +39,7 @@ test.skip('Tab: When in the tab list, focus on the tabpanel unless the first mea test.skip('Left / Right arrow keys', () => { const solveToolsTab = screen.getByRole("tab", { name: "solving tools" }) - const stratsTab = screen.getByRole("tab", { name: "strats" }) + const stratsTab = screen.getByRole("tab", { name: "strategies" }) userEvent.click(stratsTab) userEvent.keyboard('{ArrowLeft}') @@ -58,7 +58,7 @@ test.skip('Left / Right arrow keys', () => { test('Home / End', () => { const solveToolsTab = screen.getByRole("tab", { name: "solving tools" }) - const stratsTab = screen.getByRole("tab", { name: "strats" }) + const stratsTab = screen.getByRole("tab", { name: "strategies" }) userEvent.click(stratsTab) userEvent.keyboard('{Home}') diff --git a/src/Elems/MainElems/Sudoku.css b/src/Elems/MainElems/Sudoku.css index b7eea653..6401b8b4 100644 --- a/src/Elems/MainElems/Sudoku.css +++ b/src/Elems/MainElems/Sudoku.css @@ -57,6 +57,7 @@ td.Cell:nth-child(3n):not(:last-child) { [dir=rtl] td.Cell:nth-child(3n):not(:last-child), td.Cell:nth-child(3n):not(:last-child):dir(rtl) { + border-right: initial; border-left: var(--large-border) solid var(--border-color); } diff --git a/src/Elems/MainElems/Sudoku.test.tsx b/src/Elems/MainElems/Sudoku.test.tsx index 10c4f6d2..0d59dd25 100644 --- a/src/Elems/MainElems/Sudoku.test.tsx +++ b/src/Elems/MainElems/Sudoku.test.tsx @@ -47,19 +47,26 @@ test("Focused buttonCells are highlighted", () => { fireEvent.blur(buttonCell) expect(buttonCell).toHaveAttribute('data-active', 'false') // selection disabled + // An actual bug - pressing right moved the focus right and selected the cell right + const nextCell = getButtonCellElement(0, 2) + userEvent.click(buttonCell) + userEvent.keyboard('{ArrowRight}') + expect(nextCell).not.toHaveAttribute('data-active') // not selected + userEvent.click(buttonCell) + // Multi-select - const nextCell = getButtonCellElement(3, 4) + const buttonCell2 = getButtonCellElement(3, 4) userEvent.keyboard('{Control>}') - userEvent.click(nextCell) + userEvent.click(buttonCell2) expect(buttonCell).toHaveAttribute('data-active', 'true') // selected - expect(nextCell).toHaveAttribute('data-active', 'true') // selected - fireEvent.blur(nextCell) + expect(buttonCell2).toHaveAttribute('data-active', 'true') // selected + fireEvent.blur(buttonCell2) expect(buttonCell).toHaveAttribute('data-active', 'false') // selection disabled - expect(nextCell).toHaveAttribute('data-active', 'false') // selection disabled - userEvent.click(nextCell) + expect(buttonCell2).toHaveAttribute('data-active', 'false') // selection disabled + userEvent.click(buttonCell2) userEvent.keyboard('{/Control}{Escape}') expect(buttonCell).not.toHaveAttribute('data-active') // not selected - expect(nextCell).not.toHaveAttribute('data-active') // not selected + expect(buttonCell2).not.toHaveAttribute('data-active') // not selected }) test("Setting a cell to a digit", () => { @@ -179,6 +186,10 @@ test("Cell keyboard navigation: Tab + Arrows", () => { tryKey('{ArrowDown}', 0, 7) tryKey('{ArrowRight}', 0, 8) tryKey('{ArrowRight}', 0, 0) + tryKey('{ArrowRight>}{ArrowDown}{/ArrowRight}', 1, 1) // Actual bug + tryKey('{ArrowRight>3/}', 1, 4) // Repeat + // tryKey('{ArrowUp>}{ArrowLeft>4/}{/ArrowUp}', 7, 1) // Diagonal repeat + // I tested this manually and it works so... idk }) test("Cell keyboard navigation: End / Home", () => { diff --git a/src/Elems/MainElems/Sudoku.tsx b/src/Elems/MainElems/Sudoku.tsx index bcf94fdf..b0cd1216 100644 --- a/src/Elems/MainElems/Sudoku.tsx +++ b/src/Elems/MainElems/Sudoku.tsx @@ -170,6 +170,7 @@ export default class Sudoku extends React.Component { // The effect of Ctrl is to add (or remove) only one cell to the selection. whenCellFocus(cell: Cell, _event: React.FocusEvent) { + // console.debug("focus", cell.id) const ctrlMultiselect = keysPressed.has('Control') && !keysPressed.has('Tab') if (!ctrlMultiselect) { this.cellsSelected.clear() @@ -181,6 +182,7 @@ export default class Sudoku extends React.Component { } whenCellBlur(cell: Cell, event: React.FocusEvent) { + // console.debug("blur", cell.id) // When Escape blurs a cell, the selection could be empty // in which case, do nothing if (this.selectionStatus === null) { @@ -208,6 +210,7 @@ export default class Sudoku extends React.Component { * Edit: I'm not sure if I should actually follow that section */ whenCellKeydown(targetCell: Cell, event: React.KeyboardEvent) { + // console.debug("keyboard", targetCell.id, event.repeat, event.key) // Use default behavior when tabbing if (event.key === 'Tab') { return @@ -219,17 +222,24 @@ export default class Sudoku extends React.Component { // This has no effect on the listeners. SAFETY keysPressed.add(event.key) + // Let's say the user pressed both Up and Right at basically the same time + // What the code sees: Up, Right, UpEnd, RightEnd + // What the code does: Up, Up+Right + // We will only consider other keys to repeat when a repeat event occurs + const keysToProcess = event.repeat ? keysPressed : [event.key] + const newSelected = new Set() const target = event.target as HTMLDivElement const shiftHeld = keysPressed.has('Shift') const ctrlHeld = keysPressed.has('Control') - for (let {row, column} of this.cellsSelected) { + // Copy/save previous cells selected, since it changes during the loop + for (let {row, column} of [...this.cellsSelected]) { let cell = this.props.sudoku.cells[row][column] const wasTarget = cell === targetCell - for (const key of keysPressed) { + for (const key of keysToProcess) { if (cell == null) { break } diff --git a/src/Elems/Version.tsx b/src/Elems/Version.tsx index facc341e..c7a31ac6 100644 --- a/src/Elems/Version.tsx +++ b/src/Elems/Version.tsx @@ -9,7 +9,7 @@ import StaticComponent from './StaticComponent'; * */ function Version () { - return v0.35.0 + return v0.35.1 } export default StaticComponent(Version)