Merge pull request #10662 from keymanapp/fix/web/transform-selection-interactions

fix(web): corrects Android over-deletion of selected text, other context diffs involving selected text

common/web/keyboard-processor/src/index.ts

@@ -39,6 +39,7 @@ export { default as KeyMapping } from "./text/keyMapping.js";
 export { default as OutputTarget } from "./text/outputTarget.js";
 export * from "./text/outputTarget.js";
 export { default as RuleBehavior } from "./text/ruleBehavior.js";
+export * from "./text/stringDivergence.js";
 export * from "./text/systemStores.js";
 
 export * from "@keymanapp/web-utils";

common/web/keyboard-processor/src/text/outputTarget.ts

@@ -1,6 +1,7 @@
 ///<reference types="@keymanapp/models-types" />
 
 import { extendString } from "@keymanapp/web-utils";
+import { findCommonSubstringEndIndex } from "./stringDivergence.js";
 
 extendString();
 
@@ -120,85 +121,24 @@ export default abstract class OutputTarget {
    * @param from An output target (preferably a Mock) representing the prior state of the input/output system.
    */
   buildTransformFrom(original: OutputTarget): Transform {
-    let to = this.getText();
-    let from = original.getText();
+    const toLeft = this.getTextBeforeCaret();
+    const fromLeft = original.getTextBeforeCaret();
 
-    let fromCaret = original.getDeadkeyCaret();
-    let toCaret = this.getDeadkeyCaret();
+    const leftDivergenceIndex = findCommonSubstringEndIndex(fromLeft, toLeft, false);
+    const deletedLeft = fromLeft.substring(leftDivergenceIndex)._kmwLength();
+    // No need for our specialized variant here.
+    const insertedText = toLeft.substring(leftDivergenceIndex);
 
-    // Step 1:  Determine the number of left-deletions.
-    let maxSMPLeftMatch = fromCaret < toCaret ? fromCaret : toCaret;
+    const toRight = this.getTextAfterCaret();
+    const fromRight = original.getTextAfterCaret();
+    const rightDivergenceIndex = findCommonSubstringEndIndex(fromRight, toRight, true);
 
-    // We need the corresponding non-SMP caret location in order to binary-search efficiently.
-    // (Examining code units is much more computationally efficient.)
-    let maxLeftMatch = to._kmwCodePointToCodeUnit(maxSMPLeftMatch);
+    // Right insertions aren't supported, but right deletions will matter in some scenarios.
+    // In particular, once we allow right-deletion for pred-text suggestions applied with the
+    // caret mid-word..
+    const deletedRight = fromRight.substring(0, rightDivergenceIndex + 1)._kmwLength();
 
-    // 1.1:  use a non-SMP-aware binary search to determine the divergence point.
-    let start = 0;
-    let end = maxLeftMatch;  // the index AFTER the last possible matching char.
-
-    // This search is O(maxLeftMatch).  1/2 + 1/4 + 1/8 + ... converges to = 1.
-    while(start < end) {
-      let mid = Math.floor((end+start+1) / 2); // round up (compare more)
-      let fromLeft = from.substr(start, mid-start);
-      let toLeft   = to.substr(start, mid-start);
-
-      if(fromLeft == toLeft) {
-        start = mid;
-      } else {
-        end = mid - 1;
-      }
-    }
-
-    // At the loop's end:  `end` now holds the non-SMP-aware divergence point.
-    // The 'caret' is after the last matching code unit.
-
-    // 1.2:  detect a possible surrogate-pair split scenario, correcting for it
-    //       (by moving the split before the high-surrogate) if detected.
-
-    // If the split location is precisely on either end of the context, we can't
-    // have split a surrogate pair.
-    if(end > 0 && end < maxLeftMatch) {
-      let potentialHigh    = from.charCodeAt(end-1);
-      let potentialFromLow = from.charCodeAt(end);
-      let potentialToLow   = to.charCodeAt(end);
-
-      // if potentialHigh is a possible high surrogate...
-      if(potentialHigh >= 0xD800 && potentialHigh <= 0xDBFF) {
-        // and at least one potential 'low' is a possible low surrogate...
-        let flag = potentialFromLow >= 0xDC00 && potentialFromLow <= 0xDFFF;
-        flag = flag || (potentialToLow >= 0XDC00 && potentialToLow <= 0xDFFF);
-
-        // Correct the split location, moving it 'before' the high surrogate.
-        if(flag) {
-          end = end - 1;
-        }
-      }
-    }
-
-    // 1.3:  take substring from start to the split point; determine SMP-aware length.
-    //       This yields the SMP-aware divergence index, which gives the number of left-deletes.
-    let newCaret = from._kmwCodeUnitToCodePoint(end);
-    let deletedLeft = fromCaret - newCaret;
-
-    // Step 2:  Determine the other properties.
-    // Since the 'after' OutputTarget's caret indicates the end of any inserted text, we
-    // can easily calculate the rest.
-    let insertedLength = toCaret - newCaret;
-    let delta = to._kmwSubstr(newCaret, insertedLength);
-
-    let undeletedRight = to._kmwLength() - toCaret;
-    let originalRight = from._kmwLength() - fromCaret;
-    let deletedRight = originalRight - undeletedRight;
-
-    // May occur when reverting a suggestion that had been applied mid-word.
-    if(deletedRight < 0) {
-      // Restores deleteRight characters.
-      delta = delta + to._kmwSubstr(toCaret, -deletedRight);
-      deletedRight = 0;
-    }
-
-    return new TextTransform(delta, deletedLeft, deletedRight);
+    return new TextTransform(insertedText, deletedLeft, deletedRight);
   }
 
   buildTranscriptionFrom(original: OutputTarget, keyEvent: KeyEvent, readonly: boolean, alternates?: Alternate[]): Transcription {
@@ -214,6 +154,13 @@ export default abstract class OutputTarget {
    * @param original An `OutputTarget` (usually a `Mock`).
    */
   restoreTo(original: OutputTarget) {
+    this.clearSelection();
+    // We currently do not restore selected text; the mechanism isn't supported at present for
+    // all output target types - especially in regard to re-selecting the text if restored.
+    //
+    // I believe this would mostly matter if/when reverting predictions based upon selected text.
+    // That pattern isn't well-supported yet, though.
+
     //
     this.setTextBeforeCaret(original.getTextBeforeCaret());
     this.setTextAfterCaret(original.getTextAfterCaret());
@@ -223,6 +170,10 @@ export default abstract class OutputTarget {
   }
 
   apply(transform: Transform) {
+    // Selected text should disappear on any text edit; application of a transform
+    // certainly qualifies.
+    this.clearSelection();
+
     if(transform.deleteRight) {
       this.setTextAfterCaret(this.getTextAfterCaret()._kmwSubstr(transform.deleteRight));
     }

common/web/keyboard-processor/src/text/stringDivergence.ts

@@ -0,0 +1,93 @@
+// Future TODO:  import from @keymanapp/common-types... once we no longer need to support ES5.
+import { Uni_IsSurrogate1, Uni_IsSurrogate2 } from '@keymanapp/web-utils';
+
+/**
+ * Returns the index for the code point divergence point between two strings, as measured in code
+ * unit coordinates.
+ * @param str1
+ * @param str2
+ * @param commonSuffix If false, asserts a common prefix to the strings.  If true, asserts a common suffix.
+ * @returns The code unit index within `str1` for the start of the code point not common to both.
+ *
+ * Follows the convention of (start, end) substring parameterizations having 'end' be exclusive.
+ */
+export function findCommonSubstringEndIndex(str1: string, str2: string, commonSuffix: boolean): number {
+  /**
+   * The maximum number of iterations to consider; exceeding this would go past a string boundary.
+   */
+  const maxInterval = Math.min(str1.length, str2.length);
+
+  /**
+   * The first valid index within the string.
+   */
+  let start: number;
+
+  /**
+   * The current index within the string under consideration as the divergence point.
+   */
+  let index: number;
+
+  /**
+   * The index at which to terminate the search for a divergence point.
+   */
+  let end: number;
+
+  /**
+   * Index shift per loop iteration.
+   */
+  let inc: number;
+
+  /**
+   * Difference in index for comparison between strings.
+   * Mostly matters when assuming a common right-hand side.
+   */
+  let offset: number;
+
+  if(commonSuffix) {
+    start = index = str1.length - 1; // e.g. str.length == 10 => start = 9.
+    end = index - maxInterval;       // e.g. maxInterval 8, start 9 => iterate from 9 to 2, end at 1.
+    inc = -1;
+    offset = str2.length - str1.length;
+  } else {
+    start = index = 0;
+    end = maxInterval; // last valid index: - 1.  e.g. maxInterval 8 => iterate from 0 to 7, end at 8.
+    inc = 1;
+    offset = 0;
+  }
+
+  // Step 1: Find the index for the first code unit different between the strings.
+  for(; index != end; index += inc) {
+    if(str1.charAt(index) != str2.charAt(index + offset)) {
+      break;
+    }
+  }
+
+  // Step 2:  Ensure that we're not splitting a surrogate pair.
+
+  // `index` corresponds to the first char that is different _in the direction indicated by inc_.
+  // If it's the start position, it can't split a (completed) surrogate pair.
+  if(index != start && index != end) {
+    // if commonLeft, high surrogate; if commonRight, low surrogate.
+    const commonPotentialSurrogate = str1.charCodeAt(index - inc);
+    // Opposite surrogate type from the previous variable.
+    const divergentChar1 = str1.charCodeAt(index);
+    const divergentChar2 = str2.charCodeAt(index + offset);
+
+    const commonSurrogateChecker = commonSuffix ? Uni_IsSurrogate2 : Uni_IsSurrogate1;
+    const divergentSurrogateChecker = commonSuffix ? Uni_IsSurrogate1 : Uni_IsSurrogate2;
+
+    // If the last common character if of the direction-appropriate surrogate type (for
+    // comprising a potential split surrogate pair representing a non-BMP char)...
+    if(commonSurrogateChecker(commonPotentialSurrogate)) {
+      // And one of the two divergent chars is a qualifying match - a surrogate
+      // of the opposite type...
+      if(divergentSurrogateChecker(divergentChar1) || divergentSurrogateChecker(divergentChar2)) {
+        // Our current index would split a surrogate pair; decrement the index to
+        // preserve the pair.
+        return index - inc;
+      }
+    }
+  }
+
+  return index;
+}

common/web/keyboard-processor/tests/node/transcriptions.js

@@ -1,22 +1,161 @@
 import { assert } from 'chai';
 
-import { Mock } from '@keymanapp/keyboard-processor';
+import { Mock, findCommonSubstringEndIndex } from '@keymanapp/keyboard-processor';
 import { extendString } from '@keymanapp/web-utils';
 
 extendString();  // Ensure KMW's string-extension functionality is available.
 
 String.kmwEnableSupplementaryPlane(false);
 
+// A unicode-coding like alias for use in constructing non-BMP strings.
+const u = String.fromCodePoint;
+
+/**
+ * Returns the "Mathematical Sans-Serif Small" non-BMP encoding for
+ * a passed-in lowercase char between 'a' and 'z', inclusive.
+ * @param {*} char
+ * @returns
+ */
+const ss = (char) => {
+  const charCodeOffset = char.charCodeAt(0) - 'a'.charCodeAt(0);
+  return u(0x1d5ba + charCodeOffset);
+}
+
+describe("String divergence calculations", function() {
+  describe("Common prefix", () => {
+    it("BMP text", () => {
+      const result1 = findCommonSubstringEndIndex("apple", "applause", false);
+      assert.equal(result1, 4);
+
+      const result2 = findCommonSubstringEndIndex("applesauce", "applause", false);
+      assert.equal(result2, 4);
+    });
+
+    it("BMP edge cases", () => {
+      const result1 = findCommonSubstringEndIndex("applesauce", "applesauce", false);
+      assert.equal(result1, 10);
+
+      const result2 = findCommonSubstringEndIndex("applesauce", "banana bread", false);
+      assert.equal(result2, 0);
+    });
+
+    it("non-BMP text", () => {
+      const smp_ify = (str) => str.split('').map(ss).join('');
+
+      const result1 = findCommonSubstringEndIndex(
+        smp_ify('apple'),
+        smp_ify('applause'),
+        false
+      );
+
+      // 2 per non-BMP char; is in code-unit... units.
+      // Will avoid splitting code points, though.
+      assert.equal(result1, 8);
+
+      const result2 = findCommonSubstringEndIndex(
+        smp_ify('applesauce'),
+        smp_ify('applause'),
+        false
+      );
+
+      assert.equal(result2, 8);
+    });
+
+    it("non-BMP edge cases", () => {
+      const smp_ify = (str) => str.split('').map(ss).join('');
+
+      const result1 = findCommonSubstringEndIndex(
+        smp_ify('applesauce'),
+        smp_ify('applesauce'),
+        false
+      );
+
+      assert.equal(result1, 20);
+
+      const result2 = findCommonSubstringEndIndex(
+        smp_ify('applesauce'),
+        smp_ify('banana bread'),
+        false
+      );
+
+      assert.equal(result2, 0);
+    })
+  });
+
+  describe("Common suffix", () => {
+    it("BMP text", () => {
+      //    att|endance
+      // transc|endance
+      const result1 = findCommonSubstringEndIndex("attendance", "transcendance", true);
+      assert.equal(result1, 2);
+
+      // transcend|ance
+      //  happenst|ance
+      const result2 = findCommonSubstringEndIndex("transcendance", "happenstance", true);
+      assert.equal(result2, 8);
+
+    });
+
+    it("BMP edge cases", () => {
+      // If the two are equal...
+      const result1 = findCommonSubstringEndIndex("post-caret text", "post-caret text", true);
+      assert.equal(result1, -1);
+
+      // If the two are completely different...
+      const result2 = findCommonSubstringEndIndex("post-caret text", "supercalifragilistic", true);
+      assert.equal(result2, "post-caret text".length-1);
+    })
+
+    it("non-BMP text", () => {
+      const smp_ify = (str) => str.split('').map(ss).join('');
+
+      //   att|endance
+      // trans|endance
+      const result1 = findCommonSubstringEndIndex(
+        smp_ify("attendance"),
+        smp_ify("transcendance"),
+        true
+      );
+
+      // 2 per non-BMP char; is in code-unit... units.
+      // Will avoid splitting code points; is odd b/c we get the index of the LAST char of the pair.
+      assert.equal(result1, 5);
+
+      // transcend|ance
+      //  happenst|ance
+      const result2 = findCommonSubstringEndIndex(
+        smp_ify("transcendance"),
+        smp_ify("happenstance"),
+        true
+      );
+      assert.equal(result2, 17);
+
+    });
+
+    it("non-BMP edge cases", () => {
+      const smp_ify = (str) => str.split('').map(ss).join('');
+
+      // If the two are equal...
+      const result3 = findCommonSubstringEndIndex(
+        smp_ify("post-caret text"),
+        smp_ify("post-caret text"),
+        true
+      );
+      assert.equal(result3, -1);
+
+      // If the two are completely different...
+      const result2 = findCommonSubstringEndIndex(
+        smp_ify("post-caret text"),
+        smp_ify("supercalifragilistic"),
+        true
+      );
+      assert.equal(result2, smp_ify("post-caret text").length-1);
+    })
+  })
+});
+
 describe("Transcriptions and Transforms", function() {
-  var toSupplementaryPairString = function(code){
-    var H = Math.floor((code - 0x10000) / 0x400) + 0xD800;
-    var L = (code - 0x10000) % 0x400 + 0xDC00;
-
-    return String.fromCharCode(H, L);
-  }
-
-  // Built in-line via function.  Looks functionally equivalent to "apple", but with SMP characters.
-  let u = toSupplementaryPairString;
+  // Built in-line via function.  Looks functionally equivalent to "apple", but with non-BMP characters.
   let smpApple = u(0x1d5ba)+u(0x1d5c9)+u(0x1d5c9)+u(0x1d5c5)+u(0x1d5be);
 
   it("does not store an alias for related OutputTargets", function() {
@@ -124,7 +263,7 @@ but not himself.`;  // Sheev Palpatine, in the Star Wars prequels.
       assert.equal(transcription.transform.deleteRight, 1, "Incorrect count for right-of-caret deletions");
     });
 
-    it("handles deletions around the caret without text insertion (SMP text)", function() {
+    it("handles deletions around the caret without text insertion (non-BMP text)", function() {
       try {
         String.kmwEnableSupplementaryPlane(true);
         var target = new Mock(smpApple, 2);
@@ -216,7 +355,7 @@ but not himself.`;  // Sheev Palpatine, in the Star Wars prequels.
       assert.equal(transcription.transform.deleteRight, 3, "Incorrect count for right-of-caret deletions");
     });
 
-    it("handles deletions around the caret with text insertion (SMP text)", function() {
+    it("handles deletions around the caret with text insertion (non-BMP text)", function() {
       try {
         String.kmwEnableSupplementaryPlane(true);
 
@@ -297,6 +436,35 @@ but not himself.`;  // Sheev Palpatine, in the Star Wars prequels.
         String.kmwEnableSupplementaryPlane(false);
       }
     });
+
+    it('from targets with existing selection', () => {
+      //                              |            |
+      const target = new Mock("testing testing one two three");
+      target.setSelection(8, 20)
+      const original = Mock.from(target);
+      target.clearSelection();
+
+      const transform = target.buildTransformFrom(original);
+      assert.deepEqual(transform, {
+        insert: '',
+        deleteLeft: 0,
+        deleteRight: 0
+      });
+    });
+
+    it('to targets with existing selection', () => {
+      //                              |            |
+      const target = new Mock("testing testing one two three");
+      target.setSelection(8, 20)
+      const transform = {
+        insert: '',
+        deleteLeft: 0,
+        deleteRight: 0
+      };
+
+      target.apply(transform);
+      assert.equal(target.getText(), 'testing two three');
+    });
   });
 
   /*describe("Operations with deadkeys", function() {

common/web/utils/src/index.ts

@@ -21,6 +21,8 @@ export { default as extendString } from "./kmwstring.js";
 export { default as ManagedPromise } from "./managedPromise.js";
 export { default as TimeoutPromise, timedPromise } from "./timeoutPromise.js";
 
+export { Uni_IsSurrogate1, Uni_IsSurrogate2 } from "./surrogates.js";
+
 // // Uncomment the following line and run the bundled output to verify successful
 // // esbuild bundling of this submodule:
 // console.log(Version.CURRENT.toString());

common/web/utils/src/surrogates.ts

@@ -0,0 +1,27 @@
+/*
+ * The definitions below are duplicated from common/web/types/util/util.ts;
+ * we can't downcompile the originals to ES5 when bundling with esbuild.
+ * `import type` stuff is fine, but not non-type `import` statements.
+ *
+ * TODO:  Use those instead, once we're no longer building ES5 versions of Web.
+ */
+
+export const Uni_LEAD_SURROGATE_START = 0xD800;
+export const Uni_LEAD_SURROGATE_END = 0xDBFF;
+export const Uni_TRAIL_SURROGATE_START = 0xDC00;
+export const Uni_TRAIL_SURROGATE_END = 0xDFFF;
+
+/**
+ * @brief True if a lead surrogate
+ * \def Uni_IsSurrogate1
+ */
+export function Uni_IsSurrogate1(ch : number) {
+  return ((ch) >= Uni_LEAD_SURROGATE_START && (ch) <= Uni_LEAD_SURROGATE_END);
+}
+/**
+ * @brief True if a trail surrogate
+ * \def Uni_IsSurrogate2
+ */
+export function Uni_IsSurrogate2(ch : number) {
+  return ((ch) >= Uni_TRAIL_SURROGATE_START && (ch) <= Uni_TRAIL_SURROGATE_END);
+}

web/src/app/browser/src/keymanEngine.ts

@@ -205,9 +205,6 @@ export default class KeymanEngine extends KeymanEngineBase<BrowserConfiguration,
     // Automatically performs related handler setup & maintains references
     // needed for related cleanup / shutdown.
     this.pageIntegration = new PageIntegrationHandlers(window, this);
-
-    // Initialize supplementary plane string extensions
-    String.kmwEnableSupplementaryPlane(true);
     this.config.finalizeInit();
 
     if(this.ui) {

web/src/engine/main/src/keymanEngine.ts

@@ -201,6 +201,9 @@ export default class KeymanEngine<
 
     config.initialize(optionSpec);
 
+    // Initialize supplementary plane string extensions
+    String.kmwEnableSupplementaryPlane(true);
+
     // Since we're not sandboxing keyboard loads yet, we just use `window` as the jsGlobal object.
     // All components initialized below require a properly-configured `config.paths` or similar.
     const keyboardLoader = new KeyboardLoader(this.interface, config.applyCacheBusting);