Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(web): subkey-menu fat-finger correction 🐵 #9779

Merged
merged 7 commits into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions common/web/input-processor/src/corrections.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { ActiveKeyBase, KeyDistribution } from "@keymanapp/keyboard-processor";
import { CorrectionLayout } from "./correctionLayout.js";

/**
Expand All @@ -9,8 +10,8 @@ import { CorrectionLayout } from "./correctionLayout.js";
* by a correction algorithm, also within <0, 0> to <1, 1>.
* @returns A mapping of key IDs to the 'squared pseudo-distance' of the touchpoint to each key.
*/
export function keyTouchDistances(touchCoords: {x: number, y: number}, correctiveLayout: CorrectionLayout): Map<string, number> {
let keyDists: Map<string, number> = new Map<string, number>();
export function keyTouchDistances(touchCoords: {x: number, y: number}, correctiveLayout: CorrectionLayout): Map<ActiveKeyBase, number> {
let keyDists: Map<ActiveKeyBase, number> = new Map<ActiveKeyBase, number>();

// This double-nested loop computes a pseudo-distance for the touch from each key. Quite useful for
// generating a probability distribution.
Expand Down Expand Up @@ -54,7 +55,7 @@ export function keyTouchDistances(touchCoords: {x: number, y: number}, correctiv
distY += dy * entry.height;

const distance = distX * distX + distY * distY;
keyDists.set(entry.keySpec.coreID, distance);
keyDists.set(entry.keySpec, distance);
});

return keyDists;
Expand All @@ -65,8 +66,8 @@ export function keyTouchDistances(touchCoords: {x: number, y: number}, correctiv
* consideration.
* @returns
*/
export function distributionFromDistanceMap(squaredDistMap: Map<string, number>): {keyId: string, p: number}[] {
const keyProbs = new Map<string, number>();
export function distributionFromDistanceMap(squaredDistMap: Map<ActiveKeyBase, number>): KeyDistribution {
const keyProbs = new Map<ActiveKeyBase, number>();
let totalMass = 0;

// Should we wish to allow multiple different transforms for distance -> probability, use a function parameter in place
Expand All @@ -81,10 +82,10 @@ export function distributionFromDistanceMap(squaredDistMap: Map<string, number>)
keyProbs.set(key, keyProbs.get(key) ?? 0 + entry);
}

const list: {keyId: string, p: number}[] = [];
const list: {keySpec: ActiveKeyBase, p: number}[] = [];

for(let key of keyProbs.keys()) {
list.push({keyId: key, p: keyProbs.get(key) / totalMass});
list.push({keySpec: key, p: keyProbs.get(key) / totalMass});
}

return list.sort(function(a, b) {
Expand Down
5 changes: 2 additions & 3 deletions common/web/input-processor/src/text/inputProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,6 @@ export default class InputProcessor {
// Sort the distribution into probability-descending order.
keyDistribution.sort((a, b) => b.p - a.p);

let activeLayout = this.activeKeyboard.layout(keyEvent.device.formFactor);
alternates = [];

let totalMass = 0; // Tracks sum of non-error probabilities.
Expand All @@ -314,9 +313,9 @@ export default class InputProcessor {

let mock = Mock.from(windowedMock, false);

let altKey = activeLayout.getLayer(keyEvent.kbdLayer).getKey(pair.keyId);
const altKey = pair.keySpec;
if(!altKey) {
console.warn("Potential fat-finger key could not be found in layer!");
console.warn("Internal error: failed to properly filter set of keys for corrections");
Comment on lines +316 to +318
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This obviously is connected to this ongoing Sentry error: https://keyman.sentry.io/share/issue/977c8e43dc1b40ddb7f6948252a6118b/

Note that the new pattern directly uses the object that was previously retrieved by lookup. There will no longer be an inability to find the object.

Of course, that doesn't solve the whole problem - whatever had happened to the layer, and how whatever that is should be handled in relation to fat-fingering - but this should help us get closer to finding the root of the issue, even then.

continue;
}

Expand Down
4 changes: 2 additions & 2 deletions common/web/keyboard-processor/src/keyboards/keyboard.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import Codes from "../text/codes.js";
import { Layouts, type LayoutFormFactor } from "./defaultLayouts.js";
import { ActiveKey, ActiveLayout } from "./activeLayout.js";
import { ActiveKey, ActiveKeyBase, ActiveLayout } from "./activeLayout.js";
import KeyEvent from "../text/keyEvent.js";
import type OutputTarget from "../text/outputTarget.js";

Expand Down Expand Up @@ -468,7 +468,7 @@ export default class Keyboard {
return keyEvent;
}

constructKeyEvent(key: ActiveKey, device: DeviceSpec, stateKeys: StateKeyMap): KeyEvent {
constructKeyEvent(key: ActiveKeyBase, device: DeviceSpec, stateKeys: StateKeyMap): KeyEvent {
// Make a deep copy of our preconstructed key event, filling it out from there.
const Lkc = key.baseKeyEvent;
Lkc.device = device;
Expand Down
7 changes: 4 additions & 3 deletions common/web/keyboard-processor/src/text/keyEvent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,11 @@ import { type DeviceSpec } from "@keymanapp/web-utils";

import Codes from './codes.js';
import DefaultRules from './defaultRules.js';
import { ActiveKeyBase } from "../index.js";

// Represents a probability distribution over a keyboard's keys.
// Defined here to avoid compilation issues.
export type KeyDistribution = {keyId: string, p: number}[];
export type KeyDistribution = {keySpec: ActiveKeyBase, p: number}[];

/**
* A simple instance of the standard 'default rules' for keystroke processing from the
Expand Down Expand Up @@ -45,8 +46,8 @@ export interface KeyEventSpec {
*/
srcKeyboard?: Keyboard;

// Holds relevant event properties leading to construction of this KeyEvent.
source?: any; // Technically, KeyEvent|MouseEvent|Touch - but those are DOM types that must be kept out of headless mode.
// // Holds relevant event properties leading to construction of this KeyEvent.
// source?: any; // Technically, KeyEvent|MouseEvent|Touch - but those are DOM types that must be kept out of headless mode.
jahorton marked this conversation as resolved.
Show resolved Hide resolved
// Holds a generated fat-finger distribution (when appropriate)
keyDistribution?: KeyDistribution;

Expand Down
98 changes: 96 additions & 2 deletions web/src/engine/osk/src/input/gestures/browser/subkeyPopup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ import { type KeyElement } from '../../../keyElement.js';
import OSKBaseKey from '../../../keyboard-layout/oskBaseKey.js';
import VisualKeyboard from '../../../visualKeyboard.js';

import { DeviceSpec, KeyEvent, ActiveSubKey } from '@keymanapp/keyboard-processor';
import { DeviceSpec, KeyEvent, ActiveSubKey, KeyDistribution } from '@keymanapp/keyboard-processor';
import { ConfigChangeClosure, GestureRecognizerConfiguration, GestureSequence, PaddedZoneSource } from '@keymanapp/gesture-recognizer';
import { GestureHandler } from '../gestureHandler.js';
import { CorrectionLayout, CorrectionLayoutEntry, distributionFromDistanceMap, keyTouchDistances } from '@keymanapp/input-processor';
import { GestureParams } from '../specsForLayout.js';

/**
* Represents a 'realized' longpress gesture's default implementation
Expand All @@ -31,15 +33,18 @@ export default class SubkeyPopup implements GestureHandler {
public readonly subkeys: KeyElement[];

private source: GestureSequence<KeyElement>;
private readonly gestureParams: GestureParams;

constructor(
source: GestureSequence<KeyElement>,
configChanger: ConfigChangeClosure<KeyElement>,
vkbd: VisualKeyboard,
e: KeyElement
e: KeyElement,
gestureParams: GestureParams
) {
this.baseKey = e;
this.source = source;
this.gestureParams = gestureParams;

source.on('complete', () => {
this.currentSelection?.key.highlight(false);
Expand Down Expand Up @@ -337,6 +342,95 @@ export default class SubkeyPopup implements GestureHandler {
return this.element.style.visibility == 'visible';
}

buildCorrectiveLayout(): CorrectionLayout {
const baseBounding = this.element.getBoundingClientRect();
const aspectRatio = baseBounding.width / baseBounding.height;

const keys = this.subkeys.map((keyElement) => {
const subkeyBounds = keyElement.getBoundingClientRect();

// Ensures we have the right typing.
const correctiveData: CorrectionLayoutEntry = {
keySpec: keyElement.key.spec,
centerX: ((subkeyBounds.right - subkeyBounds.width / 2) - baseBounding.left) / baseBounding.width,
centerY: ((subkeyBounds.bottom - subkeyBounds.height / 2) - baseBounding.top) / baseBounding.height,
width: subkeyBounds.width / baseBounding.width,
height: subkeyBounds.height / baseBounding.height
}

return correctiveData;
});

return {
keys: keys,
kbdScaleRatio: aspectRatio
}
}

currentStageKeyDistances(): KeyDistribution {
const latestStage = this.source.stageReports[this.source.stageReports.length-1];
const baseStage = this.source.stageReports[0];
const gestureSource = latestStage.sources[0];
const lastCoord = gestureSource.currentSample;

const baseBounding = this.element.getBoundingClientRect();
const mappedCoord = {
x: lastCoord.targetX / baseBounding.width,
y: lastCoord.targetY / baseBounding.height
}

// Lock the coordinate within base-element bounds; corrects for the allowed 'popup roaming' zone.
//
// To consider: add a 'clipping' feature to `keyTouchDistances`? It'd make sense for base keys,
// too.
mappedCoord.x = mappedCoord.x < 0 ? 0 : (mappedCoord.x > 1 ? 1: mappedCoord.x);
mappedCoord.y = mappedCoord.y < 0 ? 0 : (mappedCoord.y > 1 ? 1: mappedCoord.y);

const rawSqDistances = keyTouchDistances(mappedCoord, this.buildCorrectiveLayout());
const currentKeyDist = rawSqDistances.get(lastCoord.item.key.spec);

/*
* - how long has the subkey menu been visible?
* - Base key should be less likely if it's been visible a while,
* but reasonably likely if it only just appeared.
* - Especially if up-flicks are allowed. Though, in that case, consider
* base-layer neighbors, and particularly the one directly under the touchpoint?
* - raw distance traveled (since the menu appeared)
* - similarly, short distance = a more likely base key?
*/

// The concept: how likely is it that the user MEANT to output a subkey?
let timeDistance = Math.min(
// The full path is included by the model - meaning the base wait is included here in
// in the stats; we subtract it to get just the duration of the subkey menu.
gestureSource.path.stats.duration - baseStage.sources[0].path.stats.duration,
this.gestureParams.longpress.waitLength
) / (2 * this.gestureParams.longpress.waitLength); // normalize: max time distance of 0.5

let pathDistance = Math.min(
gestureSource.path.stats.rawDistance,
this.gestureParams.longpress.noiseTolerance*4
) / (this.gestureParams.longpress.noiseTolerance * 8); // normalize similarly.

// We only want to add a single distance 'dimension' - we'll choose the one that affects
// the interpreted distance the least. (This matters for upflick-shortcutting in particular)
const layerDistance = Math.min(timeDistance * timeDistance, pathDistance * pathDistance);
const baseKeyDistance = currentKeyDist + layerDistance;

// Include the base key as a corrective option.
const subkeyMatch = this.subkeys.find((entry) => entry.keyId == this.baseKey.keyId);
if(subkeyMatch) {
const distAsSubkey = rawSqDistances.get(subkeyMatch.key.spec);
if(distAsSubkey > baseKeyDistance) {
rawSqDistances.set(subkeyMatch.key.spec, distAsSubkey);
} // else make no changes
} else {
rawSqDistances.set(this.baseKey.key.spec, currentKeyDist + layerDistance);
}
// TODO: allow use of multiple maps for distance combining instead!
jahorton marked this conversation as resolved.
Show resolved Hide resolved
return distributionFromDistanceMap(rawSqDistances);
}

cancel() {
this.clear();
this.source.cancel();
Expand Down
4 changes: 4 additions & 0 deletions web/src/engine/osk/src/input/gestures/gestureHandler.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { KeyDistribution } from "@keymanapp/keyboard-processor";

export interface GestureHandler {
/**
* Triggers cancellation of any further processing for the gesture being handled.
Expand All @@ -9,4 +11,6 @@ export interface GestureHandler {
* scenario in which key previews (on phones) should be disabled.
*/
readonly hasModalVisualization: boolean;

currentStageKeyDistances(): KeyDistribution;
}
5 changes: 5 additions & 0 deletions web/src/engine/osk/src/input/gestures/heldRepeater.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { GestureSequence } from "@keymanapp/gesture-recognizer";
import { KeyDistribution } from "@keymanapp/keyboard-processor";

import { KeyElement } from "../../keyElement.js";
import { GestureHandler } from './gestureHandler.js';
Expand Down Expand Up @@ -35,4 +36,8 @@ export class HeldRepeater implements GestureHandler {

this.timerHandle = window.setTimeout(this.deleteRepeater, HeldRepeater.REPEAT_DELAY);
}

currentStageKeyDistances(): KeyDistribution {
return null;
}
}
Loading