fix(mac): improved adherence to backspace rules for compliant apps

mac/Keyman4MacIM/Keyman4MacIM/KMInputMethodEventHandler.m

@@ -8,6 +8,7 @@
 #import "KMInputMethodEventHandler.h"
 #import <KeymanEngine4Mac/KeymanEngine4Mac.h>
 #import <Carbon/Carbon.h> /* For kVK_ constants. */
+#import <CoreFoundation/CoreFoundation.h>
 #import "KeySender.h"
 #import "TextApiCompliance.h"
 #import "KMSettingsRepository.h"
@@ -370,7 +371,7 @@ -(NSString*)readContext:(NSEvent *)event forClient:(id) client {
           contextString = attributedString.string;
 
           //only uncomment for testing as we do not want to write context in logs
-          //os_log_debug([KMLogs testLog], " length: %lu result: %{public}@", contextString.length, contextString);
+          //os_log_debug([KMLogs keyTraceLog], " length: %lu result: %{public}@", contextString.length, contextString);
         }
       }
     }
@@ -405,18 +406,7 @@ -(BOOL)applyKeyOutputToTextInputClient:(CoreKeyOutput*)output keyDownEvent:(nonn
     [KMSentryHelper addDebugBreadCrumb:@"event" message:message];
     [self insertAndReplaceTextForOutput:output client:client];
   } else if (output.isDeleteOnlyScenario) {
-    if ((event.keyCode == kVK_Delete) && output.codePointsToDeleteBeforeInsert == 1) {
-      // let the delete pass through in the original event rather than sending a new delete
-      NSString *message = @"applyOutputToTextInputClient, delete only scenario with passthrough";
-      os_log_debug([KMLogs keyTraceLog], "%@", message);
-      [KMSentryHelper addDebugBreadCrumb:@"event" message:message];
-      handledEvent = NO;
-    } else {
-      NSString *message = @"applyOutputToTextInputClient, delete only scenario";
-      os_log_debug([KMLogs keyTraceLog], "%@", message);
-      [KMSentryHelper addDebugBreadCrumb:@"event" message:message];
-      [self sendEvents:event forOutput:output];
-    }
+    handledEvent = [self handleDeleteOnlyScenario:output keyDownEvent:event client:client];
   } else if (output.isDeleteAndInsertScenario) {
     // TODO: fix issue #10246
     /*
@@ -511,6 +501,204 @@ -(void)insertAndReplaceText:(NSString *)text deleteCount:(int) replacementCount
   }
 }
 
+/**
+ * Handles deleting without an associated insert in one of three methods:
+ * 1. delete via replace: do the delete by replacing two (or more) characters with one.
+ * 2. backspace passthrough : if the original keydown event was a backspace, pass it through unhandled
+ * 3. generate event: generate keydown backspace events as necessary
+ */
+-(BOOL)handleDeleteOnlyScenario:(CoreKeyOutput*)output keyDownEvent:(nonnull NSEvent *)event client:(id) client {
+
+  // attempt to delete by replacing -- for compliant apps only
+  if ([self handleDeleteWithReplacement:output keyDownEvent:event client:client]) {
+    return YES;
+  }
+
+  // pass through if this was a backspace keydown event
+  if (event.keyCode == kVK_Delete && output.codePointsToDeleteBeforeInsert == 1) {
+    // let the delete pass through in the original event rather than sending a new delete
+    NSString *message = @"handleDeleteOnlyForOutput, delete only scenario with passthrough";
+    os_log_debug([KMLogs keyTraceLog], "%@", message);
+    [KMSentryHelper addDebugBreadCrumb:@"event" message:message];
+
+    // instruct system to handle the event
+    return NO;
+  }
+  else {
+    // otherwise generate a backspace
+    NSString *message = @"handleDeleteOnlyForOutput, send backspace event";
+    os_log_debug([KMLogs keyTraceLog], "%@", message);
+    [KMSentryHelper addDebugBreadCrumb:@"event" message:message];
+    [self sendEvents:event forOutput:output];
+
+    return YES;
+  }
+}
+
+/**
+ * For compliant apps only.
+ * 
+ * This an attempt to make sure that deletion removes only the expected codepoints.
+ * When handling a transform which only deletes a character, or when allowing a
+ * backspace to pass through, the OS or application may not use the same rules
+ * around deletion as Keyman -- especially when deleting clusters such as letter +
+ * combining diacritic (e.g. `U+0062 U+0301`), where some applications may delete
+ * both together as they represent a single 'grapheme cluster'. 
+ * 
+ * (Note, the question of whether it is appropriate for backspace to delete a
+ * cluster rather than a codepoint from an end-user perspective is not relevant
+ * here, because what is important is that we match the rules that the keyboard has
+ * provided, which means we need a method of deleting a precise number of
+ * codepoints. The keyboard author can and should include rules for cluster
+ * deletion that meet end-user expectations.)
+ * 
+ * The `insertText` API takes two parameters: a string to insert, and a range to
+ * replace with that string. However, we cannot simply pass through a zero-length
+ * insertion string along with the range to delete, because the `insertText` API
+ * treats this as an invalid call and ignores it.
+ * 
+ * Instead, we can delete the desired number of codepoints only by using the
+ * `insertText` API to replace e.g. two codepoints with one.
+ * 
+ * This method only works for compliant apps because non-compliant apps do not
+ * support the `insertText` API.
+ * 
+ * Ref: https://developer.apple.com/documentation/appkit/nstextinputclient/inserttext(_:replacementrange:)
+ */
+-(BOOL)handleDeleteWithReplacement:(CoreKeyOutput*)output keyDownEvent:(nonnull NSEvent *)event client:(id) client {
+  BOOL handledEvent = NO;
+  NSString *context = [self readContext:event forClient:client];
+
+  // guard: only for compliant apps with sufficient context
+  if (!(self.apiCompliance.canReplaceText) || ([context length] <= output.textToDelete.length)) {
+    os_log_debug([KMLogs keyTraceLog], "cannot replace text, non-compliant or insufficient context");
+    return NO; // return without deleting/replacing
+  }
+
+  // guard: the logic of this method depends on locating textToDelete in the context
+  if (![self stringToDeleteMatchesContextSuffix:output.textToDelete context:context]) {
+    os_log_debug([KMLogs keyTraceLog], "cannot replace text, textToDelete not found at end of context");
+    return NO; // return without deleting/replacing
+  }
+
+  NSUInteger deletionTargetLength = output.textToDelete.length;
+  NSUInteger deletionTargetLocation = context.length-deletionTargetLength;
+  NSUInteger precedingCharacterLocation = deletionTargetLocation - 1;
+
+  if ([self deletionWillReplacePartOfCluster: deletionTargetLocation precedingCharacterLocation:precedingCharacterLocation context:context]) {
+    handledEvent = [self deleteByReplacingWithPrecedingCharacter:precedingCharacterLocation deleteLength:deletionTargetLength context:context client:client];
+  } else {
+    handledEvent = [self deleteByReplacingWithPrecedingCluster:precedingCharacterLocation deleteLength:deletionTargetLength context:context client:client];
+  }
+
+  return handledEvent;
+}
+
+/**
+ * Check whether the string to be deleted is found at the tail end of the current context.
+ */
+-(BOOL) stringToDeleteMatchesContextSuffix:(NSString*)textToDelete context:(NSString*) context {
+  BOOL doesMatch = NO;
+
+  // get length of string to delete and compare to end of context
+  NSUInteger deleteLength = textToDelete.length;
+  NSUInteger locationOfDeletionTarget = context.length-deleteLength;
+  NSUInteger locationOfPrecedingCharacter = locationOfDeletionTarget - 1;
+  NSString *contextSuffix = [context substringFromIndex:context.length-deleteLength];
+
+  os_log_debug([KMLogs keyTraceLog], "stringToDeleteMatchesSuffix, textToDelete: '%{public}@', contextSuffix: '%{public}@', locationOfDeletionTarget: %u, locationOfprecedingCharacter: %u", textToDelete, contextSuffix, (int)locationOfDeletionTarget, (int)locationOfPrecedingCharacter);
+
+  doesMatch = [textToDelete isEqualToString:contextSuffix];
+  os_log_debug([KMLogs keyTraceLog], "stringToDeleteMatchesSuffix: %{public}@", doesMatch?@"YES":@"NO");
+  return doesMatch;
+}
+
+/**
+ * Check whether the string to be deleted is part of the same cluster as the character in the context that precedes it.
+ */
+-(BOOL) deletionWillReplacePartOfCluster: (NSUInteger)deletionLocation precedingCharacterLocation: (NSUInteger)precedingLocation context:(NSString*) context {
+  // NSString objects hold UTF-16 characters, so a single unicode composed character
+  // or grapheme cluster may occupy a range of NSString indices instead of a single character.
+  // This includes base and combining characters potentially composed of surrogate pairs.
+  NSRange firstDeletionTargetClusterRange = [context rangeOfComposedCharacterSequenceAtIndex: deletionLocation];
+
+  // get range of the preceding cluster in the context
+  NSRange precedingClusterRange = [context rangeOfComposedCharacterSequenceAtIndex: precedingLocation];
+
+  NSString *firstFullCharacterToDelete = [context substringWithRange:firstDeletionTargetClusterRange];
+  NSString *precedingFullCharacter = [context substringWithRange:precedingClusterRange];
+  os_log_debug([KMLogs keyTraceLog], "firstDeletionTargetCharacterRange: %{public}@, deletionCharacter: %{public}@, precedingCharacterRange %{public}@, precedingCharacter: %{public}@", NSStringFromRange(firstDeletionTargetClusterRange), firstFullCharacterToDelete, NSStringFromRange(precedingClusterRange), precedingFullCharacter);
+
+  // true when the first character to delete and the preceding character
+  // from the context are part of the same grapheme cluster
+  return NSEqualRanges(firstDeletionTargetClusterRange, precedingClusterRange);
+}
+
+/**
+ * Replace both the text to delete and the character preceding it solely with the character that precedes it.
+ * Returns YES if executing the replace/delete and NO otherwise.
+ */
+-(BOOL) deleteByReplacingWithPrecedingCharacter:(NSUInteger)precedingCharacterLocation deleteLength:(NSUInteger)deleteLength context:(NSString*) context client:(id) client {
+
+  os_log_debug([KMLogs keyTraceLog], "deleteByReplacingWithPrecedingCharacter, deletion target is part of the same grapheme cluster as the character that precedes it");
+  // get the preceding character
+  NSRange precedingCharacterRange = NSMakeRange(precedingCharacterLocation, 1);
+  NSString *replacementString = [context substringWithRange:precedingCharacterRange];
+
+  // guard: if preceding character is a control character, return NO
+  if ([self containsControlCharacter:replacementString]) {
+    NSString *message = @"replacementString contains control characters, cannot delete with replace";
+    os_log_debug([KMLogs keyTraceLog], "%@", message);
+    [KMSentryHelper addDebugBreadCrumb:@"event" message:message];
+    return NO;
+  }
+
+  // perform the replacement
+  NSUInteger replacementLength = [replacementString length] + deleteLength;
+  NSRange replacementRange = NSMakeRange(precedingCharacterLocation, replacementLength);
+  os_log_debug([KMLogs keyTraceLog], "replacementRange: %{public}@", NSStringFromRange(replacementRange));
+  [client insertText:replacementString replacementRange:replacementRange];
+
+  return YES;
+}
+
+/**
+ * Replace both the text to delete and the cluster preceding it solely with the cluster that precedes it.
+ * The 'cluster' may be just one character, but if contains surrogate pairs, this ensures that they stay together.
+ * Returns YES if executing the replace/delete and NO otherwise.
+ */
+  -(BOOL) deleteByReplacingWithPrecedingCluster:(NSUInteger)precedingCharacterLocation deleteLength:(NSUInteger)deleteLength context:(NSString*) context client:(id) client {
+
+    os_log_debug([KMLogs keyTraceLog], "deleteByReplacingWithPrecedingCluster, deletion target is independent of the grapheme cluster that precedes it");
+
+    // get range of the preceding cluster and the substring from the context
+    NSRange precedingClusterRange = [context rangeOfComposedCharacterSequenceAtIndex: precedingCharacterLocation];
+    NSString *replacementString = [context substringWithRange:precedingClusterRange];
+
+    // guard: if preceding cluster contains control characters, return NO
+    if ([self containsControlCharacter:replacementString]) {
+      NSString *message = @"replacementString contains control characters, cannot delete with replace";
+      os_log_debug([KMLogs keyTraceLog], "%@", message);
+      [KMSentryHelper addDebugBreadCrumb:@"event" message:message];
+      return NO;
+    }
+
+    // perform the replacement
+    NSUInteger replacementLength = [replacementString length] + deleteLength;
+    NSRange replacementRange = NSMakeRange([context length] - replacementLength, replacementLength);
+    os_log_debug([KMLogs keyTraceLog], "replacementRange: %{public}@", NSStringFromRange(replacementRange));
+    [client insertText:replacementString replacementRange:replacementRange];
+
+    return YES;
+}
+
+-(BOOL) containsControlCharacter:(NSString*)text {
+  NSCharacterSet *controlSet = [NSCharacterSet controlCharacterSet];
+  NSRange range = [text rangeOfCharacterFromSet:controlSet];
+
+  return (range.location != NSNotFound);
+}
+
 /**
  * Calculates the range where text will be inserted and replace existing text.
  * Returning {NSNotFound, NSNotFound} for range signifies to insert at current location without replacement.

mac/Keyman4MacIM/Keyman4MacIM/TextApiCompliance.h

@@ -22,6 +22,7 @@ NS_ASSUME_NONNULL_BEGIN
 -(void) checkComplianceAfterInsert:(NSString *)insertedText deleted:(NSString *)deletedText;
 -(BOOL)isComplianceUncertain;
 -(BOOL)canReadText;
+-(BOOL)canReplaceText;
 -(BOOL)mustBackspaceUsingEvents;
 @end