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

refactor(web): OSK spacebar-label updates now managed by layer object 🪠 #11175

Merged
merged 2 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
50 changes: 49 additions & 1 deletion web/src/engine/osk/src/keyboard-layout/oskLayer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ import OSKRow from './oskRow.js';
import OSKBaseKey from './oskBaseKey.js';
import VisualKeyboard from '../visualKeyboard.js';

export interface LayerLayoutParams {
keyboardHeight: number;
spacebarText: string;
}

export default class OSKLayer {
public readonly element: HTMLDivElement;
public readonly rows: OSKRow[];
Expand Down Expand Up @@ -76,6 +81,21 @@ export default class OSKLayer {
this.capsKey = this.findKey('K_CAPS');
this.numKey = this.findKey('K_NUMLOCK');
this.scrollKey = this.findKey('K_SCROLL');

if(this.spaceBarKey) {
const spacebarLabel = this.spaceBarKey.label;
let tButton = this.spaceBarKey.btn;

if (typeof (tButton.className) == 'undefined' || tButton.className == '') {
tButton.className = 'kmw-spacebar';
} else if (tButton.className.indexOf('kmw-spacebar') == -1) {
tButton.className += ' kmw-spacebar';
}
Comment on lines +89 to +93
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (typeof (tButton.className) == 'undefined' || tButton.className == '') {
tButton.className = 'kmw-spacebar';
} else if (tButton.className.indexOf('kmw-spacebar') == -1) {
tButton.className += ' kmw-spacebar';
}
tButton.classList.add('kmw-spacebar');

https://developer.mozilla.org/en-US/docs/Web/API/Element/classList


if (spacebarLabel.className != 'kmw-spacebar-caption') {
spacebarLabel.className = 'kmw-spacebar-caption';
}
Comment on lines +95 to +97
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (spacebarLabel.className != 'kmw-spacebar-caption') {
spacebarLabel.className = 'kmw-spacebar-caption';
}
spacebarLabel.className = 'kmw-spacebar-caption';

No need to guard this ... I am sure it'll already be optimized.

Comment on lines +86 to +97
Copy link
Member

Choose a reason for hiding this comment

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

Or how about this ... so much shorter:

Suggested change
const spacebarLabel = this.spaceBarKey.label;
let tButton = this.spaceBarKey.btn;
if (typeof (tButton.className) == 'undefined' || tButton.className == '') {
tButton.className = 'kmw-spacebar';
} else if (tButton.className.indexOf('kmw-spacebar') == -1) {
tButton.className += ' kmw-spacebar';
}
if (spacebarLabel.className != 'kmw-spacebar-caption') {
spacebarLabel.className = 'kmw-spacebar-caption';
}
this.spaceBarKey.btn.classList.add('kmw-spacebar');
this.spaceBarKey.label.className = 'kmw-spacebar-caption';

}
}

/**
Expand All @@ -96,7 +116,35 @@ export default class OSKLayer {
return null;
}

public refreshLayout(vkbd: VisualKeyboard, layerHeight: number) {
/**
* Indicate the current language and keyboard on the space bar
**/
showLanguage(displayName: string) {
if(!this.spaceBarKey) {
return () => {};
Copy link
Member

Choose a reason for hiding this comment

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

What's this about? Nothing else returns anything?

}

try {
const spacebarLabel = this.spaceBarKey.label;

// The key can read the text from here during the display update without us
// needing to trigger a reflow by running the closure below early.
this.spaceBarKey.spec.text = displayName;

// It sounds redundant, but this dramatically cuts down on browser DOM processing;
// but sometimes innerText is reported empty when it actually isn't, so set it
// anyway in that case (Safari, iOS 14.4)
if (spacebarLabel.innerText != displayName || displayName == '') {
spacebarLabel.innerText = displayName;
}
}
catch (ex) { }
Copy link
Member

Choose a reason for hiding this comment

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

Why are we catching everything here?

}

public refreshLayout(vkbd: VisualKeyboard, layoutParams: LayerLayoutParams) {
const layerHeight = layoutParams.keyboardHeight;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this here?

this.showLanguage(layoutParams.spacebarText);

// Check the heights of each row, in case different layers have different row counts.
const nRows = this.rows.length;
const rowHeight = this._rowHeight = Math.floor(layerHeight/(nRows == 0 ? 1 : nRows));
Expand Down
11 changes: 2 additions & 9 deletions web/src/engine/osk/src/views/oskView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -658,16 +658,14 @@ export default abstract class OSKView
if(this.bannerView.height > 0) {
availableHeight -= this.bannerView.height + 5;
}
// Triggers the VisualKeyboard.refreshLayout() method, which includes a showLanguage() call.
this.vkbd.setSize(this.computedWidth, availableHeight, pending);

const bs = this._Box.style;
// OSK size settings can only be reliably applied to standard VisualKeyboard
// visualizations, not to help text or empty views.
bs.width = bs.maxWidth = this.computedWidth + 'px';
bs.height = bs.maxHeight = this.computedHeight + 'px';

// Ensure that the layer's spacebar is properly captioned.
this.vkbd.showLanguage();
} else {
const bs = this._Box.style;
bs.width = 'auto';
Expand Down Expand Up @@ -855,9 +853,8 @@ export default abstract class OSKView
// Also, only change the layer ID itself if there is an actual corresponding layer
// in the OSK.
if(this.vkbd?.layerGroup.layers[newValue] && !this.vkbd?.layerLocked) {
// triggers state-update + layer refresh automatically.
this.vkbd.layerId = newValue;
// Ensure that the layer's spacebar is properly captioned.
this.vkbd.showLanguage();
}

// Ensure the keyboard view is modeling the correct state. (Correct layer, etc.)
Expand Down Expand Up @@ -892,10 +889,6 @@ export default abstract class OSKView
// First thing after it's made visible.
this.refreshLayoutIfNeeded();

if(this.keyboardView instanceof VisualKeyboard) {
this.keyboardView.showLanguage();
}

this._Visible=true;

/* In case it's still '0' from a hide() operation.
Expand Down
38 changes: 4 additions & 34 deletions web/src/engine/osk/src/visualKeyboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1115,38 +1115,6 @@ export default class VisualKeyboard extends EventEmitter<EventMap> implements Ke

//#endregion

/**
* Indicate the current language and keyboard on the space bar
**/
showLanguage() {
let activeStub = this.layoutKeyboardProperties;
let displayName: string = activeStub?.displayName ?? '(System keyboard)';

try {
var t = <HTMLElement>this.spaceBar.key.label;
let tParent = <HTMLElement>t.parentNode;
if (typeof (tParent.className) == 'undefined' || tParent.className == '') {
tParent.className = 'kmw-spacebar';
} else if (tParent.className.indexOf('kmw-spacebar') == -1) {
tParent.className += ' kmw-spacebar';
}

if (t.className != 'kmw-spacebar-caption') {
t.className = 'kmw-spacebar-caption';
}

// It sounds redundant, but this dramatically cuts down on browser DOM processing;
// but sometimes innerText is reported empty when it actually isn't, so set it
// anyway in that case (Safari, iOS 14.4)
if (t.innerText != displayName || displayName == '') {
t.innerText = displayName;
}

this.spaceBar.key.refreshLayout(this);
}
catch (ex) { }
}

/**
* 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 @@ -1269,7 +1237,6 @@ export default class VisualKeyboard extends EventEmitter<EventMap> implements Ke
const isInDOM = computedStyle.height != '' && computedStyle.height != 'auto';

// Step 2: determine basic layout geometry, refresh things that might update.
this.showLanguage(); // In case the spacebar-text mode setting has changed.

if (fixedSize) {
this._computedWidth = this.width;
Expand Down Expand Up @@ -1307,7 +1274,10 @@ export default class VisualKeyboard extends EventEmitter<EventMap> implements Ke
// Step 4: perform layout operations.
// Needs the refreshed layout info to work correctly.
if(this.currentLayer) {
this.currentLayer.refreshLayout(this, this._computedHeight - this.getVerticalLayerGroupPadding());
this.currentLayer.refreshLayout(this, {
keyboardHeight: this._computedHeight - this.getVerticalLayerGroupPadding(),
spacebarText: this.layoutKeyboardProperties?.displayName ?? '(System keyboard)'
});
}
}

Expand Down
Loading