Skip to content

Commit

Permalink
fix(web): font size was not consistently set
Browse files Browse the repository at this point in the history
Fixes #5779.
Fixes #5731 (I believe mitigation is sufficient to close this issue).

A variety of interrelated font and font size display issues resolved:
1. KVK font was not applied early enough for size calculations, which
   meant that we were calculating font scaling per key based on a
   default font when transforming from the KVK data for desktop devices
   (defaultLayout.ts)
2. Font scaling for non-default layers was calculated when elements were
   not visible and had no size information, giving incorrect values. To
   resolve this, font scaling is now calculated when a layer is made
   visible, which had performance impacts; resolved by reducing
   unnecessary `layer.refreshLayout()` calls; see performance point 1
   below (oskView.ts:layerChangeHandler())
3. `getViewportScale()` would return an incorrect scaled value when
   emulating touch devices on a desktop browser (kmwutils.ts)
4. After switching keyboards, the device-specific scaling factor was not
   maintained (oskView.ts:refreshLayout())

Related performance improvements:
1. Multiple calls to `layer.refreshLayout()` in `refreshLayout()` have
   been eliminated, and only the currently visible layer is now
   refreshed. This dramatically reduces the number of calls to
   `getIdealFontSize()` which was the primary concern of #5731.
   (visualKeyboards.ts)
2. Unnecessary use of `innerHTML` replaced with `innerText`
   (oskBaseKey.ts)

Minor Keyman Developer performance improvement:
1. The web debugger no longer recalculates the OSK twice (test.js)
  • Loading branch information
