From 91cd73602a5102c8ab04c4042f6d8aa983ffd47a Mon Sep 17 00:00:00 2001
From: Marc Durdin <marc@durdin.net>
Date: Thu, 12 Sep 2024 10:02:18 +0700
Subject: [PATCH 1/2] fix(developer): rwrite ldml visual keyboard compiler

The visual keyboard compiler was never finished in 17.0. This rewrites
it to:

1. Use the kmxplus data rather than reading from xml directly
2. Fill in `visualkeyboard.header.kbdname`
3. Support modifiers
4. Handle encoded characters like `\u{1234}`
5. Handle string variables like `${one}`*

Additional unit tests have been added to verify the behavior of the
visual keyboard compiler in more detail.

* String variable tests will be enabled in next commit (which is a
cherry-pick of #12404).

Other fixes:

1. The LDML XML reader was relying on its input being a Node.js `Buffer`
   even though it was declared `Uint8Array`, as it implicitly used
   `Buffer.toString()` to do text conversion. (`Buffer` subclasses from
   `Uint8Array`). This breaks when using `Uint8Array` directly and means
   we had an implicit dependency on Node.js. See also #12331.
2. XML errors were not captured in the LDML XML reader. See also #12331.
3. The unused and unfinished touch-layout-compiler.ts and
   keymanweb-compiler.ts have been removed along with corresponding unit
   tests and fixtures. These will be replaced by Core implementations;
   see #12291.

Fixes: #12395
Cherry-pick-of: #12402
---
 .../src/consts/modifier-key-constants.ts      |  33 +++
 .../ldml-keyboard/ldml-keyboard-xml-reader.ts |  26 +-
 common/web/types/src/main.ts                  |   1 +
 common/web/types/src/util/common-events.ts    |   7 +
 developer/src/kmc-ldml/package.json           |   2 +
 .../src/kmc-ldml/src/compiler/compiler.ts     |   5 +-
 .../src/compiler/keymanweb-compiler.ts        | 113 ---------
 .../src/compiler/touch-layout-compiler.ts     | 112 ---------
 .../src/compiler/visual-keyboard-compiler.ts  | 111 ++++++---
 .../src/kmc-ldml/test/fixtures/basic-kvk.txt  |   6 +-
 .../kmc-ldml/test/fixtures/basic-no-debug.js  |   1 -
 developer/src/kmc-ldml/test/fixtures/basic.js |  48 ----
 .../src/kmc-ldml/test/fixtures/basic.txt      |   6 +-
 .../src/kmc-ldml/test/fixtures/basic.xml      |   4 +-
 developer/src/kmc-ldml/test/helpers/index.ts  |  31 +--
 .../kmc-ldml/test/test-keymanweb-compiler.ts  |  50 ----
 .../test/test-visual-keyboard-compiler-e2e.ts |  31 ---
 .../test/test-visual-keyboard-compiler.ts     | 227 ++++++++++++++++++
 package-lock.json                             |  19 ++
 19 files changed, 401 insertions(+), 432 deletions(-)
 create mode 100644 common/web/types/src/consts/modifier-key-constants.ts
 delete mode 100644 developer/src/kmc-ldml/src/compiler/keymanweb-compiler.ts
 delete mode 100644 developer/src/kmc-ldml/src/compiler/touch-layout-compiler.ts
 delete mode 100644 developer/src/kmc-ldml/test/fixtures/basic-no-debug.js
 delete mode 100644 developer/src/kmc-ldml/test/fixtures/basic.js
 delete mode 100644 developer/src/kmc-ldml/test/test-keymanweb-compiler.ts
 delete mode 100644 developer/src/kmc-ldml/test/test-visual-keyboard-compiler-e2e.ts
 create mode 100644 developer/src/kmc-ldml/test/test-visual-keyboard-compiler.ts

diff --git a/common/web/types/src/consts/modifier-key-constants.ts b/common/web/types/src/consts/modifier-key-constants.ts
new file mode 100644
index 00000000000..245cbf70f37
--- /dev/null
+++ b/common/web/types/src/consts/modifier-key-constants.ts
@@ -0,0 +1,33 @@
+/*
+ * Keyman is copyright (C) SIL International. MIT License.
+ *
+ * Modifier key bit-flags
+ */
+
+export const ModifierKeyConstants = {
+  // Define Keyman Developer modifier bit-flags (exposed for use by other modules)
+  // Compare against /common/include/kmx_file.h.  CTRL+F "#define LCTRLFLAG" to find the secton.
+  LCTRLFLAG:      0x0001, // Left Control flag
+  RCTRLFLAG:      0x0002, // Right Control flag
+  LALTFLAG:       0x0004, // Left Alt flag
+  RALTFLAG:       0x0008, // Right Alt flag
+  K_SHIFTFLAG:    0x0010, // Either shift flag
+  K_CTRLFLAG:     0x0020, // Either ctrl flag
+  K_ALTFLAG:      0x0040, // Either alt flag
+  K_METAFLAG:     0x0080, // Either Meta-key flag (tentative).  Not usable in keyboard rules;
+                          // Used internally (currently, only by KMW) to ensure Meta-key
+                          // shortcuts safely bypass rules
+                          // Meta key = Command key on macOS, Windows key on Windows/Linux
+  CAPITALFLAG:    0x0100, // Caps lock on
+  NOTCAPITALFLAG: 0x0200, // Caps lock NOT on
+  NUMLOCKFLAG:    0x0400, // Num lock on
+  NOTNUMLOCKFLAG: 0x0800, // Num lock NOT on
+  SCROLLFLAG:     0x1000, // Scroll lock on
+  NOTSCROLLFLAG:  0x2000, // Scroll lock NOT on
+  ISVIRTUALKEY:   0x4000, // It is a Virtual Key Sequence
+  VIRTUALCHARKEY: 0x8000, // Keyman 6.0: Virtual Key Cap Sequence NOT YET
+
+  // Note: OTHER_MODIFIER = 0x10000, used by KMX+ for the
+  // other modifier flag in layers, > 16 bit so not available here.
+  // See keys_mod_other in keyman_core_ldml.ts
+}
diff --git a/common/web/types/src/ldml-keyboard/ldml-keyboard-xml-reader.ts b/common/web/types/src/ldml-keyboard/ldml-keyboard-xml-reader.ts
index 4f5c30bb1ac..efe3984ff0d 100644
--- a/common/web/types/src/ldml-keyboard/ldml-keyboard-xml-reader.ts
+++ b/common/web/types/src/ldml-keyboard/ldml-keyboard-xml-reader.ts
@@ -113,6 +113,7 @@ export class LDMLKeyboardXMLSourceFileReader {
       for (const sub of obj) {
         // retain the same subtag
         if (!this.boxImportsAndSpecials(sub, subtag)) {
+          // resolveImports has already logged a message
           return false;
         }
       }
@@ -125,6 +126,7 @@ export class LDMLKeyboardXMLSourceFileReader {
           boxXmlArray(obj, key);
           // Now, resolve the import
           if (!this.resolveImports(obj, subtag)) {
+            // resolveImports has already logged a message
             return false;
           }
           // now delete the import array we so carefully constructed, the caller does not
@@ -132,6 +134,7 @@ export class LDMLKeyboardXMLSourceFileReader {
           delete obj['import'];
         } else {
           if (!this.boxImportsAndSpecials(obj[key], key)) {
+            // resolveImports has already logged a message
             return false;
           }
         }
@@ -151,6 +154,7 @@ export class LDMLKeyboardXMLSourceFileReader {
     // first, the explicit imports
     for (const asImport of ([...obj['import'] as LKImport[]].reverse())) {
       if (!this.resolveOneImport(obj, subtag, asImport)) {
+        // resolveOneImport has already logged a message
         return false;
       }
     }
@@ -161,6 +165,7 @@ export class LDMLKeyboardXMLSourceFileReader {
         base: constants.cldr_import_base,
         path: constants.cldr_implied_keys_import
       }, true)) {
+        // resolveOneImport has already logged a message
         return false;
       }
     } else if (subtag === 'forms') {
@@ -169,6 +174,7 @@ export class LDMLKeyboardXMLSourceFileReader {
         base: constants.cldr_import_base,
         path: constants.cldr_implied_forms_import
       }, true)) {
+        // resolveOneImport has already logged a message
         return false;
       }
     }
@@ -266,7 +272,8 @@ export class LDMLKeyboardXMLSourceFileReader {
         // An alternative fix would be to pull xml2js directly from github
         // rather than using the version tagged on npmjs.com.
       });
-      parser.parseString(file, (e: unknown, r: unknown) => { a = r as LDMLKeyboardXMLSourceFile }); // TODO-LDML: isn't 'e' the error?
+      const data = new TextDecoder().decode(file);
+      parser.parseString(data, (e: unknown, r: unknown) => { if(e) throw e; a = r as LDMLKeyboardXMLSourceFile }); // TODO-LDML: isn't 'e' the error?
       return a;
     })();
     return source;
