Skip to content

Commit

Permalink
fix(web): Fix layout for subkey menus with unusually-sized keys
Browse files Browse the repository at this point in the history
The previous code didn't check that the keys we put in a subkey menu
actually fit within the width of the osk. Additionally the subkey menu
was limited to two rows. This change checks the total width of the keys
in a row. If it gets too wide we continue in the next row.

Fixes #10126.
  • Loading branch information
ermshiperete committed Apr 22, 2024
1 parent 180cd9a commit 9818472
Show file tree
Hide file tree
Showing 5 changed files with 217 additions and 25 deletions.
8 changes: 2 additions & 6 deletions web/src/engine/osk/src/input/gestures/browser/oskSubKey.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export default class OSKSubKey extends OSKKey {
return 'popup-'+this.layer+'-'+this.spec['id'];
}

construct(osk: VisualKeyboard, baseKey: KeyElement, topMargin: boolean): HTMLDivElement {
construct(osk: VisualKeyboard, baseKey: KeyElement, width: number, topMargin: boolean): HTMLDivElement {
let spec = this.spec;

let kDiv=document.createElement('div');
Expand All @@ -32,11 +32,7 @@ export default class OSKSubKey extends OSKKey {
ks.marginTop='5px';
}

if(typeof spec['width'] != 'undefined') {
ks.width=(spec['width']*baseKey.offsetWidth/100)+'px';
} else {
ks.width=baseKey.offsetWidth+'px';
}
ks.width=width+'px';
ks.height=baseKey.offsetHeight+'px';

let btnEle=document.createElement('div');
Expand Down
49 changes: 35 additions & 14 deletions web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@ export default class SubkeyPopup implements GestureHandler {
// position:static and display:inline-block
const elements = this.element = document.createElement('div');

var i;
elements.id='kmw-popup-keys';

// #3718: No longer prepend base key to popup array
Expand All @@ -143,34 +142,56 @@ export default class SubkeyPopup implements GestureHandler {
ss.fontSize=computedStyle.fontSize;
ss.visibility='hidden';

var nKeys=subKeySpec.length,nRows,nCols;
nRows=Math.min(Math.ceil(nKeys/9),2);
nCols=Math.ceil(nKeys/nRows);

this.menuWidth = (nCols*e.offsetWidth + nCols*SUBKEY_DEFAULT_MARGIN_LEFT);
ss.width = this.menuWidth+'px';
const nKeys = subKeySpec.length;
// If we have more than 9 keys we put them in (at least) two rows.
const nRows=Math.min(Math.ceil(nKeys/9),2);
const nCols=Math.ceil(nKeys/nRows);

// Add nested button elements for each sub-key
this.subkeys = [];
for(i=0; i<nKeys; i++) {
var needsTopMargin = false;
let nRow=Math.floor(i/nCols);
if(nRows > 1 && nRow > 0) {
needsTopMargin = true;
let thisRowWidth = 0;
let iRow = 0;
for(let i=0, iCol=0; i<nKeys; i++, iCol++) {
if (iCol >= nCols) {
iRow++;
iCol = 0;
thisRowWidth = 0;
}

let layer = e['key'].layer;
if(typeof(layer) != 'string' || layer == '') {
// Use the currently-active layer.
layer = vkbd.layerId;
}
let keyGenerator = new OSKSubKey(subKeySpec[i], layer);
let kDiv = keyGenerator.construct(vkbd, <KeyElement> e, needsTopMargin);

let subkeyWidth = (typeof subKeySpec[i]['width'] != 'undefined') ?
subKeySpec[i]['width'] * e.offsetWidth / 100 :
e.offsetWidth;

if (subkeyWidth > vkbd.width - 2 * SUBKEY_DEFAULT_MARGIN_LEFT) {
subkeyWidth = vkbd.width - 2 * SUBKEY_DEFAULT_MARGIN_LEFT;
}

if (thisRowWidth + subkeyWidth + SUBKEY_DEFAULT_MARGIN_LEFT > vkbd.width) {
// New subkey doesn't fit in the current row. Start a new row.
// TODO: currently we don't check that the rows fit vertically,
// so it's possible that the top or bottom of the subkey menu
// is not visible.
iRow++;
iCol = 0;
thisRowWidth = 0;
}
const keyGenerator = new OSKSubKey(subKeySpec[i], layer);
const kDiv = keyGenerator.construct(vkbd, <KeyElement>e, subkeyWidth, iRow > 0);
thisRowWidth += subkeyWidth + SUBKEY_DEFAULT_MARGIN_LEFT;
this.menuWidth = Math.max(this.menuWidth ?? 0, thisRowWidth);
this.subkeys.push(kDiv.firstChild as KeyElement);

elements.appendChild(kDiv);
}

ss.width = this.menuWidth + 'px';

// And add a filter to fade main keyboard
this.shim = document.createElement('div');
this.shim.id = 'kmw-popup-shim';
Expand Down
12 changes: 8 additions & 4 deletions web/src/engine/osk/src/visualKeyboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,14 @@ import { GesturePreviewHost } from './keyboard-layout/gesturePreviewHost.js';
import OSKBaseKey from './keyboard-layout/oskBaseKey.js';
import { OSKResourcePathConfiguration } from './index.js';

// #region KeyRuleEffects
interface KeyRuleEffects {
contextToken?: number,
alteredText?: boolean
};
// #endregion

// #region VisualKeyboardConfiguration
export interface VisualKeyboardConfiguration extends CommonConfiguration {
/**
* The Keyman keyboard on which to base the on-screen keyboard being represented.
Expand Down Expand Up @@ -106,6 +109,7 @@ export interface VisualKeyboardConfiguration extends CommonConfiguration {
*/
specialFont?: InternalKeyboardFont;
}
// #endregion

interface BoundingRect {
left: number,
Expand All @@ -128,6 +132,7 @@ interface EventMap {
'globekey': (keyElement: KeyElement, on: boolean) => void
}

// #region VisualKeyboard
export default class VisualKeyboard extends EventEmitter<EventMap> implements KeyboardView {
/**
* The gesture-engine used to support user interaction with this keyboard.
Expand Down Expand Up @@ -286,7 +291,7 @@ export default class VisualKeyboard extends EventEmitter<EventMap> implements Ke
return this.currentLayer?.spaceBarKey?.btn;
}

//#region OSK constructor and helpers
//#region VisualKeyboard - constructor and helpers

/**
* @param {Object} PVK Visual keyboard name
Expand Down Expand Up @@ -916,7 +921,7 @@ export default class VisualKeyboard extends EventEmitter<EventMap> implements Ke
};
//#endregion

//#region OSK touch handlers
//#region VisualKeyboard - OSK touch handlers
getTouchCoordinatesOnKeyboard(input: InputSample<KeyElement, string>) {
// `input` is already in keyboard-local coordinates. It's not scaled, though.
let offsetCoords = { x: input.targetX, y: input.targetY };
Expand Down Expand Up @@ -1122,8 +1127,6 @@ export default class VisualKeyboard extends EventEmitter<EventMap> implements Ke
this._UpdateVKShiftStyle();
}

//#endregion

/**
* Add or remove a class from a keyboard key (when touched or clicked)
* or add a key preview for phone devices
Expand Down Expand Up @@ -1708,4 +1711,5 @@ export default class VisualKeyboard extends EventEmitter<EventMap> implements Ke

return callbackData;
}
// #endregion VisualKeyboard
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
import { assert } from 'chai';
import { JSDOM } from 'jsdom';
import sinon from 'sinon';

import { GesturePath, GestureSequence, GestureSource, GestureSourceSubview } from '@keymanapp/gesture-recognizer';

import { ActiveSubKey } from '@keymanapp/keyboard-processor';
import { DeviceSpec } from '../../../../../../../../../../common/web/utils/build/obj/deviceSpec.js';
import OSKSubKey from '../../../../../../../../../build/engine/osk/obj/input/gestures/browser/oskSubKey.js';
import OSKBaseKey from '../../../../../../../../../build/engine/osk/obj/keyboard-layout/oskBaseKey.js';
import OSKRow from '../../../../../../../../../build/engine/osk/obj/keyboard-layout/oskRow.js';
import { default as SubkeyPopup } from '../../../../../../../../../build/engine/osk/obj/input/gestures/browser/subkeyPopup.js'
import { link } from '../../../../../../../../../build/engine/osk/obj/keyElement.js'
import { DEFAULT_GESTURE_PARAMS, KeyElement, VisualKeyboard } from 'keyman/engine/osk';

// Tests for #10126
describe('subkey menu width', () => {
let dom: JSDOM;
let keyCount: number;

beforeEach(() => {
dom = new JSDOM('<!DOCTYPE html><p>Hello world</p>');
global.document = dom.window.document;
global.getComputedStyle = dom.window.getComputedStyle;
keyCount = 0;
});

const DEFAULT_KEY = -1;

const mockSource = () => {
const gestureSourceSubview = new GestureSourceSubview<KeyElement>(
{ path: new GesturePath<KeyElement, any>() } as GestureSource<KeyElement>,
{} as typeof GestureSource.prototype['recognizerConfigStack'], false, null);
return {
stageReports: [{ sources: [gestureSourceSubview] }],
on: (event, callback) => { },
} as GestureSequence<KeyElement, string>;
}

const mockVisualKeyboard = (oskWidth: number) => {
const visualKeyboard = sinon.createStubInstance(VisualKeyboard);
sinon.stub(visualKeyboard, 'device').get(() => new DeviceSpec('Chrome', 'Phone', 'Android', false));
sinon.stub(visualKeyboard, 'topContainer').get(() => document.createElement('div'));
sinon.stub(visualKeyboard, 'isEmbedded').get(() => false);
sinon.stub(visualKeyboard, 'layerId').get(() => '_default');
sinon.stub(visualKeyboard, 'width').get(() => oskWidth);

// Create property. "as any" is required in TS since property is readonly
(visualKeyboard as any).kbdDiv = null;
sinon.stub(visualKeyboard, 'kbdDiv').value(document.createElement('div'));
return visualKeyboard;
}

const mockKeys = (width: number, height: number, subkeySpecs: number[]) => {
const row = sinon.createStubInstance(OSKRow);
(row as any).element = document.createElement('div');

const baseKey = sinon.createStubInstance(OSKBaseKey);
(baseKey as any).row = null;
sinon.stub(baseKey, 'row').value(row);

const subKeys = [];
for (const width of subkeySpecs) {
const subKey = sinon.createStubInstance(ActiveSubKey);
if (width >= 0) {
(subKey as any).width = null;
sinon.stub(subKey, 'width').get(() => width);
}
subKeys.push(subKey);
}

const key = link(document.createElement('div'), { key: baseKey, keyId: `${keyCount++}`, subKeys: subKeys });
sinon.stub(key, 'offsetWidth').get(() => width);
sinon.stub(key, 'offsetHeight').get(() => height);
return key;
}

it('One subkey with standard width', () => {
const sut = new SubkeyPopup(
mockSource(),
sinon.stub(),
mockVisualKeyboard(300 /*px*/),
mockKeys(25, 30, [DEFAULT_KEY]),
DEFAULT_GESTURE_PARAMS
);

assert.equal(sut.element.style.width, '30px' /* 25 + SUBKEY_DEFAULT_MARGIN_LEFT */);
assert.equal((sut.element.children[0] as HTMLDivElement).style.marginTop, '');
});

it('10 subkeys with standard width', () => {
const sut = new SubkeyPopup(
mockSource(),
sinon.stub(),
mockVisualKeyboard(300 /*px*/),
mockKeys(25, 30, [DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY]),
DEFAULT_GESTURE_PARAMS
);

// should be 2 rows with 5 keys each
assert.equal(sut.element.style.width, '150px' /* 5 * (25 + SUBKEY_DEFAULT_MARGIN_LEFT) */);
assert.equal((sut.element.children[0] as HTMLDivElement).style.marginTop, '');
assert.equal((sut.element.children[4] as HTMLDivElement).style.marginTop, '');
assert.equal((sut.element.children[5] as HTMLDivElement).style.marginTop, '5px');
});

it('4 wider subkeys requiring two rows', () => {
const sut = new SubkeyPopup(
mockSource(),
sinon.stub(),
mockVisualKeyboard(350 /*px*/),
mockKeys(100, 30, [DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY]),
DEFAULT_GESTURE_PARAMS
);

// should be 2 rows with 3 keys each
assert.equal(sut.element.style.width, '315px' /* 3 * (100 + SUBKEY_DEFAULT_MARGIN_LEFT) */);
assert.equal((sut.element.children[0] as HTMLDivElement).style.marginTop, '');
assert.equal((sut.element.children[2] as HTMLDivElement).style.marginTop, '');
assert.equal((sut.element.children[3] as HTMLDivElement).style.marginTop, '5px');
});

it('10 subkeys with different widths fitting in row', () => {
const sut = new SubkeyPopup(
mockSource(),
sinon.stub(),
mockVisualKeyboard(350 /*px*/),
mockKeys(25, 30, [DEFAULT_KEY, 400, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY]),
DEFAULT_GESTURE_PARAMS
);

// should be 2 rows with 5 keys each
// First row: 4 * (25 + SUBKEY_DEFAULT_MARGIN_LEFT) + 1 * (100 + SUBKEY_DEFAULT_MARGIN_LEFT) = 225
// Second row: 5 * (25 + SUBKEY_DEFAULT_MARGIN_LEFT) = 150
assert.equal(sut.element.style.width, '225px');
assert.equal((sut.element.children[0] as HTMLDivElement).style.marginTop, '');
assert.equal((sut.element.children[4] as HTMLDivElement).style.marginTop, '');
assert.equal((sut.element.children[5] as HTMLDivElement).style.marginTop, '5px');
});

it('9 subkeys with different widths not fitting in row', () => {
const sut = new SubkeyPopup(
mockSource(),
sinon.stub(),
mockVisualKeyboard(300 /*px*/),
mockKeys(25, 30, [DEFAULT_KEY, 400, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY, DEFAULT_KEY]),
DEFAULT_GESTURE_PARAMS
);

// should be 2 rows
// First row: 6 * (25 + SUBKEY_DEFAULT_MARGIN_LEFT) + 1 * (100 + SUBKEY_DEFAULT_MARGIN_LEFT) = 285
// Second row: 2 * (25 + SUBKEY_DEFAULT_MARGIN_LEFT) = 60
assert.equal(sut.element.style.width, '285px');
assert.equal((sut.element.children[0] as HTMLDivElement).style.marginTop, '');
assert.equal((sut.element.children[6] as HTMLDivElement).style.marginTop, '');
assert.equal((sut.element.children[7] as HTMLDivElement).style.marginTop, '5px');
});

it('One monster subkey that does not fit in a row', () => {
const sut = new SubkeyPopup(
mockSource(),
sinon.stub(),
mockVisualKeyboard(150 /*px*/),
mockKeys(200, 30, [DEFAULT_KEY]),
DEFAULT_GESTURE_PARAMS
);

assert.equal(sut.element.style.width, '145px');
assert.equal((sut.element.children[0] as HTMLDivElement).style.marginTop, '');
});

});
1 change: 0 additions & 1 deletion web/src/version.txt

This file was deleted.

0 comments on commit 9818472

Please sign in to comment.