fix(mac): improved adherence to backspace rules for compliant apps#15561
fix(mac): improved adherence to backspace rules for compliant apps#15561
Conversation
introduces an alternative to approach to backspace for compliant apps by using the insertText API to replace a character to be deleted and the preceding character from the context with only the character from the context Fixes: #15543
User Test ResultsTest specification and instructions
Test Artifacts |
Test Specs
Test Results
|
mcdurdin
left a comment
There was a problem hiding this comment.
This looks really good -- it's a clear and easy to follow function. I think we need to cover off the surrogate pair scenario for completeness.
I have tried to write a background description in the function header comment because the 'why' for this I think will be unclear to future maintainers. Feel free to adapt especially if I have gotten any details wrong.
| if ((self.apiCompliance.canReplaceText) && ([context length] > codePointsToDelete)) { | ||
| int codePointsToReplace = codePointsToDelete + 1; | ||
| NSRange replacementStringRange = NSMakeRange([context length] - codePointsToReplace, 1); | ||
| NSString *replacementString = [context substringWithRange:replacementStringRange]; | ||
|
|
||
| // replace only works for non-control characters | ||
| // if replacementString contains control characters, then return without handling event | ||
| NSString *message = nil; | ||
| if ([self containsControlCharacter:replacementString]) { | ||
| message = @"handleDeleteByReplace, replacementString contains control characters, cannot delete with replace"; | ||
| os_log_debug([KMLogs keyTraceLog], "%@", message); | ||
| [KMSentryHelper addDebugBreadCrumb:@"event" message:message]; | ||
| return NO; | ||
| } else { | ||
| message = @"handleDeleteByReplace, canReplaceText == true"; | ||
| os_log_debug([KMLogs keyTraceLog], "%@", message); | ||
| [KMSentryHelper addDebugBreadCrumb:@"event" message:message]; | ||
| handledEvent = YES; | ||
| } | ||
|
|
||
| NSRange replacementRange = NSMakeRange(replacementStringRange.location, codePointsToReplace); | ||
| [client insertText:replacementString replacementRange:replacementRange]; | ||
| } |
There was a problem hiding this comment.
Sorry to make your life more difficult here, but I think we need to make sure this works even with non-BMP characters (U+10000 - U+10FFFF) because it's not clear to me that splitting a surrogate pair in the replacement will work consistently -- even though it's effectively not changing, some apps may reject the transform due to encoding boundaries or safety concerns.
There was a problem hiding this comment.
That was a worthwhile test! I tried with the Malar Tirhuta keyboard, and after typing 𑒏𑒹 (U+1148F U+114B9) and hitting backspace, it only deleted the second half of the surrogate pair, leaving me with U+1148F U+FFFD
I could check the character that core tells me to delete and behave differently if it is a surrogate pair: either resort to the passthrough/generate backspace or, probably riskier, look for a match between the delete string returned from core and the context and try to replace more of the string.
I tried finding APIs that know more about the Unicode string without checking for specific character ranges, but what I found didn't work. There may be better support with the Swift Unicode APIs, so we may have better options when we move the Input Method to Swift in Keyman 20+.
There was a problem hiding this comment.
It looks like the key is to examine the character that precedes the substring that core instructs us to delete and see whether it is part of a 'composed character'. In the macOS APIs, both surrogate pairs and characters with combining diacritics or vowels are considered to be composed characters.
If we try to delete by letting the backspace event pass through, then it seems the system will delete an entire composed character. If we delete using the character index, then we may delete a partial character and cut something in half.
So when we delete by replacing, we need to check the previous character, and, if it is a composed character, use the whole thing for the replacement. This will prevent breaking up surrogates pairs.
Keyman Core may instruct us to delete something that is part of what macOS considers to be the same composed character. In that case, we can see if the code unit that core instructs us to delete and the code unit preceding it in the context are both part of the same composed character. We can do this by calling the following API for each index in the string: rangeOfComposedCharacterSequenceAtIndex
An example of this scenario would be the test case TEST_BKSP_D_ACUTE
If this is the right approach, I am still not sure whether there is a way to distinguish the difference between deleting the diacritic for that test case and the combining vowel (which is a surrogate pair on its own) in the Malar Tirhuta case.
There was a problem hiding this comment.
I think we should treat surrogate pairs differently to clusters. As far as I can tell, a compliant app needs only the surrogate pair handling for correct deletion via the insertText API, and so we should do just that there.
Non-compliant apps are a whole separate can of worms. In non-compliant apps, cluster awareness seems like it would be a really good way to approach a more robust backspacing model, but we do run the risk of being too clever for some apps which 'naively' only delete a single codepoint on backspace. This should be tracked as a new issue (is it related to #10246?)
Co-authored-by: Marc Durdin <marc@durdin.net>
Co-authored-by: Marc Durdin <marc@durdin.net>
deals with preceding character being a surrogate pair when a delete occurs Fixes: #15543
Test Specs
Test Prerequisites
Test Results
|
Introduces an alternative to approach to backspace for compliant apps by using the insertText API to
replace a character to be deleted and the preceding character from the context with only the character from the context
Fixes: #15543
Build-bot: release:mac
User Testing
Preparation: install the keyboard bksp_ldml.zip
All tests should be using compliant apps such as Apple's Pages, TextEdit or Stickies.
abc d/
Select the text, copy and paste it into a character viewer and verify that the last two characters are
U+0064 U+0301.abc d/
Then press bksp. The result should be
abc d.caf'e
The result should be
café.Then press bksp. The result should be
caf.abcreturnab
Then press bksp four times. The result should be
ab.