@@ -278,14 +285,23 @@ export class LDMLKeyboardXMLSourceFileReader {
    */
   public load(file: Uint8Array): LDMLKeyboardXMLSourceFile | null {
     if (!file) {
+      throw new Error(`file parameter must not be null`);
+    }
+
+    let source: LDMLKeyboardXMLSourceFile = null;
+    try {
+      source = this.loadUnboxed(file);
+    } catch(e) {
+      this.callbacks.reportMessage(CommonTypesMessages.Error_InvalidXml({e}));
       return null;
     }
-    const source = this.loadUnboxed(file);
-    if(this.boxArrays(source)) {
+
+    if (this.boxArrays(source)) {
       return source;
-    } else {
-      return null;
     }
+
+    // boxArrays ... resolveImports has already logged a message
+    return null;
   }
 
   loadTestDataUnboxed(file: Uint8Array): any {
diff --git a/common/web/types/src/main.ts b/common/web/types/src/main.ts
index 5c79ed67614..53200dec807 100644
--- a/common/web/types/src/main.ts
+++ b/common/web/types/src/main.ts
@@ -20,6 +20,7 @@ export { VariableParser, MarkerParser } from './ldml-keyboard/pattern-parser.js'
 export { LDMLKeyboardXMLSourceFileReader, LDMLKeyboardXMLSourceFileReaderOptions } from './ldml-keyboard/ldml-keyboard-xml-reader.js';
 
 export * as Constants from './consts/virtual-key-constants.js';
+export { ModifierKeyConstants } from './consts/modifier-key-constants.js';
 
 export { defaultCompilerOptions, CompilerBaseOptions, CompilerCallbacks, CompilerOptions, CompilerEvent, CompilerErrorNamespace,
          CompilerErrorSeverity, CompilerPathCallbacks, CompilerFileSystemCallbacks, CompilerCallbackOptions,
diff --git a/common/web/types/src/util/common-events.ts b/common/web/types/src/util/common-events.ts
index eee8a6c84ca..5b3fd7d5070 100644
--- a/common/web/types/src/util/common-events.ts
+++ b/common/web/types/src/util/common-events.ts
@@ -1,3 +1,6 @@
+/*
+ * Keyman is copyright (C) SIL International. MIT License.
+ */
 import { CompilerErrorNamespace, CompilerErrorSeverity,  CompilerMessageDef as def, CompilerMessageSpec as m } from './compiler-interfaces.js';
 import { constants } from '@keymanapp/ldml-keyboard-constants';
 
@@ -43,4 +46,8 @@ export class CommonTypesMessages {
   static Error_TestDataUnexpectedArray = (o: {subtag: string}) =>
     m(this.ERROR_TestDataUnexpectedArray,
       `Problem reading test data: expected single ${def(o.subtag)} element, found multiple`);
+
+  static ERROR_InvalidXml = SevError | 0x0008;
+  static Error_InvalidXml = (o:{e: any}) =>
+  m(this.ERROR_InvalidXml, `The XML file could not be read: ${(o.e ?? '').toString()}`);
 };
diff --git a/developer/src/kmc-ldml/package.json b/developer/src/kmc-ldml/package.json
index ffb495ac7b5..a2b56a24f37 100644
--- a/developer/src/kmc-ldml/package.json
+++ b/developer/src/kmc-ldml/package.json
@@ -35,12 +35,14 @@
     "@keymanapp/developer-test-helpers": "*",
     "@keymanapp/resources-gosh": "*",
     "@types/chai": "^4.1.7",
+    "@types/common-tags": "^1.8.4",
     "@types/mocha": "^5.2.7",
     "@types/node": "^20.4.1",
     "@types/semver": "^7.3.12",
     "c8": "^7.12.0",
     "chai": "^4.3.4",
     "chalk": "^2.4.2",
+    "common-tags": "^1.8.2",
     "mocha": "^8.4.0",
     "ts-node": "^9.1.1",
     "typescript": "^4.9.5"
diff --git a/developer/src/kmc-ldml/src/compiler/compiler.ts b/developer/src/kmc-ldml/src/compiler/compiler.ts
index b972baf7509..85aa79c37b0 100644
--- a/developer/src/kmc-ldml/src/compiler/compiler.ts
+++ b/developer/src/kmc-ldml/src/compiler/compiler.ts
@@ -125,6 +125,8 @@ export class LdmlKeyboardCompiler implements KeymanCompiler {
       return null;
     }
 
+    outputFilename = outputFilename ?? inputFilename.replace(/\.xml$/, '.kmx');
+
     // In order for the KMX file to be loaded by non-KMXPlus components, it is helpful
     // to duplicate some of the metadata
     KMXPlusMetadataCompiler.addKmxMetadata(kmx.kmxplus, kmx.keyboard, compilerOptions);
@@ -134,7 +136,7 @@ export class LdmlKeyboardCompiler implements KeymanCompiler {
     const kmx_binary = builder.compile();
 
     const vkcompiler = new LdmlKeyboardVisualKeyboardCompiler(this.callbacks);
-    const vk = vkcompiler.compile(source);
+    const vk = vkcompiler.compile(kmx.kmxplus, this.callbacks.path.basename(outputFilename, '.kmx'));
     const writer = new KvkFileWriter();
     const kvk_binary = writer.write(vk);
 
@@ -149,7 +151,6 @@ export class LdmlKeyboardCompiler implements KeymanCompiler {
     //KMW17.0: const encoder = new TextEncoder();
     //KMW17.0: const kmw_binary = encoder.encode(kmw_string);
 
-    outputFilename = outputFilename ?? inputFilename.replace(/\.xml$/, '.kmx');
 
     return {
       artifacts: {
diff --git a/developer/src/kmc-ldml/src/compiler/keymanweb-compiler.ts b/developer/src/kmc-ldml/src/compiler/keymanweb-compiler.ts
deleted file mode 100644
index a35739247b4..00000000000
--- a/developer/src/kmc-ldml/src/compiler/keymanweb-compiler.ts
+++ /dev/null
@@ -1,113 +0,0 @@
-import { CompilerCallbacks, VisualKeyboard, LDMLKeyboard, TouchLayoutFileWriter, KeymanFileTypes } from "@keymanapp/common-types";
-import { LdmlCompilerOptions } from "./ldml-compiler-options.js";
-import { TouchLayoutCompiler } from "./touch-layout-compiler.js";
-import { LdmlKeyboardVisualKeyboardCompiler } from "./visual-keyboard-compiler.js";
-
-const MINIMUM_KMW_VERSION = '16.0';
-
-export class LdmlKeyboardKeymanWebCompiler {
-  private readonly options: LdmlCompilerOptions;
-  private readonly nl: string;
-  private readonly tab: string;
-  constructor(private callbacks: CompilerCallbacks, options?: LdmlCompilerOptions) {
-    this.options = { ...options };
-    this.nl = this.options.saveDebug ? "\n" : '';
-    this.tab = this.options.saveDebug ? "  " : '';
-  }
-
-  public compileVisualKeyboard(source: LDMLKeyboard.LDMLKeyboardXMLSourceFile) {
-    const nl = this.nl, tab = this.tab;
-    const vkc = new LdmlKeyboardVisualKeyboardCompiler(this.callbacks);
-    const vk: VisualKeyboard.VisualKeyboard = vkc.compile(source);
-
-    let result =
-      `{F: ' 1em ${JSON.stringify(vk.header.unicodeFont.name)}', `+
-      `K102: ${vk.header.flags & VisualKeyboard.VisualKeyboardHeaderFlags.kvkh102 ? 1 : 0}};${nl}` + // TODO-LDML: escape ' and " in font name correctly
-      `${tab}this.KV.KLS={${nl}` +
-      `${tab}${tab}TODO_LDML: ${vk.keys.length}${nl}` +
-      // TODO-LDML: fill in KLS
-      `${tab}}`;
-
-    return result;
-  }
-
-  public compileTouchLayout(source: LDMLKeyboard.LDMLKeyboardXMLSourceFile) {
-    const tlcompiler = new TouchLayoutCompiler();
-    const layout = tlcompiler.compileToJavascript(source);
-    const writer = new TouchLayoutFileWriter({formatted: this.options.saveDebug});
-    return writer.compile(layout);
-  }
-
-  private cleanName(name: string): string {
-    let result = this.callbacks.path.basename(name, KeymanFileTypes.Source.LdmlKeyboard);
-    if(!result) {
-      throw new Error(`Invalid file name ${name}`);
-    }
-
-    result = result.replaceAll(/[^a-z0-9]/g, '_');
-    if(result.match(/^[0-9]/)) {
-      // Can't have a digit as initial
-      result = '_' + result;
-    }
-    return result;
-  }
-
-  public compile(name: string, source: LDMLKeyboard.LDMLKeyboardXMLSourceFile): string {
-    const nl = this.nl, tab = this.tab;
-
-    const sName = 'Keyboard_'+this.cleanName(name);
-    const displayUnderlying = true; // TODO-LDML
-    const modifierBitmask = 0; // TODO-LDML: define the modifiers used by this keyboard
-    const vkDictionary = '';  // TODO-LDML: vk dictionary for touch keys
-    const hasSupplementaryPlaneChars = false; // TODO-LDML
-    const isRTL = false; // TODO-LDML
-
-    let result =
-      `if(typeof keyman === 'undefined') {${nl}` +
-      `${tab}console.error('Keyboard requires KeymanWeb ${MINIMUM_KMW_VERSION} or later');${nl}` +
-      `} else {${nl}` +
-      `${tab}KeymanWeb.KR(new ${sName}());${nl}` +
-      `}${nl}` +
-      `function ${sName}() {${nl}` +
-      // `${tab}${this.setupDebug()}${nl}` + ? we may use this for modifierBitmask in future
-      // `${tab}this._v=(typeof keyman!="undefined"&&typeof keyman.version=="string")?parseInt(keyman.version,10):9;${nl}` + ? we probably don't need this, it's for back-compat
-      `${tab}this.KI="${sName}";${nl}` +
-      `${tab}this.KN=${JSON.stringify(source.keyboard3.info.name)};${nl}` +
-      `${tab}this.KMINVER=${JSON.stringify(MINIMUM_KMW_VERSION)};${nl}` +
-      `${tab}this.KV=${this.compileVisualKeyboard(source)};${nl}` +
-      `${tab}this.KDU=${displayUnderlying ? '1' : '0'};${nl}` +
-      `${tab}this.KH="";${nl}` +  // TODO-LDML: help text not supported
-      `${tab}this.KM=0;${nl}` +  // TODO-LDML: mnemonic layout not supported for LDML keyboards
-      `${tab}this.KBVER=${JSON.stringify(source.keyboard3.version?.number || '0.0')};${nl}` +
-      `${tab}this.KMBM=${modifierBitmask};${nl}`;
-
-    if(isRTL) {
-      result += `${tab}this.KRTL=1;${nl}`;
-    }
-
-    if(hasSupplementaryPlaneChars) {
-      result += `${tab}this.KS=1;${nl}`;
-    }
-
-    if(vkDictionary != '') {
-      result += `${tab}this.KVKD=${JSON.stringify(vkDictionary)};${nl}`;
-    }
-
-    let layoutFile = this.compileTouchLayout(source);
-    if(layoutFile != '') {
-      result += `${tab}this.KVKL=${layoutFile};${nl}`;
-    }
-    // TODO-LDML: KCSS not supported
-
-    // TODO-LDML: embed binary keyboard for loading into Core
-
-    // A LDML keyboard has a no-op for its gs() (begin Unicode) function,
-    // because the functionality is embedded in Keyman Core
-    result += `${tab}this.gs=function(t,e){${nl}`+
-              `${tab}${tab}return 0;${nl}`+ // TODO-LDML: we will start by embedding call into Keyman Core here
-              `${tab}};${nl}`;
-
-    result += `}${nl}`;
-    return result;
-  }
-}
diff --git a/developer/src/kmc-ldml/src/compiler/touch-layout-compiler.ts b/developer/src/kmc-ldml/src/compiler/touch-layout-compiler.ts
deleted file mode 100644
index 5c5c7eb194e..00000000000
--- a/developer/src/kmc-ldml/src/compiler/touch-layout-compiler.ts
+++ /dev/null
@@ -1,112 +0,0 @@
-import { TouchLayout, LDMLKeyboard } from "@keymanapp/common-types";
-
-export class TouchLayoutCompiler {
-  public compileToJavascript(source: LDMLKeyboard.LDMLKeyboardXMLSourceFile): TouchLayout.TouchLayoutFile {
-    let result: TouchLayout.TouchLayoutFile = {};
-
-    // start with desktop to mimic vk emit
-    result.desktop = {
-      defaultHint: "none",  // TODO-LDML this should be optional
-      layer: []
-    };
-
-    for(let layers of source.keyboard3.layers) {
-      for(let layer of layers.layer) {
-        const resultLayer = this.compileHardwareLayer(source, result, layer);
-        result.desktop.layer.push(resultLayer);
-      }
-    }
-    return result;
-  }
-
-  private compileHardwareLayer(
-    source: LDMLKeyboard.LDMLKeyboardXMLSourceFile,
-    file: TouchLayout.TouchLayoutFile,
-    layer: LDMLKeyboard.LKLayer
-  ) {
-    // TODO-LDML: consider consolidation with keys.ts?
-
-    let fileLayer: TouchLayout.TouchLayoutLayer = {
-      id: this.translateLayerIdToTouchLayoutLayerId(layer.id, layer.modifiers),
-      row: []
-    };
-
-    let y = -1;
-
-    for(let row of layer.row) {
-      y++;
-
-      let fileRow: TouchLayout.TouchLayoutRow = {id: y, key: []};
-      fileLayer.row.push(fileRow);
-
-      const keys = row.keys.split(' ');
-      for(let key of keys) {
-        const keydef = source.keyboard3.keys?.key?.find(x => x.id == key);
-        if(keydef) {
-          const fileKey: TouchLayout.TouchLayoutKey = {
-            id: this.translateKeyIdentifierToTouch(keydef.id) as TouchLayout.TouchLayoutKeyId,
-            text: keydef.output || '',
-            // TODO-LDML: additional properties
-          };
-          fileRow.key.push(fileKey);
-        } else {
-          // TODO-LDML: consider logging missing keys
-        }
-      }
-    }
-
-    return fileLayer;
-  }
-
-  private translateLayerIdToTouchLayoutLayerId(id: string, modifier: string): string {
-    // Touch layout layers have a set of reserved names that correspond to
-    // hardware modifiers. We want to map these identifiers first before falling
-    // back to the layer ids
-
-    // The set of recognized layer identifiers is:
-    //
-    // touch          | LDML
-    // ---------------+-------------
-    // default        | none
-    // shift          | shift
-    // caps           | caps
-    // rightalt       | altR
-    // rightalt-shift | altR shift
-    //
-    const map = {
-      none:         'default',
-      shift:        'shift',
-      caps:         'caps',
-      altR:         'rightalt',
-      "altR shift": 'rightalt-shift'
-    };
-
-    // canonicalize modifier string, alphabetical
-    // TODO-LDML: need to support multiple here
-    if (modifier && modifier.indexOf(',') !== -1) {
-      throw Error(`Internal error: TODO-LDML: multiple modifiers ${modifier} not yet supported.`);
-    }
-    modifier = (modifier||'').split(/\b/).sort().join(' ').trim();
-
-    if(Object.hasOwn(map, modifier)) {
-      return (map as any)[modifier];
-    }
-
-    // TODO-LDML: Other layer names will be used unmodified, is this sufficient?
-    return id;
-  }
-
-  private translateKeyIdentifierToTouch(id: string): string {
-    // Note: keys identifiers in kmx were traditionally case-insensitive, but we
-    // are going to use them as case-insensitive for LDML keyboards. The set of
-    // standard key identifiers a-z, A-Z, 0-9 will be used, where possible, and
-    // all other keys will be mapped to `T_key`.
-
-    if(id.match(/^[0-9a-zA-Z]$/)) {
-      return 'K_'+id;
-    }
-
-    // Not a standard key
-    return 'T_'+id;
-  }
-}
diff --git a/developer/src/kmc-ldml/src/compiler/visual-keyboard-compiler.ts b/developer/src/kmc-ldml/src/compiler/visual-keyboard-compiler.ts
index d1a1d4e71a8..812e57ef824 100644
--- a/developer/src/kmc-ldml/src/compiler/visual-keyboard-compiler.ts
+++ b/developer/src/kmc-ldml/src/compiler/visual-keyboard-compiler.ts
@@ -1,5 +1,12 @@
-import { VisualKeyboard, LDMLKeyboard, CompilerCallbacks } from "@keymanapp/common-types";
-import { KeysCompiler } from "./keys.js";
+/*
+ * Keyman is copyright (C) SIL International. MIT License.
+ *
+ * Export LDML data (https://www.unicode.org/reports/tr35/tr35-keyboards.html)
+ * to .kvk format. This is an interim solution until Keyman Core supports
+ * interrogation of the KMX+ data for OSK.
+ */
+import { ModifierKeyConstants, KMXPlus } from "@keymanapp/common-types";
+import { VisualKeyboard, CompilerCallbacks } from "@keymanapp/common-types";
 import { CompilerMessages } from "./messages.js";
 
 // This is a partial polyfill for findLast, so not polluting Array.prototype
@@ -19,25 +26,37 @@ function findLast(arr: any, callback: any) {
   return undefined;
 }
 
+
+const LDML_MODIFIER_TO_KVK_MODIFIER = new Map<number, number>();
+LDML_MODIFIER_TO_KVK_MODIFIER.set(ModifierKeyConstants.LCTRLFLAG,      VisualKeyboard.VisualKeyboardShiftState.KVKS_LCTRL);
+LDML_MODIFIER_TO_KVK_MODIFIER.set(ModifierKeyConstants.RCTRLFLAG,      VisualKeyboard.VisualKeyboardShiftState.KVKS_RCTRL);
+LDML_MODIFIER_TO_KVK_MODIFIER.set(ModifierKeyConstants.LALTFLAG,       VisualKeyboard.VisualKeyboardShiftState.KVKS_LALT);
+LDML_MODIFIER_TO_KVK_MODIFIER.set(ModifierKeyConstants.RALTFLAG,       VisualKeyboard.VisualKeyboardShiftState.KVKS_RALT);
+LDML_MODIFIER_TO_KVK_MODIFIER.set(ModifierKeyConstants.K_SHIFTFLAG,    VisualKeyboard.VisualKeyboardShiftState.KVKS_SHIFT);
+LDML_MODIFIER_TO_KVK_MODIFIER.set(ModifierKeyConstants.K_CTRLFLAG,     VisualKeyboard.VisualKeyboardShiftState.KVKS_CTRL);
+LDML_MODIFIER_TO_KVK_MODIFIER.set(ModifierKeyConstants.K_ALTFLAG,      VisualKeyboard.VisualKeyboardShiftState.KVKS_ALT);
+
 export class LdmlKeyboardVisualKeyboardCompiler {
   public constructor(private callbacks: CompilerCallbacks) {
   }
 
-  public compile(source: LDMLKeyboard.LDMLKeyboardXMLSourceFile): VisualKeyboard.VisualKeyboard {
+  public compile(source: KMXPlus.KMXPlusData, keyboardId: string): VisualKeyboard.VisualKeyboard {
     let result = new VisualKeyboard.VisualKeyboard();
 
     /* TODO-LDML: consider VisualKeyboardHeaderFlags.kvkhUseUnderlying kvkhDisplayUnderlying kvkhAltGr kvkh102 */
     result.header.flags = 0;
     result.header.version = 0x0600;
-
-    /* TODO-LDML: consider associatedKeyboard: this _must_ be set to id (aka basename sans ext) of keyboard .kmx file */
-    result.header.associatedKeyboard = '';
+    result.header.associatedKeyboard = keyboardId;
     result.header.ansiFont = {...VisualKeyboard.DEFAULT_KVK_FONT};
     result.header.unicodeFont = {...VisualKeyboard.DEFAULT_KVK_FONT};
 
-    for(let layers of source.keyboard3.layers) {
-      const { formId } = layers;
-      for(let layer of layers.layer) {
+    for(let layersList of source.layr.lists) {
+      const formId = layersList.hardware.value;
+      if(formId == 'touch') {
+        continue;
+      }
+
+      for(let layer of layersList.layers) {
         this.compileHardwareLayer(source, result, layer, formId);
       }
     }
@@ -45,58 +64,74 @@ export class LdmlKeyboardVisualKeyboardCompiler {
   }
 
   private compileHardwareLayer(
-    source: LDMLKeyboard.LDMLKeyboardXMLSourceFile,
+    source: KMXPlus.KMXPlusData,
     vk: VisualKeyboard.VisualKeyboard,
-    layer: LDMLKeyboard.LKLayer,
+    layer: KMXPlus.LayrEntry,
     hardware: string,
   ) {
-    const layerId = layer.id;
-    if (hardware === 'touch') {
-      hardware = 'us'; // TODO-LDML: US Only. Do something different here?
-    }
-    const keymap = KeysCompiler.getKeymapFromForms(source.keyboard3?.forms?.form, hardware);
-    if (!keymap) {
-      this.callbacks.reportMessage(
-        CompilerMessages.Error_InvalidHardware({ formId: hardware })
-      );
+    const layerId = layer.id.value;
+
+    hardware = 'us'; // TODO-LDML: US Only. We need to clean this up for other hardware forms
+
+    const shift = this.translateLayerModifiersToVisualKeyboardShift(layer.mod);
+    if(shift === null) {
+      // Caps (num, scroll) is not a supported shift state in .kvk
       return;
     }
-    const shift = this.translateLayerIdToVisualKeyboardShift(layer.id);
 
     let y = -1;
-    for(let row of layer.row) {
+    for(let row of layer.rows) {
       y++;
-
-      const keys = row.keys.split(' ');
       let x = -1;
-      for(let key of keys) {
-        const keyId = key;
+      for(let key of row.keys) {
         x++;
 
-        //@ts-ignore
-        let keydef = findLast(source.keyboard3.keys?.key, x => x.id == key);
+        const keydef: KMXPlus.KeysKeys = findLast(source.keys?.keys, (kd: KMXPlus.KeysKeys) => kd.id.value == key.value);
+        const kmap = source.keys.kmap.find(k => k.key == keydef.id.value && k.mod == layer.mod);
+        const text = this.getDisplayFromKey(keydef, source) ?? null;
 
-        if (!keydef) {
+        if (!keydef || !kmap || text === null) {
           this.callbacks.reportMessage(
-            CompilerMessages.Error_KeyNotFoundInKeyBag({ keyId, layer: layerId, row: y, col: x, form: hardware })
+            CompilerMessages.Error_KeyNotFoundInKeyBag({ keyId: key.value, layer: layerId, row: y, col: x, form: hardware })
           );
         } else {
           vk.keys.push({
             flags: VisualKeyboard.VisualKeyboardKeyFlags.kvkkUnicode,
-            shift: shift,
-            text: keydef.output, // TODO-LDML: displays
-            vkey: keymap[y][x],
+            shift,
+            text,
+            vkey: kmap.vkey
           });
         }
       }
     }
   }
 
-  private translateLayerIdToVisualKeyboardShift(id: string) {
-    if(id == 'base') {
-      return 0;
+  private getDisplayFromKey(keydef: KMXPlus.KeysKeys, source: KMXPlus.KMXPlusData) {
+    const display = source.disp?.disps?.find(d => d.id.value == keydef.id.value || d.to.value == keydef.to.value);
+    return display?.display.value ?? keydef.to.value;
+  }
+
+  private translateLayerModifiersToVisualKeyboardShift(modifiers: number): VisualKeyboard.VisualKeyboardShiftState {
+
+    if(modifiers == 0) {
+      return VisualKeyboard.VisualKeyboardShiftState.KVKS_NORMAL;
     }
-    // TODO-LDML: other modifiers
-    return 0;
+
+    if(modifiers &
+      (ModifierKeyConstants.CAPITALFLAG | ModifierKeyConstants.NUMLOCKFLAG | ModifierKeyConstants.SCROLLFLAG)
+    ) {
+      // Caps/Num/Scroll are not supported in .kvk, in combination or alone
+      return null;
+    }
+
+    let shift: VisualKeyboard.VisualKeyboardShiftState = 0;
+
+    for(const mod of LDML_MODIFIER_TO_KVK_MODIFIER.keys()) {
+      if(modifiers & mod) {
+        shift |= LDML_MODIFIER_TO_KVK_MODIFIER.get(mod);
+      }
+    }
+
+    return shift;
   }
 }
diff --git a/developer/src/kmc-ldml/test/fixtures/basic-kvk.txt b/developer/src/kmc-ldml/test/fixtures/basic-kvk.txt
index 68666feacf3..da8f5e6011a 100644
--- a/developer/src/kmc-ldml/test/fixtures/basic-kvk.txt
+++ b/developer/src/kmc-ldml/test/fixtures/basic-kvk.txt
@@ -1,4 +1,6 @@
 #
+# Keyman is copyright (C) SIL International. MIT License.
+#
 # basic-kvk.txt describes the expected output of running kmc against basic.xml to generate
 # a .kvk file. It is used in the end-to-end test test-visual-keyboard-compiler-e2e.ts.
 #
@@ -10,8 +12,8 @@ block(kvkheader)
   00 06 00 00      # version 0x0600
   00               # flags
 block(associated_keyboard)
-  01 00            # string len in UTF-16 code units incl zterm
-  00 00            # '\0'
+  06 00            # string len in UTF-16 code units incl zterm
+  62 00 61 00 73 00 69 00 63 00 00 00           # 'basic\0'
 
 block(ansi_font)
   06 00            # string len in UTF-16 code units incl zterm
diff --git a/developer/src/kmc-ldml/test/fixtures/basic-no-debug.js b/developer/src/kmc-ldml/test/fixtures/basic-no-debug.js
deleted file mode 100644
index 84168414cf8..00000000000
--- a/developer/src/kmc-ldml/test/fixtures/basic-no-debug.js
+++ /dev/null
@@ -1 +0,0 @@
-if(typeof keyman === 'undefined') {console.error('Keyboard requires KeymanWeb 16.0 or later');} else {KeymanWeb.KR(new Keyboard_basic());}function Keyboard_basic() {this.KI="Keyboard_basic";this.KN="TestKbd";this.KMINVER="16.0";this.KV={F: ' 1em "Arial"', K102: 0};this.KV.KLS={TODO_LDML: 2};this.KDU=1;this.KH="";this.KM=0;this.KBVER="1.0.0";this.KMBM=0;this.KVKL={"desktop":{"defaultHint":"none","layer":[{"id":"base","row":[{"id":"0","key":[{"id":"T_hmaqtugha","text":"ħ"},{"id":"T_that","text":"ថា"}]}]}],"displayUnderlying":false}};this.gs=function(t,e){return 0;};}
diff --git a/developer/src/kmc-ldml/test/fixtures/basic.js b/developer/src/kmc-ldml/test/fixtures/basic.js
deleted file mode 100644
index c74798cfac5..00000000000
--- a/developer/src/kmc-ldml/test/fixtures/basic.js
+++ /dev/null
@@ -1,48 +0,0 @@
-if(typeof keyman === 'undefined') {
-  console.error('Keyboard requires KeymanWeb 16.0 or later');
-} else {
-  KeymanWeb.KR(new Keyboard_basic());
-}
-function Keyboard_basic() {
-  this.KI="Keyboard_basic";
-  this.KN="TestKbd";
-  this.KMINVER="16.0";
-  this.KV={F: ' 1em "Arial"', K102: 0};
-  this.KV.KLS={
-    TODO_LDML: 2
-  };
-  this.KDU=1;
-  this.KH="";
-  this.KM=0;
-  this.KBVER="1.0.0";
-  this.KMBM=0;
-  this.KVKL={
-  "desktop": {
-    "defaultHint": "none",
-    "layer": [
-      {
-        "id": "base",
-        "row": [
-          {
-            "id": "0",
-            "key": [
-              {
-                "id": "T_hmaqtugha",
-                "text": "ħ"
-              },
-              {
-                "id": "T_that",
-                "text": "ថា"
-              }
-            ]
-          }
-        ]
-      }
-    ],
-    "displayUnderlying": false
-  }
-};
-  this.gs=function(t,e){
-    return 0;
-  };
-}
diff --git a/developer/src/kmc-ldml/test/fixtures/basic.txt b/developer/src/kmc-ldml/test/fixtures/basic.txt
index b471228a52c..6302a5eb276 100644
--- a/developer/src/kmc-ldml/test/fixtures/basic.txt
+++ b/developer/src/kmc-ldml/test/fixtures/basic.txt
@@ -1,4 +1,6 @@
 #
+# Keyman is copyright (C) SIL International. MIT License.
+#
 # basic.txt describes the expected output of running kmc against basic.xml. It is used in
 # the end-to-end test test-compiler-e2e.ts.
 #
@@ -402,7 +404,7 @@ block(layr)                         # struct COMP_KMXPLUS_LAYR {
   01 00 00 00 # count
   7B 00 00 00                       #   KMX_DWORD minDeviceWidth; // 123
   # layers 0
-  index(strNull,strBase,2)          #   KMXPLUS_STR id;
+  00 00 00 00                       #   KMXPLUS_STR id;
   00 00 00 00                       #   KMX_DWORD mod
   00 00 00 00                       #   KMX_DWORD row index
   01 00 00 00                       #   KMX_DWORD  count
@@ -502,7 +504,6 @@ block(strs)                         #  struct COMP_KMXPLUS_STRS {
   diff(strs,strSet)   sizeof(strSet,2)
   diff(strs,strSet2)   sizeof(strSet2,2)
   diff(strs,strTranTo)          sizeof(strTranTo,2)
-  diff(strs,strBase)   sizeof(strBase,2)
   diff(strs,strElemBkspFrom2) sizeof(strElemBkspFrom2,2)
   diff(strs,strGapReserved) sizeof(strGapReserved,2)
   diff(strs,strHmaqtugha) sizeof(strHmaqtugha,2)
@@ -538,7 +539,6 @@ block(strs)                         #  struct COMP_KMXPLUS_STRS {
 #str #0A
   block(strSet2)             61 00 62 00 63 00 block(x) 00 00                           # 'abc'
   block(strTranTo)           61 00 02 03 block(x) 00 00                          # 'â' (U+0061 U+0302)
-  block(strBase)             62 00 61 00 73 00 65 00 block(x) 00 00                     # 'base'
   block(strElemBkspFrom2)    65 00 block(x) 00 00                                       # 'e'
   block(strGapReserved)         67 00 61 00 70 00 20 00 28 00 72 00 65 00 73 00 65 00 72 00 76 00 65 00 64 00 29 00 block(x) 00 00 # 'gap (reserved)'
   block(strHmaqtugha)        68 00 6d 00 61 00 71 00 74 00 75 00 67 00 68 00 61 00 block(x) 00 00   # 'hmaqtugha'
diff --git a/developer/src/kmc-ldml/test/fixtures/basic.xml b/developer/src/kmc-ldml/test/fixtures/basic.xml
index c8565271ed1..168ee0ef9a0 100644
--- a/developer/src/kmc-ldml/test/fixtures/basic.xml
+++ b/developer/src/kmc-ldml/test/fixtures/basic.xml
@@ -1,4 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
+<!-- Keyman is copyright (C) SIL International. MIT License. -->
 
 <!--
 @@keys: [K_Q][K_W][K_Q]
@@ -23,7 +24,8 @@
   </keys>
 
   <layers formId="us" minDeviceWidth="123">
-    <layer id="base">
+    <!-- TODO-LDML: unit test for <layers formId="us" with <layer id="base" should be illegal -->
+    <layer modifiers="none">
       <row keys="hmaqtugha that" />
     </layer>
   </layers>
diff --git a/developer/src/kmc-ldml/test/helpers/index.ts b/developer/src/kmc-ldml/test/helpers/index.ts
index 2e9e16ff6af..0fd659fd1b7 100644
--- a/developer/src/kmc-ldml/test/helpers/index.ts
+++ b/developer/src/kmc-ldml/test/helpers/index.ts
@@ -1,16 +1,17 @@
-/**
+/*
+ * Keyman is copyright (C) SIL International. MIT License.
+ *
  * Helpers and utilities for the Mocha tests.
  */
 import 'mocha';
 import * as path from 'path';
 import { fileURLToPath } from 'url';
 import { SectionCompiler, SectionCompilerNew } from '../../src/compiler/section-compiler.js';
-import { util, KMXPlus, LDMLKeyboardXMLSourceFileReader, VisualKeyboard, CompilerEvent, LDMLKeyboardTestDataXMLSourceFile, compilerEventFormat, LDMLKeyboard, UnicodeSetParser, CompilerCallbacks } from '@keymanapp/common-types';
+import { util, KMXPlus, LDMLKeyboardXMLSourceFileReader, CompilerEvent, LDMLKeyboardTestDataXMLSourceFile, compilerEventFormat, LDMLKeyboard, UnicodeSetParser, CompilerCallbacks } from '@keymanapp/common-types';
 import { LdmlKeyboardCompiler } from '../../src/main.js'; // make sure main.js compiles
 import { assert } from 'chai';
 import { KMXPlusMetadataCompiler } from '../../src/compiler/metadata-compiler.js';
 import { LdmlCompilerOptions } from '../../src/compiler/ldml-compiler-options.js';
-import { LdmlKeyboardVisualKeyboardCompiler } from '../../src/compiler/visual-keyboard-compiler.js';
 import { TestCompilerCallbacks } from '@keymanapp/developer-test-helpers';
 
 import KMXPlusFile = KMXPlus.KMXPlusFile;
@@ -20,7 +21,6 @@ import Section = KMXPlus.Section;
 import { ElemCompiler, ListCompiler, StrsCompiler } from '../../src/compiler/empty-compiler.js';
 import { KmnCompiler } from '@keymanapp/kmc-kmn';
 import { VarsCompiler } from '../../src/compiler/vars.js';
-// import Vars = KMXPlus.Vars;
 
 /**
  * Builds a path to the fixture with the given path components.
@@ -47,7 +47,7 @@ beforeEach(function() {
 
 afterEach(function() {
   if (this.currentTest.state !== 'passed') {
-    compilerTestCallbacks.messages.forEach(message => console.log(message.message));
+    compilerTestCallbacks.printMessages();
   }
 });
 
@@ -160,28 +160,7 @@ export async function compileKeyboard(inputFilename: string, options: LdmlCompil
   return kmx;
 }
 
-export async function compileVisualKeyboard(inputFilename: string, options: LdmlCompilerOptions): Promise<VisualKeyboard.VisualKeyboard> {
-  const k = new LdmlKeyboardCompiler();
-  assert.isTrue(await k.init(compilerTestCallbacks, options));
-  const source = k.load(inputFilename);
-  checkMessages();
-  assert.isNotNull(source, 'k.load should not have returned null');
-
-  const valid = await k.validate(source);
-  checkMessages();
-  assert.isTrue(valid, 'k.validate should not have failed');
-
-  const vk = (new LdmlKeyboardVisualKeyboardCompiler(compilerTestCallbacks)).compile(source);
-  checkMessages();
-  assert.isNotNull(vk, 'LdmlKeyboardVisualKeyboardCompiler.compile should not have returned null');
-
-  return vk;
-}
-
 export function checkMessages() {
-  if(compilerTestCallbacks.messages.length > 0) {
-    console.log(compilerTestCallbacks.messages);
-  }
   assert.isEmpty(compilerTestCallbacks.messages, compilerEventFormat(compilerTestCallbacks.messages));
 }
 
diff --git a/developer/src/kmc-ldml/test/test-keymanweb-compiler.ts b/developer/src/kmc-ldml/test/test-keymanweb-compiler.ts
deleted file mode 100644
index 9a14b38c33d..00000000000
--- a/developer/src/kmc-ldml/test/test-keymanweb-compiler.ts
+++ /dev/null
@@ -1,50 +0,0 @@
-import 'mocha';
-import { assert } from 'chai';
-import { checkMessages, compilerTestCallbacks, compilerTestOptions, makePathToFixture } from './helpers/index.js';
-import { LdmlKeyboardKeymanWebCompiler } from '../src/compiler/keymanweb-compiler.js';
-import { LdmlKeyboardCompiler } from '../src/compiler/compiler.js';
-import * as fs from 'fs';
-
-describe('LdmlKeyboardKeymanWebCompiler', function() {
-
-  it('should build a .js file', async function() {
-    // Let's build basic.xml
-    // It should generate content identical to basic.js
-    const inputFilename = makePathToFixture('basic.xml');
-    const outputFilename = makePathToFixture('basic.js');
-    const outputFilenameNoDebug = makePathToFixture('basic-no-debug.js');
-
-    // Load input data; we'll use the LDML keyboard compiler loader to save us
-    // effort here
-    const k = new LdmlKeyboardCompiler();
-    await k.init(compilerTestCallbacks, {...compilerTestOptions, saveDebug: true, shouldAddCompilerVersion: false});
-    const source = k.load(inputFilename);
-    checkMessages();
-    assert.isNotNull(source, 'k.load should not have returned null');
-
-    // Sanity check ... this is also checked in other tests
-    const valid = await k.validate(source);
-    checkMessages();
-    assert.isTrue(valid, 'k.validate should not have failed');
-
-    // Actual test: compile to javascript
-    const jsCompiler = new LdmlKeyboardKeymanWebCompiler(compilerTestCallbacks, {...compilerTestOptions, saveDebug: true});
-    const output = jsCompiler.compile('basic.xml', source);
-    assert.isNotNull(output);
-
-    // Does the emitted js match?
-    const outputFixture = fs.readFileSync(outputFilename, 'utf-8').replaceAll(/\r\n/g, '\n');
-    assert.strictEqual(output, outputFixture);
-
-    // Second test: compile to javascript without debug formatting
-    const jsCompilerNoDebug = new LdmlKeyboardKeymanWebCompiler(compilerTestCallbacks, {...compilerTestOptions, saveDebug: false});
-    const outputNoDebug = jsCompilerNoDebug.compile('basic.xml', source);
-    assert.isNotNull(outputNoDebug);
-
-    // Does the emitted js match? The nodebug has no newline at end, but allow one in the fixture
-    const outputFixtureNoDebug = fs.readFileSync(outputFilenameNoDebug, 'utf-8').replaceAll(/\r\n/g, '\n').trim();
-    assert.strictEqual(outputNoDebug, outputFixtureNoDebug);
-
-    // TODO(lowpri): consider using Typescript parser to generate AST for further validation
-  });
-});
diff --git a/developer/src/kmc-ldml/test/test-visual-keyboard-compiler-e2e.ts b/developer/src/kmc-ldml/test/test-visual-keyboard-compiler-e2e.ts
deleted file mode 100644
index df0aea03918..00000000000
--- a/developer/src/kmc-ldml/test/test-visual-keyboard-compiler-e2e.ts
+++ /dev/null
@@ -1,31 +0,0 @@
-import 'mocha';
-import {assert} from 'chai';
-import hextobin from '@keymanapp/hextobin';
-import { KvkFileWriter } from '@keymanapp/common-types';
-import {checkMessages,  compilerTestOptions,  compileVisualKeyboard, makePathToFixture} from './helpers/index.js';
-
-describe('visual-keyboard-compiler', function() {
-  this.slow(500); // 0.5 sec -- json schema validation takes a while
-
-  it('should build fixtures', async function() {
-    // Let's build basic.xml
-    // It should match basic.kvk (built from basic-kvk.txt)
-
-    const inputFilename = makePathToFixture('basic.xml');
-    const binaryFilename = makePathToFixture('basic-kvk.txt');
-
-    // Compile the visual keyboard
-    const vk = await compileVisualKeyboard(inputFilename, {...compilerTestOptions, saveDebug: true, shouldAddCompilerVersion: false});
-    assert.isNotNull(vk);
-
-    // Use the builder to generate the binary output file
-    const writer = new KvkFileWriter();
-    const code = writer.write(vk);
-    checkMessages();
-    assert.isNotNull(code);
-
-    // Compare output
-    let expected = await hextobin(binaryFilename, undefined, {silent:true});
-    assert.deepEqual<Uint8Array>(code, expected);
-  });
-});
diff --git a/developer/src/kmc-ldml/test/test-visual-keyboard-compiler.ts b/developer/src/kmc-ldml/test/test-visual-keyboard-compiler.ts
new file mode 100644
index 00000000000..cc44286909c
--- /dev/null
+++ b/developer/src/kmc-ldml/test/test-visual-keyboard-compiler.ts
@@ -0,0 +1,227 @@
+/*
+ * Keyman is copyright (C) SIL International. MIT License.
+ */
+import 'mocha';
+import * as path from 'path';
+import {assert} from 'chai';
+import { stripIndent } from 'common-tags';
+
+import { KvkFileWriter, LDMLKeyboardXMLSourceFileReader, VisualKeyboard } from '@keymanapp/common-types';
+import hextobin from '@keymanapp/hextobin';
+
+import { checkMessages, compilerTestCallbacks, compilerTestOptions, makePathToFixture } from './helpers/index.js';
+
+import { LdmlKeyboardVisualKeyboardCompiler } from '../src/compiler/visual-keyboard-compiler.js';
+import { LdmlKeyboardCompiler } from '../src/main.js';
+
+describe('visual-keyboard-compiler', function() {
+  this.slow(500); // 0.5 sec -- json schema validation takes a while
+
+  it('should build fixtures', async function() {
+    // Let's build basic.xml
+
+    // It should match basic.kvk (built from basic-kvk.txt)
+    const inputFilename = makePathToFixture('basic.xml');
+    const binaryFilename = makePathToFixture('basic-kvk.txt');
+
+    // Compile the visual keyboard
+    const k = new LdmlKeyboardCompiler();
+    assert.isTrue(await k.init(compilerTestCallbacks, {...compilerTestOptions, saveDebug: true, shouldAddCompilerVersion: false}));
+    const source = k.load(inputFilename);
+    checkMessages();
+    assert.isNotNull(source, 'k.load should not have returned null');
+
+    const valid = await k.validate(source);
+    checkMessages();
+    assert.isTrue(valid, 'k.validate should not have failed');
+
+    let kmx = await k.compile(source);
+    assert(kmx, 'k.compile should not have failed');
+
+    const vk = (new LdmlKeyboardVisualKeyboardCompiler(compilerTestCallbacks)).compile(kmx.kmxplus, path.basename(inputFilename, '.xml'));
+    checkMessages();
+    assert.isNotNull(vk, 'LdmlKeyboardVisualKeyboardCompiler.compile should not have returned null');
+
+    // Use the builder to generate the binary output file
+    const writer = new KvkFileWriter();
+    const code = writer.write(vk);
+    assert.isEmpty(compilerTestCallbacks.messages);
+    assert.isNotNull(code);
+
+    // Compare output
+    let expected = await hextobin(binaryFilename, undefined, {silent:true});
+
+    assert.deepEqual<Uint8Array>(code, expected);
+  });
+
+  it('should support various modifiers', async function() {
+    const xml = stripIndent`
+      <?xml version="1.0" encoding="UTF-8"?>
+      <keyboard3 xmlns="https://schemas.unicode.org/cldr/45/keyboard3" locale="mt" conformsTo="45">
+        <info name="minimal"/>
+        <keys>
+          <key id="x" output="y" />
+        </keys>
+        <layers formId="us">
+          <layer modifiers="none"><row keys="x" /></layer>
+          <layer modifiers="shift"><row keys="x" /></layer>
+          <layer modifiers="altR"><row keys="x" /></layer>
+          <layer modifiers="ctrlR altR, shift altR"><row keys="x" /></layer>
+          <layer modifiers="caps"><row keys="x" /></layer>
+        </layers>
+      </keyboard3>
+    `;
+
+    const vk = await loadVisualKeyboardFromXml(xml, 'test');
+
+    assert.equal(vk.keys.length, 5);
+    assert.equal(vk.keys[0].shift, VisualKeyboard.VisualKeyboardShiftState.KVKS_NORMAL);
+    assert.equal(vk.keys[1].shift, VisualKeyboard.VisualKeyboardShiftState.KVKS_SHIFT);
+    assert.equal(vk.keys[2].shift, VisualKeyboard.VisualKeyboardShiftState.KVKS_RALT);
+    assert.equal(vk.keys[3].shift, VisualKeyboard.VisualKeyboardShiftState.KVKS_RCTRL | VisualKeyboard.VisualKeyboardShiftState.KVKS_RALT);
+    assert.equal(vk.keys[4].shift, VisualKeyboard.VisualKeyboardShiftState.KVKS_SHIFT | VisualKeyboard.VisualKeyboardShiftState.KVKS_RALT);
+  });
+
+  it('should emit the correct associated keyboard id', async function() {
+    const xml = stripIndent`
+      <?xml version="1.0" encoding="UTF-8"?>
+      <keyboard3 xmlns="https://schemas.unicode.org/cldr/45/keyboard3" locale="mt" conformsTo="45">
+        <info name="minimal"/>
+        <keys>
+          <key id="x" output="y" />
+        </keys>
+        <layers formId="us">
+          <layer modifiers="none"><row keys="x" /></layer>
+        </layers>
+      </keyboard3>
+    `;
+
+    const vk = await loadVisualKeyboardFromXml(xml, 'test');
+
+    assert.equal(vk.header.associatedKeyboard, 'test');
+  });
+
+  it('should support <display>', async function() {
+    const xml = stripIndent`
+      <?xml version="1.0" encoding="UTF-8"?>
+      <keyboard3 xmlns="https://schemas.unicode.org/cldr/45/keyboard3" locale="mt" conformsTo="45">
+        <info name="minimal"/>
+        <keys>
+          <key id="a" output="X" />
+          <key id="b" output="Y" />
+        </keys>
+        <displays>
+          <display keyId="a" display="C" />
+          <display output="Y" display="D" />
+        </displays>
+        <layers formId="us">
+          <layer modifiers="none"><row keys="a b" /></layer>
+        </layers>
+      </keyboard3>
+    `;
+
+    const vk = await loadVisualKeyboardFromXml(xml, 'test');
+
+    assert.equal(vk.keys.length, 2);
+    assert.equal(vk.keys[0].text, 'C');
+    assert.equal(vk.keys[1].text, 'D');
+  });
+
+  it('should correctly decode \\u{xxxx}', async function() {
+    const xml = stripIndent`
+      <?xml version="1.0" encoding="UTF-8"?>
+      <keyboard3 xmlns="https://schemas.unicode.org/cldr/45/keyboard3" locale="mt" conformsTo="45">
+        <info name="minimal"/>
+        <keys>
+          <key id="x" output="\\u{0e80}" />
+          <key id="z" output="y" />
+        </keys>
+        <displays>
+          <display output="y" display="\\u{0e81}" />
+        </displays>
+        <layers formId="us">
+          <layer modifiers="none"><row keys="x z" /></layer>
+        </layers>
+      </keyboard3>
+    `;
+
+    const vk = await loadVisualKeyboardFromXml(xml, 'test');
+
+    assert.equal(vk.keys.length, 2);
+    assert.equal(vk.keys[0].text, '\u{0e80}');
+    assert.equal(vk.keys[1].text, '\u{0e81}');
+  });
+
+  it.skip('should read string variables in key.output', async function() {
+    const xml = stripIndent`
+      <?xml version="1.0" encoding="UTF-8"?>
+      <keyboard3 xmlns="https://schemas.unicode.org/cldr/45/keyboard3" locale="mt" conformsTo="45">
+        <info name="minimal"/>
+        <keys>
+          <key id="x" output="\${one}" />
+        </keys>
+        <layers formId="us">
+          <layer modifiers="none"><row keys="x" /></layer>
+        </layers>
+        <variables>
+          <string id="one" value="\${two}" />
+          <string id="two" value="2" />
+        </variables>
+      </keyboard3>
+    `;
+
+    const vk = await loadVisualKeyboardFromXml(xml, 'test');
+
+    assert.equal(vk.keys.length, 1);
+    assert.equal(vk.keys[0].text, '2');
+  });
+
+  it.skip('should read string variables in display.display', async function() {
+    const xml = stripIndent`
+      <?xml version="1.0" encoding="UTF-8"?>
+      <keyboard3 xmlns="https://schemas.unicode.org/cldr/45/keyboard3" locale="mt" conformsTo="45">
+        <info name="minimal"/>
+        <keys>
+          <key id="x" output="y" />
+        </keys>
+        <displays>
+          <display output="y" display="\${one}" />
+        </displays>
+        <layers formId="us">
+          <layer modifiers="none"><row keys="x" /></layer>
+        </layers>
+        <variables>
+          <string id="one" value="\${two}" />
+          <string id="two" value="2" />
+        </variables>
+      </keyboard3>
+    `;
+
+    const vk = await loadVisualKeyboardFromXml(xml, 'test');
+
+    assert.equal(vk.keys.length, 1);
+    assert.equal(vk.keys[0].text, '2');
+  });
+});
+
+async function loadVisualKeyboardFromXml(xml: string, id: string) {
+  const data = new TextEncoder().encode(xml);
+  assert.isOk(data);
+
+  const reader = new LDMLKeyboardXMLSourceFileReader(compilerTestOptions.readerOptions, compilerTestCallbacks);
+  const source = reader.load(data);
+  assert.isEmpty(compilerTestCallbacks.messages);
+  assert.isOk(source);
+
+  const k = new LdmlKeyboardCompiler();
+  assert.isTrue(await k.init(compilerTestCallbacks, compilerTestOptions));
+
+  const kmx = await k.compile(source);
+  assert(kmx, 'k.compile should not have failed');
+
+  const vk = (new LdmlKeyboardVisualKeyboardCompiler(compilerTestCallbacks)).compile(kmx.kmxplus, id);
+  assert.isEmpty(compilerTestCallbacks.messages);
+  assert.isOk(vk);
+
+  return vk;
+}
diff --git a/package-lock.json b/package-lock.json
index 31258179858..7706d512c61 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1794,12 +1794,14 @@
         "@keymanapp/developer-test-helpers": "*",
         "@keymanapp/resources-gosh": "*",
         "@types/chai": "^4.1.7",
+        "@types/common-tags": "^1.8.4",
         "@types/mocha": "^5.2.7",
         "@types/node": "^20.4.1",
         "@types/semver": "^7.3.12",
         "c8": "^7.12.0",
         "chai": "^4.3.4",
         "chalk": "^2.4.2",
+        "common-tags": "^1.8.2",
         "mocha": "^8.4.0",
         "ts-node": "^9.1.1",
         "typescript": "^4.9.5"
@@ -4196,6 +4198,13 @@
       "integrity": "sha512-mEo1sAde+UCE6b2hxn332f1g1E8WfYRu6p5SvTKr2ZKC1f7gFJXk4h5PyGP9Dt6gCaG8y8XhwnXWC6Iy2cmBng==",
       "dev": true
     },
+    "node_modules/@types/common-tags": {
+      "version": "1.8.4",
+      "resolved": "https://registry.npmjs.org/@types/common-tags/-/common-tags-1.8.4.tgz",
+      "integrity": "sha512-S+1hLDJPjWNDhcGxsxEbepzaxWqURP/o+3cP4aa2w7yBXgdcmKGQtZzP8JbyfOd0m+33nh+8+kvxYE2UJtBDkg==",
+      "dev": true,
+      "license": "MIT"
+    },
     "node_modules/@types/component-emitter": {
       "version": "1.2.11",
       "dev": true,
@@ -5611,6 +5620,16 @@
         "node": ">=14"
       }
     },
+    "node_modules/common-tags": {
+      "version": "1.8.2",
+      "resolved": "https://registry.npmjs.org/common-tags/-/common-tags-1.8.2.tgz",
+      "integrity": "sha512-gk/Z852D2Wtb//0I+kRFNKKE9dIIVirjoqPoA1wJU+XePVXZfGeBpk45+A1rKO4Q43prqWBNY/MiIeRLbPWUaA==",
+      "dev": true,
+      "license": "MIT",
+      "engines": {
+        "node": ">=4.0.0"
+      }
+    },
     "node_modules/component-emitter": {
       "version": "1.3.0",
       "dev": true,

From 94a4ac4eaeee754b5bb404b66f21f5f6f846294e Mon Sep 17 00:00:00 2001
From: Marc Durdin <marc@durdin.net>
Date: Mon, 16 Sep 2024 14:00:39 +0700
Subject: [PATCH 2/2] fix(developer): LDML compiler, add TSS_VISUALKEYBOARD
 store when compiling visual keyboard

Fixes: #12395
Cherry-pick-of: #12402
---
 .../src/kmc-ldml/src/compiler/compiler.ts     | 33 +++++++++++++------
 .../kmc-ldml/src/compiler/empty-compiler.ts   |  2 +-
 .../src/compiler/visual-keyboard-compiler.ts  | 33 +++++++++++++++++--
 .../test/test-visual-keyboard-compiler.ts     | 12 +++++--
 4 files changed, 64 insertions(+), 16 deletions(-)

diff --git a/developer/src/kmc-ldml/src/compiler/compiler.ts b/developer/src/kmc-ldml/src/compiler/compiler.ts
index 85aa79c37b0..6e923f98559 100644
--- a/developer/src/kmc-ldml/src/compiler/compiler.ts
+++ b/developer/src/kmc-ldml/src/compiler/compiler.ts
@@ -1,4 +1,4 @@
-import { LDMLKeyboardXMLSourceFileReader, LDMLKeyboard, KMXPlus, CompilerCallbacks, LDMLKeyboardTestDataXMLSourceFile, UnicodeSetParser, KeymanCompiler, KeymanCompilerResult, KeymanCompilerArtifacts, defaultCompilerOptions, KMXBuilder, KvkFileWriter, KeymanCompilerArtifactOptional } from '@keymanapp/common-types';
+import { KMX, LDMLKeyboardXMLSourceFileReader, LDMLKeyboard, KMXPlus, CompilerCallbacks, LDMLKeyboardTestDataXMLSourceFile, UnicodeSetParser, KeymanCompiler, KeymanCompilerResult, KeymanCompilerArtifacts, defaultCompilerOptions, KMXBuilder, KvkFileWriter, KeymanCompilerArtifactOptional } from '@keymanapp/common-types';
 import { LdmlCompilerOptions } from './ldml-compiler-options.js';
 import { CompilerMessages } from './messages.js';
 import { BkspCompiler, TranCompiler } from './tran.js';
@@ -132,13 +132,27 @@ export class LdmlKeyboardCompiler implements KeymanCompiler {
     KMXPlusMetadataCompiler.addKmxMetadata(kmx.kmxplus, kmx.keyboard, compilerOptions);
 
     // Use the builder to generate the binary output file
-    const builder = new KMXBuilder(kmx, compilerOptions.saveDebug);
-    const kmx_binary = builder.compile();
+    const kmxBuilder = new KMXBuilder(kmx, compilerOptions.saveDebug);
+    const keyboardId = this.callbacks.path.basename(outputFilename, '.kmx');
+    const vkCompiler = new LdmlKeyboardVisualKeyboardCompiler(this.callbacks);
+    const vkCompilerResult = vkCompiler.compile(kmx.kmxplus, keyboardId);
+    if(vkCompilerResult === null) {
+      return null;
+    }
+    const vkData = typeof vkCompilerResult == 'object' ? vkCompilerResult : null;
 
-    const vkcompiler = new LdmlKeyboardVisualKeyboardCompiler(this.callbacks);
-    const vk = vkcompiler.compile(kmx.kmxplus, this.callbacks.path.basename(outputFilename, '.kmx'));
-    const writer = new KvkFileWriter();
-    const kvk_binary = writer.write(vk);
+    if(vkData) {
+      kmx.keyboard.stores.push({
+        dpName: '',
+        dpString: keyboardId + '.kvk',
+        dwSystemID: KMX.KMXFile.TSS_VISUALKEYBOARD
+      });
+    }
+
+    const kmxBinary = kmxBuilder.compile();
+
+    const kvkWriter = new KvkFileWriter();
+    const kvkBinary = vkData ? kvkWriter.write(vkData) : null;
 
     // Note: we could have a step of generating source files here
     // KvksFileWriter()...
@@ -151,11 +165,10 @@ export class LdmlKeyboardCompiler implements KeymanCompiler {
     //KMW17.0: const encoder = new TextEncoder();
     //KMW17.0: const kmw_binary = encoder.encode(kmw_string);
 
-
     return {
       artifacts: {
-        kmx: { data: kmx_binary, filename: outputFilename },
-        kvk: { data: kvk_binary, filename: outputFilename.replace(/\.kmx$/, '.kvk') },
+        kmx: { data: kmxBinary, filename: outputFilename },
+        kvk: kvkBinary ? { data: kvkBinary, filename: outputFilename.replace(/\.kmx$/, '.kvk') } : null,
         //KMW17.0: js: { data: kmw_binary, filename: outputFilename.replace(/\.kmx$/, '.js') },
       }
     };
diff --git a/developer/src/kmc-ldml/src/compiler/empty-compiler.ts b/developer/src/kmc-ldml/src/compiler/empty-compiler.ts
index 911e56d8961..514024fedd6 100644
--- a/developer/src/kmc-ldml/src/compiler/empty-compiler.ts
+++ b/developer/src/kmc-ldml/src/compiler/empty-compiler.ts
@@ -5,7 +5,7 @@ import { VarsCompiler } from './vars.js';
 import { CompilerMessages } from './messages.js';
 
 /**
- * Compiler for typrs that don't actually consume input XML
+ * Compiler for types that don't actually consume input XML
  */
 export abstract class EmptyCompiler extends SectionCompiler {
   private _id: SectionIdent;
diff --git a/developer/src/kmc-ldml/src/compiler/visual-keyboard-compiler.ts b/developer/src/kmc-ldml/src/compiler/visual-keyboard-compiler.ts
index 812e57ef824..cf027043bab 100644
--- a/developer/src/kmc-ldml/src/compiler/visual-keyboard-compiler.ts
+++ b/developer/src/kmc-ldml/src/compiler/visual-keyboard-compiler.ts
@@ -40,7 +40,15 @@ export class LdmlKeyboardVisualKeyboardCompiler {
   public constructor(private callbacks: CompilerCallbacks) {
   }
 
-  public compile(source: KMXPlus.KMXPlusData, keyboardId: string): VisualKeyboard.VisualKeyboard {
+  /**
+   * Generate a visual keyboard
+   * @param source      Compiled KMX+ data; note that this is modified to add
+   *                    &VISUALKEYBOARD system store on success
+   * @param keyboardId  Basename of keyboard, without file extension
+   * @returns           Visual keyboard data on success, null on failure, or
+   *                    false if no VK was generated for this keyboard
+   */
+  public compile(source: KMXPlus.KMXPlusData, keyboardId: string): VisualKeyboard.VisualKeyboard | boolean | null {
     let result = new VisualKeyboard.VisualKeyboard();
 
     /* TODO-LDML: consider VisualKeyboardHeaderFlags.kvkhUseUnderlying kvkhDisplayUnderlying kvkhAltGr kvkh102 */
@@ -50,6 +58,8 @@ export class LdmlKeyboardVisualKeyboardCompiler {
     result.header.ansiFont = {...VisualKeyboard.DEFAULT_KVK_FONT};
     result.header.unicodeFont = {...VisualKeyboard.DEFAULT_KVK_FONT};
 
+    let hasVisualKeyboard = false;
+
     for(let layersList of source.layr.lists) {
       const formId = layersList.hardware.value;
       if(formId == 'touch') {
@@ -57,9 +67,23 @@ export class LdmlKeyboardVisualKeyboardCompiler {
       }
 
       for(let layer of layersList.layers) {
-        this.compileHardwareLayer(source, result, layer, formId);
+        const res = this.compileHardwareLayer(source, result, layer, formId);
+        if(res === false) {
+          // failed to compile the layer
+          return null;
+        }
+        if(res === null) {
+          // not a supported layer type, but not an error
+          continue;
+        }
+        hasVisualKeyboard = true;
       }
     }
+
+    if(!hasVisualKeyboard) {
+      return false;
+    }
+
     return result;
   }
 
@@ -76,9 +100,10 @@ export class LdmlKeyboardVisualKeyboardCompiler {
     const shift = this.translateLayerModifiersToVisualKeyboardShift(layer.mod);
     if(shift === null) {
       // Caps (num, scroll) is not a supported shift state in .kvk
-      return;
+      return null;
     }
 
+    let result = true;
     let y = -1;
     for(let row of layer.rows) {
       y++;
@@ -94,6 +119,7 @@ export class LdmlKeyboardVisualKeyboardCompiler {
           this.callbacks.reportMessage(
             CompilerMessages.Error_KeyNotFoundInKeyBag({ keyId: key.value, layer: layerId, row: y, col: x, form: hardware })
           );
+          result = false;
         } else {
           vk.keys.push({
             flags: VisualKeyboard.VisualKeyboardKeyFlags.kvkkUnicode,
@@ -104,6 +130,7 @@ export class LdmlKeyboardVisualKeyboardCompiler {
         }
       }
     }
+    return result;
   }
 
   private getDisplayFromKey(keydef: KMXPlus.KeysKeys, source: KMXPlus.KMXPlusData) {
diff --git a/developer/src/kmc-ldml/test/test-visual-keyboard-compiler.ts b/developer/src/kmc-ldml/test/test-visual-keyboard-compiler.ts
index cc44286909c..dee096cb221 100644
--- a/developer/src/kmc-ldml/test/test-visual-keyboard-compiler.ts
+++ b/developer/src/kmc-ldml/test/test-visual-keyboard-compiler.ts
@@ -6,7 +6,7 @@ import * as path from 'path';
 import {assert} from 'chai';
 import { stripIndent } from 'common-tags';
 
-import { KvkFileWriter, LDMLKeyboardXMLSourceFileReader, VisualKeyboard } from '@keymanapp/common-types';
+import { KMX, KvkFileWriter, LDMLKeyboardXMLSourceFileReader, VisualKeyboard } from '@keymanapp/common-types';
 import hextobin from '@keymanapp/hextobin';
 
 import { checkMessages, compilerTestCallbacks, compilerTestOptions, makePathToFixture } from './helpers/index.js';
@@ -38,9 +38,16 @@ describe('visual-keyboard-compiler', function() {
     let kmx = await k.compile(source);
     assert(kmx, 'k.compile should not have failed');
 
-    const vk = (new LdmlKeyboardVisualKeyboardCompiler(compilerTestCallbacks)).compile(kmx.kmxplus, path.basename(inputFilename, '.xml'));
+    const keyboardId = path.basename(inputFilename, '.xml');
+
+    const vk = (new LdmlKeyboardVisualKeyboardCompiler(compilerTestCallbacks)).compile(kmx.kmxplus, keyboardId);
     checkMessages();
     assert.isNotNull(vk, 'LdmlKeyboardVisualKeyboardCompiler.compile should not have returned null');
+    assert.isNotNull(kmx.keyboard.stores.find(store =>
+      store.dwSystemID == KMX.KMXFile.TSS_VISUALKEYBOARD &&
+      store.dpString == keyboardId + '.kvk'
+    ));
+    assert(typeof vk == 'object');
 
     // Use the builder to generate the binary output file
     const writer = new KvkFileWriter();
@@ -220,6 +227,7 @@ async function loadVisualKeyboardFromXml(xml: string, id: string) {
   assert(kmx, 'k.compile should not have failed');
 
   const vk = (new LdmlKeyboardVisualKeyboardCompiler(compilerTestCallbacks)).compile(kmx.kmxplus, id);
+  assert(typeof vk == 'object');
   assert.isEmpty(compilerTestCallbacks.messages);
   assert.isOk(vk);