mcdurdin committed Nov 14, 2021
1 parent 8d0bcd1 commit 9da4ffa
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 51 deletions.
Expand Up @@ -140,6 +140,15 @@ namespace com.keyman.keyboards {
var i, j, k, m, row, rows: LayoutRow[], key: LayoutKey, keys: LayoutKey[];
var chiral: boolean = (kbdBitmask & Codes.modifierBitmasks.IS_CHIRAL) != 0;

if(PVK['F']) {
// The KeymanWeb compiler generates a string of the format `[italic ][bold ] 1em "<font>"`
// We will ignore the bold, italic and font size spec
let legacyFontSpec = /^(?:(?:italic|bold) )* *[0-9.eE-]+(?:[a-z]+) "(.+)"$/.exec(PVK['F']);
if(legacyFontSpec) {
layout.font = legacyFontSpec[1];
}
}

var kmw10Plus = !(typeof keyLabels == 'undefined' || !keyLabels);
if(!kmw10Plus) {
// Save the processed key label information to the keyboard's general data.
Expand Down
5 changes: 5 additions & 0 deletions web/source/kmwutils.ts
Expand Up @@ -442,6 +442,11 @@ namespace com.keyman {
// This can sometimes fail with some browsers if called before document defined,
// so catch the exception
try {
// For emulation of iOS on a desktop device, use a default value
if(this.device.formFactor == 'desktop') {
return 1;
}

// Get viewport width
var viewportWidth = document.documentElement.clientWidth;

Expand Down
4 changes: 2 additions & 2 deletions web/source/osk/oskBaseKey.ts
Expand Up @@ -54,7 +54,7 @@ namespace com.keyman.osk {
let q = document.createElement('div');
q.className='kmw-key-label';
if(x > 0) {
q.innerHTML=String.fromCharCode(x);
q.innerText=String.fromCharCode(x);
} else {
// Keyman-only virtual keys have no corresponding physical key.
// So, no text for the key-cap.
Expand Down Expand Up @@ -128,7 +128,7 @@ namespace com.keyman.osk {
this.square.style.width = vkbd.layoutWidth.scaledBy(key.proportionalWidth).styleString;
this.square.style.marginLeft = vkbd.layoutWidth.scaledBy(key.proportionalPad).styleString;
this.btn.style.width = vkbd.usesFixedWidthScaling ? this.square.style.width : '100%';

if(vkbd.usesFixedHeightScaling) {
// Matches its row's height.
this.square.style.height = vkbd.layoutHeight.scaledBy(this.row.heightFraction).styleString;
Expand Down
69 changes: 33 additions & 36 deletions web/source/osk/oskView.ts
Expand Up @@ -22,7 +22,7 @@ namespace com.keyman.osk {
manual = "manual",
automatic = "automatic"
}

export abstract class OSKView {
_Box: HTMLDivElement;

Expand All @@ -34,23 +34,23 @@ namespace com.keyman.osk {
protected device: com.keyman.utils.DeviceSpec;
protected readonly hostDevice: com.keyman.utils.DeviceSpec;

private _boxBaseMouseDown: (e: MouseEvent) => boolean;
private _boxBaseMouseDown: (e: MouseEvent) => boolean;
private _boxBaseTouchStart: (e: TouchEvent) => boolean;
private _boxBaseTouchEventCancel: (e: TouchEvent) => boolean;

private keyboard: keyboards.Keyboard;

private _target: text.OutputTarget;

/**
* The configured width for this OSKManager. May be `undefined` or `null`
* to allow automatic width scaling.
* to allow automatic width scaling.
*/
private _width: ParsedLengthStyle;

/**
* The configured height for this OSKManager. May be `undefined` or `null`
* to allow automatic height scaling.
* to allow automatic height scaling.
*/
private _height: ParsedLengthStyle;

Expand Down Expand Up @@ -111,7 +111,7 @@ namespace com.keyman.osk {
// Register a listener for model change events so that we can hot-swap the banner as needed.
// Handled here b/c banner changes may trigger a need to re-layout the OSK.
const _this = this;
keymanweb.core.languageProcessor.on('statechange',
keymanweb.core.languageProcessor.on('statechange',
function(state: text.prediction.StateChangeEnum) {
let currentType = _this.bannerView.activeType;
_this.bannerView.selectBanner(state);
Expand Down Expand Up @@ -182,7 +182,7 @@ namespace com.keyman.osk {
/**
* Gets and sets the IME-like interface (`OutputTarget`) to be affected by events from
* the OSK.
*
*
* If `activationMode` is `'conditional'`, this property's state controls the visibility
* of the OSKView.
*/
Expand Down Expand Up @@ -259,11 +259,11 @@ namespace com.keyman.osk {

/**
* A property denoting whether or not the OSK should be presented if it meets its
* activation conditions.
*
* activation conditions.
*
* When `activationMode == 'manual'`, `displayIfActive == true` is the lone
* activation condition.
*
*
* Note: cannot be set to `false` if `activationMode == 'static'`.
*/
get displayIfActive(): boolean {
Expand Down Expand Up @@ -318,15 +318,15 @@ namespace com.keyman.osk {

/**
* The configured width for this VisualKeyboard. May be `undefined` or `null`
* to allow automatic width scaling.
* to allow automatic width scaling.
*/
get width(): ParsedLengthStyle {
return this._width;
}

/**
* The configured height for this VisualKeyboard. May be `undefined` or `null`
* to allow automatic height scaling.
* to allow automatic height scaling.
*/
get height(): ParsedLengthStyle {
return this._height;
Expand Down Expand Up @@ -357,7 +357,7 @@ namespace com.keyman.osk {
}

/**
* The top-level style string for the font size used by the predictive banner
* The top-level style string for the font size used by the predictive banner
* and the primary keyboard visualization elements.
*/
get baseFontSize(): string {
Expand Down Expand Up @@ -397,7 +397,7 @@ namespace com.keyman.osk {
return ParsedLengthStyle.special(fontScale, 'em');
} else {
return this.computedHeight ? ParsedLengthStyle.inPixels(this.computedHeight / 8) : undefined;
}
}
}

public get activeKeyboard(): keyboards.Keyboard {
Expand Down Expand Up @@ -488,13 +488,11 @@ namespace com.keyman.osk {
this.needsLayout = false;

// Step 3: perform layout operations.
if(!this._baseFontSize && this.parsedBaseFontSize) {
// Make sure to initialize the default font size if it hasn't already been set!
this.banner.element.style.fontSize = this.baseFontSize;
if(this.vkbd) {
this.vkbd.fontSize = this.parsedBaseFontSize;
}
this.banner.element.style.fontSize = this.baseFontSize;
if(this.vkbd) {
this.vkbd.fontSize = this.parsedBaseFontSize;
}

if(!pending) {
this.headerView?.refreshLayout();
this.bannerView.refreshLayout();
Expand All @@ -509,9 +507,6 @@ namespace com.keyman.osk {
availableHeight -= this.bannerView.height + 5;
}
this.vkbd.setSize(this.computedWidth, availableHeight, pending);
if(!pending) {
this.vkbd.refreshLayout();
}

const bs = this._Box.style;
// OSK size settings can only be reliably applied to standard VisualKeyboard
Expand Down Expand Up @@ -614,7 +609,7 @@ namespace com.keyman.osk {
private layerChangeHandler: text.SystemStoreMutationHandler = function(this: OSKView,
source: text.MutableSystemStore,
newValue: string) {
// This handler is also triggered on state-key state changes (K_CAPS) that
// This handler is also triggered on state-key state changes (K_CAPS) that
// may not actually change the layer.
if(this.vkbd) {
this.vkbd._UpdateVKShiftStyle();
Expand All @@ -631,7 +626,9 @@ namespace com.keyman.osk {

// Ensure the keyboard view is modeling the correct state. (Correct layer, etc.)
this.keyboardView.updateState();
this.refreshLayoutIfNeeded();
// We need to recalc the font size here because the layer did not have
// calculated dimensions available before it was visible
this.refreshLayout();
}
}.bind(this);

Expand Down Expand Up @@ -691,7 +688,7 @@ namespace com.keyman.osk {

/**
* The main function for presenting the OSKView.
*
*
* This includes:
* - refreshing its layout
* - displaying it
Expand Down Expand Up @@ -719,7 +716,7 @@ namespace com.keyman.osk {

/* In case it's still '0' from a hide() operation.
*
* (Opacity is only modified when device.touchable = true,
* (Opacity is only modified when device.touchable = true,
* though a couple of extra conditions may apply.)
*/
this._Box.style.opacity = '1';
Expand Down Expand Up @@ -808,7 +805,7 @@ namespace com.keyman.osk {
}

/**
*
*
* @returns `false` if the OSK is in an invalid state for being presented to the user.
*/
protected mayShow(): boolean {
Expand All @@ -829,8 +826,8 @@ namespace com.keyman.osk {
}

/**
*
* @param hiddenByUser
*
* @param hiddenByUser
* @returns `false` if the OSK is in an invalid state for being hidden from the user.
*/
protected mayHide(hiddenByUser: boolean): boolean {
Expand All @@ -851,12 +848,12 @@ namespace com.keyman.osk {
/**
* Applies CSS styling and handling needed to perform a fade animation when
* hiding the OSK.
*
*
* Note: currently reflects an effectively-dead code path, though this is
* likely not intentional. Other parts of the KMW engine seem to call hideNow()
* synchronously after each and every part of the engine that calls this function,
* cancelling the Promise.
*
*
* @returns A Promise denoting either cancellation of the hide (`false`) or
* completion of the hide & its animation (`true`)
*/
Expand Down Expand Up @@ -993,10 +990,10 @@ namespace com.keyman.osk {

/**
* Display build number
*
*
* In the future, this should raise an event that the consuming KeymanWeb
* engine may listen for & respond to, rather than having it integrated
* as part of the OSK itself.
* as part of the OSK itself.
*/
showBuild() {
let keymanweb = com.keyman.singleton;
Expand All @@ -1006,11 +1003,11 @@ namespace com.keyman.osk {

/**
* Display list of installed keyboards in pop-up menu
*
*
* In the future, this language menu should be defined as a UI module like the standard
* desktop UI modules. The globe key should then trigger an event to _request_ that the
* consuming engine display the active UI module's menu.
*
*
**/
showLanguageMenu() {
if(this.hostDevice.touchable) {
Expand Down
11 changes: 2 additions & 9 deletions web/source/osk/visualKeyboard.ts
Expand Up @@ -1232,12 +1232,6 @@ namespace com.keyman.osk {
gs.fontSize = this.fontSize.styleString;
bs.fontSize = ParsedLengthStyle.forScalar(fs).styleString;

// Needs the refreshed layout info to work correctly.
for (const layerId in this.layerGroup.layers) {
const layer = this.layerGroup.layers[layerId];
layer.refreshLayout(this, paddedHeight, this.height);
}

// NEW CODE ------

// Step 1: have the necessary conditions been met?
Expand Down Expand Up @@ -1268,9 +1262,8 @@ namespace com.keyman.osk {
// END NEW CODE -----------

// Needs the refreshed layout info to work correctly.
for (const layerId in this.layerGroup.layers) {
const layer = this.layerGroup.layers[layerId];
layer.refreshLayout(this, paddedHeight, this._computedHeight);
if(this.currentLayer) {
this.currentLayer.refreshLayout(this, paddedHeight, this._computedHeight);
}
}

Expand Down
4 changes: 0 additions & 4 deletions windows/src/developer/TIKE/xml/kmw/test.js
Expand Up @@ -219,10 +219,6 @@ window.onload = function() {
newOSK.setSize(targetDevice.dimensions[0]+'px', targetDevice.dimensions[1]+'px');
}
document.getElementById('osk-host').appendChild(newOSK.element);

keyman.osk = newOSK;
newOSK.activeKeyboard = keyman.core.activeKeyboard;
keyman.alignInputs();
}

setOSK();
Expand Down

0 comments on commit 9da4ffa

Please sign in to comment.