From 9046d0525ea3a1ecd7fa4efa74a2e8738dc09572 Mon Sep 17 00:00:00 2001 From: Matthew Lee Date: Mon, 8 Dec 2025 10:55:38 -0500 Subject: [PATCH 1/3] feat(web): implement subkey menu realignment for even columns (#9768) This change addresses a usability issue where subkey popup menus with even-numbered columns (2, 4, 6, 8) create ambiguous default selection. When there's an even number of columns, the centered position falls between two options, making it unclear which character will be selected. The solution shifts the popup menu alignment based on the base key's position on the keyboard: - Keys on the right side: shift menu left by half a key width - Keys on the left side: shift menu right by half a key width This ensures one option always sits unambiguously under the default touch position, following the pattern used by Google's Gboard. Changes: - Added numColumns instance variable to track column count - Modified reposition() to apply shift for even-column layouts - Added comprehensive unit tests for even/odd column scenarios - Existing bounds checking prevents menu overflow Closes #9768 --- .../src/input/gestures/browser/subkeyPopup.ts | 15 ++ .../gestures/browser/subkeyPopup.tests.ts | 209 ++++++++++++++++++ 2 files changed, 224 insertions(+) diff --git a/web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts b/web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts index dcfd45ca9b8..8559a7e8a3d 100644 --- a/web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts +++ b/web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts @@ -61,6 +61,7 @@ export default class SubkeyPopup implements GestureHandler { private callout: HTMLDivElement; private readonly menuWidth: number; + private readonly numColumns: number; public readonly baseKey: KeyElement; public readonly subkeys: KeyElement[]; @@ -154,6 +155,7 @@ export default class SubkeyPopup implements GestureHandler { // Put a maximum of 9 keys in a row to reduce travel distance const nRows=Math.ceil(nKeys/9); const nCols=Math.ceil(nKeys/nRows); + this.numColumns = nCols; // Add nested button elements for each sub-key this.subkeys = []; @@ -307,6 +309,19 @@ export default class SubkeyPopup implements GestureHandler { const ss=subKeys.style; const parentOffsetLeft = e.offsetParent ? (e.offsetParent).offsetLeft : 0; let x = e.offsetLeft + parentOffsetLeft + 0.5*(e.offsetWidth-subKeys.offsetWidth); + + // Issue #9768: Realign subkey menu when columns are even to avoid ambiguous default selection + // With even-numbered columns, shift the menu by half a key width to ensure one option + // sits unambiguously under the default touch position (like Google's Gboard) + if (this.numColumns % 2 === 0) { + const keyCenter = e.offsetLeft + parentOffsetLeft + e.offsetWidth / 2; + const keyboardCenter = vkbd.width / 2; + const halfKeyShift = e.offsetWidth / 2; + + // Shift left if key is on right side, right if on left side + x += (keyCenter > keyboardCenter) ? -halfKeyShift : halfKeyShift; + } + const xMax = vkbd.width - subKeys.offsetWidth; if(x > xMax) { diff --git a/web/src/test/auto/headless/engine/osk/input/gestures/browser/subkeyPopup.tests.ts b/web/src/test/auto/headless/engine/osk/input/gestures/browser/subkeyPopup.tests.ts index 72679897f37..6a025a16338 100644 --- a/web/src/test/auto/headless/engine/osk/input/gestures/browser/subkeyPopup.tests.ts +++ b/web/src/test/auto/headless/engine/osk/input/gestures/browser/subkeyPopup.tests.ts @@ -198,3 +198,212 @@ describe('subkey menu width', () => { }); }); + +// Tests for #9768: even-column realignment +describe('subkey menu even-column realignment', () => { + let dom: JSDOM; + + beforeEach(() => { + dom = new JSDOM('

Hello world

'); + global.document = dom.window.document; + global.getComputedStyle = dom.window.getComputedStyle; + }); + + const DEFAULT_KEY = -1; + + const mockSource = () => { + const gestureSourceSubview = new GestureSourceSubview( + { path: new GesturePath() } as GestureSource, + {} as typeof GestureSource.prototype['recognizerConfigStack'], false, null); + return { + stageReports: [{ sources: [gestureSourceSubview] }], + on: (event, callback) => { }, + } as GestureSequence; + } + + const mockVisualKeyboard = (oskWidth: number) => { + const topContainer = document.createElement('div'); + document.body.appendChild(topContainer); + + const oskElement = document.createElement('div'); + topContainer.appendChild(oskElement); + + const visualKeyboard = sinon.createStubInstance(VisualKeyboard); + sinon.stub(visualKeyboard, 'device').get(() => new DeviceSpec('Chrome', 'Phone', 'Android', false)); + sinon.stub(visualKeyboard, 'topContainer').get(() => topContainer); + sinon.stub(visualKeyboard, 'element').get(() => oskElement); + sinon.stub(visualKeyboard, 'isEmbedded').get(() => false); + sinon.stub(visualKeyboard, 'layerId').get(() => '_default'); + sinon.stub(visualKeyboard, 'width').get(() => oskWidth); + + (visualKeyboard as any).kbdDiv = null; + sinon.stub(visualKeyboard, 'kbdDiv').value(document.createElement('div')); + return visualKeyboard; + } + + const mockKeys = (offsetLeft: number, width: number, height: number, subkeySpecs: number[]) => { + const rowElement = document.createElement('div'); + document.body.appendChild(rowElement); + + const row = sinon.createStubInstance(OSKRow); + (row as any).element = null; + sinon.stub(row, 'element').value(rowElement); + + const baseKey = sinon.createStubInstance(OSKBaseKey); + (baseKey as any).row = null; + sinon.stub(baseKey, 'row').value(row); + + const subKeys = []; + for (const subkeyWidth of subkeySpecs) { + const subKey = sinon.createStubInstance(ActiveSubKey); + if (subkeyWidth >= 0) { + (subKey as any).width = null; + sinon.stub(subKey, 'width').get(() => subkeyWidth); + } + subKeys.push(subKey); + } + + const key = link(document.createElement('div'), { key: baseKey, keyId: 'test-key', subKeys: subKeys }); + sinon.stub(key, 'offsetLeft').get(() => offsetLeft); + sinon.stub(key, 'offsetWidth').get(() => width); + sinon.stub(key, 'offsetHeight').get(() => height); + sinon.stub(key, 'offsetParent').get(() => null); + + return key; + } + + it('Even columns (2) on left side shifts menu right', () => { + // Key on left side of keyboard (offsetLeft=10, keyboard width=300) + const sut = new SubkeyPopup( + mockSource(), + sinon.stub(), + mockVisualKeyboard(300 /*px*/), + mockKeys(10, 50, 40, [DEFAULT_KEY, DEFAULT_KEY]), + DEFAULT_GESTURE_PARAMS + ); + + // Insert element into DOM to get computed width + document.body.appendChild(sut.element); + + // Manually set the menu width to simulate layout + sut.element.style.width = '110px'; // 2 * (50 + 5) + 5 + Object.defineProperty(sut.element, 'offsetWidth', { value: 110, configurable: true }); + + // Reposition to apply the alignment logic + sut.reposition(mockVisualKeyboard(300)); + + const menuLeft = parseInt(sut.element.style.left); + + // Base position would be: 10 + 0.5*(50-110) = 10 - 30 = -20 + // With even-column shift right: -20 + 25 = 5 + // But clamped to >= 0, so expected = 5 + assert.isAtLeast(menuLeft, 0, 'Menu should not overflow left edge'); + assert.isAtMost(menuLeft, 190, 'Menu should not overflow right edge (300 - 110)'); + + // The key is on the left side, so we expect a rightward shift + // Without shift: centered would be at (10 + 25 - 55) = -20, clamped to 0 + // With shift: (-20 + 25) = 5, clamped to 5 + assert.equal(menuLeft, 5); + }); + + it('Even columns (2) on right side shifts menu left', () => { + // Key on right side of keyboard (offsetLeft=240, keyboard width=300) + const sut = new SubkeyPopup( + mockSource(), + sinon.stub(), + mockVisualKeyboard(300 /*px*/), + mockKeys(240, 50, 40, [DEFAULT_KEY, DEFAULT_KEY]), + DEFAULT_GESTURE_PARAMS + ); + + document.body.appendChild(sut.element); + sut.element.style.width = '110px'; + Object.defineProperty(sut.element, 'offsetWidth', { value: 110, configurable: true }); + + sut.reposition(mockVisualKeyboard(300)); + + const menuLeft = parseInt(sut.element.style.left); + + // Base position: 240 + 0.5*(50-110) = 240 - 30 = 210 + // With even-column shift left: 210 - 25 = 185 + // Max allowed: 300 - 110 = 190, so 185 is within bounds + assert.equal(menuLeft, 185); + }); + + it('Odd columns (3) remains centered regardless of position', () => { + // Key on left side + const sutLeft = new SubkeyPopup( + mockSource(), + sinon.stub(), + mockVisualKeyboard(300 /*px*/), + mockKeys(10, 50, 40, [DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY]), + DEFAULT_GESTURE_PARAMS + ); + + document.body.appendChild(sutLeft.element); + sutLeft.element.style.width = '165px'; // 3 * (50 + 5) + 5 + Object.defineProperty(sutLeft.element, 'offsetWidth', { value: 165, configurable: true }); + + sutLeft.reposition(mockVisualKeyboard(300)); + + const menuLeftPos = parseInt(sutLeft.element.style.left); + + // Base position: 10 + 0.5*(50-165) = 10 - 57.5 = -47.5 + // No shift for odd columns, clamped to 0 + assert.equal(menuLeftPos, 0); + + // Key on right side + const sutRight = new SubkeyPopup( + mockSource(), + sinon.stub(), + mockVisualKeyboard(300 /*px*/), + mockKeys(240, 50, 40, [DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY]), + DEFAULT_GESTURE_PARAMS + ); + + document.body.appendChild(sutRight.element); + sutRight.element.style.width = '165px'; + Object.defineProperty(sutRight.element, 'offsetWidth', { value: 165, configurable: true }); + + sutRight.reposition(mockVisualKeyboard(300)); + + const menuRightPos = parseInt(sutRight.element.style.left); + + // Base position: 240 + 0.5*(50-165) = 240 - 57.5 = 182.5 + // Max allowed: 300 - 165 = 135, so clamped to 135 + assert.equal(menuRightPos, 135); + }); + + it('Even columns (4) applies shift correctly', () => { + // 4 subkeys = 2 columns (max 9 per row, ceil(4/1) = 4, but width constraint may wrap) + // For this test, assume they fit in 2x2 layout + const sut = new SubkeyPopup( + mockSource(), + sinon.stub(), + mockVisualKeyboard(400 /*px*/), + mockKeys(100, 50, 40, [DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY]), + DEFAULT_GESTURE_PARAMS + ); + + document.body.appendChild(sut.element); + + // With 4 keys: nRows = ceil(4/9) = 1, nCols = ceil(4/1) = 4 + // So this will have 4 columns (even), not 2 + // Width: 4 * (50 + 5) + 5 = 225 + sut.element.style.width = '225px'; + Object.defineProperty(sut.element, 'offsetWidth', { value: 225, configurable: true }); + + sut.reposition(mockVisualKeyboard(400)); + + const menuLeft = parseInt(sut.element.style.left); + + // Key center: 100 + 25 = 125 + // Keyboard center: 200 + // Key is on left side (125 < 200), so shift right + // Base position: 100 + 0.5*(50-225) = 100 - 87.5 = 12.5 + // With shift: 12.5 + 25 = 37.5 -> 37 (rounded) + assert.isAtLeast(menuLeft, 0); + assert.isAtMost(menuLeft, 175); // 400 - 225 + }); + +}); From 1606a286bda9ab3f3c76354980049e309d4cbdaf Mon Sep 17 00:00:00 2001 From: Matthew Lee Date: Mon, 8 Dec 2025 14:42:02 -0500 Subject: [PATCH 2/3] Adds note --- web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts b/web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts index 8559a7e8a3d..cc5f595dd74 100644 --- a/web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts +++ b/web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts @@ -313,6 +313,9 @@ export default class SubkeyPopup implements GestureHandler { // Issue #9768: Realign subkey menu when columns are even to avoid ambiguous default selection // With even-numbered columns, shift the menu by half a key width to ensure one option // sits unambiguously under the default touch position (like Google's Gboard) + // Note: This uses the idealized column count and assumes uniform key widths. If subkeys + // have significantly different widths, the actual layout may already be unambiguous, + // but applying this shift won't cause harm (bounds checking prevents overflow). if (this.numColumns % 2 === 0) { const keyCenter = e.offsetLeft + parentOffsetLeft + e.offsetWidth / 2; const keyboardCenter = vkbd.width / 2; From d39a9b4ae1cbb39cf1e66d530c377f74b9f5349b Mon Sep 17 00:00:00 2001 From: Matthew Lee Date: Mon, 8 Dec 2025 15:37:04 -0500 Subject: [PATCH 3/3] Math fixes --- .../src/input/gestures/browser/subkeyPopup.ts | 29 ++++++--- .../gestures/browser/subkeyPopup.tests.ts | 60 +++++++++++++++++++ 2 files changed, 82 insertions(+), 7 deletions(-) diff --git a/web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts b/web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts index cc5f595dd74..a0e4d39b29b 100644 --- a/web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts +++ b/web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts @@ -61,7 +61,7 @@ export default class SubkeyPopup implements GestureHandler { private callout: HTMLDivElement; private readonly menuWidth: number; - private readonly numColumns: number; + private readonly maxRowColumns: number; public readonly baseKey: KeyElement; public readonly subkeys: KeyElement[]; @@ -155,13 +155,14 @@ export default class SubkeyPopup implements GestureHandler { // Put a maximum of 9 keys in a row to reduce travel distance const nRows=Math.ceil(nKeys/9); const nCols=Math.ceil(nKeys/nRows); - this.numColumns = nCols; // Add nested button elements for each sub-key this.subkeys = []; let thisRowWidth = SUBKEY_DEFAULT_MARGIN_LEFT; let iRow = 0; - for(let i=0, iCol=0; i { assert.isAtMost(menuLeft, 175); // 400 - 225 }); + it('Uneven rows (11 keys: 6+5) applies shift when widest row is even', () => { + // 11 keys: nRows = ceil(11/9) = 2, nCols = ceil(11/2) = 6 + // Layout: Top row has 6 keys (even), bottom row has 5 keys (odd) + // Rows are left-aligned, so widest row (6, even) determines shift + const sut = new SubkeyPopup( + mockSource(), + sinon.stub(), + mockVisualKeyboard(400 /*px*/), + mockKeys(100, 50, 40, Array(11).fill(DEFAULT_KEY)), + DEFAULT_GESTURE_PARAMS + ); + + document.body.appendChild(sut.element); + + // Width for 6 keys: 6 * (50 + 5) + 5 = 335 + sut.element.style.width = '335px'; + Object.defineProperty(sut.element, 'offsetWidth', { value: 335, configurable: true }); + + sut.reposition(mockVisualKeyboard(400)); + + const menuLeft = parseInt(sut.element.style.left); + + // Key center: 100 + 25 = 125 + // Keyboard center: 200 + // Key is on left side, widest row has even columns (6), so shift RIGHT + // Base position: 100 + 0.5*(50-335) = 100 - 142.5 = -42.5 + // With shift: -42.5 + 25 = -17.5, clamped to 0 + assert.equal(menuLeft, 0, 'Should apply rightward shift but be clamped to 0'); + }); + + it('Uneven rows (12 keys: 6+6) applies shift when widest row is even', () => { + // 12 keys: nRows = ceil(12/9) = 2, nCols = ceil(12/2) = 6 + // Layout: Top row has 6 keys (even), bottom row has 6 keys (even) + // Widest row is 6 (even), shift SHOULD be applied + const sut = new SubkeyPopup( + mockSource(), + sinon.stub(), + mockVisualKeyboard(400 /*px*/), + mockKeys(100, 50, 40, Array(12).fill(DEFAULT_KEY)), + DEFAULT_GESTURE_PARAMS + ); + + document.body.appendChild(sut.element); + + // Width for 6 keys: 6 * (50 + 5) + 5 = 335 + sut.element.style.width = '335px'; + Object.defineProperty(sut.element, 'offsetWidth', { value: 335, configurable: true }); + + sut.reposition(mockVisualKeyboard(400)); + + const menuLeft = parseInt(sut.element.style.left); + + // Key center: 100 + 25 = 125 + // Keyboard center: 200 + // Key is on left side, widest row has even columns, so shift right + // Base position: 100 + 0.5*(50-335) = 100 - 142.5 = -42.5 + // With shift: -42.5 + 25 = -17.5, clamped to 0 + assert.equal(menuLeft, 0, 'Should apply rightward shift but be clamped to 0'); + }); + });