fix(ios): deletion of selected text #11179
Conversation
keymanapp-test-bot
bot
added
has-user-test
user-test-required
User Test ResultsTest specification and instructions
Retesting Template |
Test Results
|
Test Results
|
Test Results
..missing khmer_angkor dictionary in the suggestion bar 
..missing other keyman keyboard in keyboard list 
|
keymanapp-test-bot
bot
removed
the
user-test-required
Huh, that's... useful test data to have, though the observed issues are unrelated to this PR. I'll make a separate tracking issue. |
|
Whose bright idea was it at Apple to have @keymanapp-test-bot retest all I've fixed the error plaguing the second user-test now. As for the third user test, please verify that "Allow Full Access" has been toggled - that would probably affect the system keyboard issue you saw. As for the Khmer model not downloading... perhaps there was a connection hiccup? Perhaps fully uninstall and reinstall the Keyman app, to be safe. |
keymanapp-test-bot
bot
added
user-test-required
Test Results
|
Test Results
|
keymanapp-test-bot
bot
added
user-test-failed
and removed
user-test-required
Interesting that we haven't seen this one until now. There always have been odd interactions with that specific use case - see #2300 - and I can see the host-page getting reloaded / restored after locking and unlocking. It's temporarily blank for me, rather than permanently blank... but I'm also not sitting and waiting between locking & unlocking. If something were to prevent the host-page reload, that could certainly cause the issue. Of course, getting evidence of that is an entirely different matter. |
|
I've created a separate issue corresponding to the blank-keyboard test result; this PR's changes affect nothing near the reported behavior. |
|
@jahorton The behavior described in TEST_SELECTION_BACKSPACE appears to differ from the behavior for the Apple keyboard. Should a backspace be treated differently than typing a letter key if text is selected? |
| The one silver lining: Apple makes it impossible for users to select text | ||
| in a way that splits character clusters. | ||
|
|
||
| So, we can just insert something that won't combine, like a ZWNJ, and then delete it. |
There was a problem hiding this comment.
Are you saying that you are working around the fact that insertText of the empty string does not cause the selection to be replaced by doing an insert of something invisible that we can then delete? Does this only happen for backspace?
It sounds like you are detecting this backspace case (based on line 360) by saying we have an empty string to insert and the text selection is greater than zero. Is that only true for backspace?
This comment is hard to follow because the problem is identified, then the silver lining is mentioned and then the workaround. Also, it would be nice to keep the emotion or sarcasm out of the code as it can distract from the technical content -- even if the technical content is correct. The same is true for comments in issues or PRs. This is not productive for our relationships with our vendors.
There was a problem hiding this comment.
Are you saying that you are working around the fact that insertText of the empty string does not cause the selection to be replaced by doing an insert of something invisible that we can then delete?
Yes. That's exactly what it's saying.
Does this only happen for backspace?
It sounds like you are detecting this backspace case (based on line 360) by saying we have an empty string to insert and the text selection is greater than zero. Is that only true for backspace?
It's theoretically possible for other backspace-like keys to be defined, I suppose. Changes to keyboard options don't trigger insertText unless they also actually manipulate text - we don't need to worry about keystrokes that aren't intended to change the context in some manner, as Web doesn't send those through.
Suppose the following:
self.insertText(keymanWeb , /*numCharsToDelete:*/ 2, /*newText*/ "a")If text is selected that immediately follows a space, we need to preserve that space in order to properly match how many characters the internal Web engine has said to delete. (In fact... we need to preserve it regardless in order to stay synch'd with the internal engine.) We have to do the deletions before our text-insertions, too.
So... I think this needs to happen this way regardless of whether or not we're inserting text down the line - otherwise, we run the risk of desynchronizing. If temptext is the context and text is selected, as this code currently is, at the time of this comment, the first char to delete will be silenced due to erasing the selected text while not deleting the preceding p.
So... thanks for raising the point that helped me catch this.
There was a problem hiding this comment.
Also, it would be nice to keep the emotion or sarcasm out of the code as it can distract from the technical content -- even if the technical content is correct.
I have at times been guilty of this. It's not a good practice, I agree. Frustration can be expressed in Slack ;-) and we'll keep the code comments, issues, and PRs focused on technical.
So... I think this needs to happen this way regardless of whether or not we're inserting text down the line - otherwise, we run the risk of desynchronizing. If
temptextis the context andtextis selected, as this code currently is, at the time of this comment, the first char to delete will be silenced due to erasing the selected text while not deleting the precedingp.
.insertText() will delete selection if the string is not empty, right? And in that scenario I don't think it deletes the space prior to the selection. AFAICT, we only need to jump through the hoop when we are deleting selection with no insertion (generally, backspace)
| } | ||
| sendContextUpdate() | ||
| return | ||
| } else if numCharsToDelete <= 0 || deleteSelection { |
There was a problem hiding this comment.
This method was already long, and the new additions would be easier to follow and maintain if they were encapsulated in their own methods. If someone is reading this code and not concerned with the edge case, then it would be helpful for the edge case to be contained separately. Some of the comments could also be reduced if the new methods were named in a way that indicated what their purpose was.
That's because Apple does fancy handling with spaces. Type The original issue is about how we don't want that space deleted if we're replacing text - use of |
|
Before I forget, that makes things pretty darn tricky: the internal Web engine doesn't know or have Apple's default handling for that scenario. We're only safe to delete that space if we can ensure the edit is properly synchronized with the internal Web engine. At present... I'm pretty sure that would trigger a context reset, and we don't want that. Due to limitations, and how late we are in the release cycle, I think we need to break from how Apple does it - otherwise, we risk triggering context-desyncs, which are quite possibly the mobile app's nastiest class of errors. |
I checked for consistency with Apple's keyboard because I didn't have a strong intuitive feel for whether the space should be deleted when backspacing with a word selected. Just for comparison, I looked at Pages and found the same behavior. Then I looked at TextEdit and saw that the backspace does not remove the space preceeding the selected word. Should this be the same for desktop and mobile? I don't know. It seems more consistent to say, in both cases, that the key typed should be applied after deleting the selected text, meaning the backspace will remove the preceeding space. However, when typing in a text editor on a desktop platform, if I select a word and hit backspace, I don't think I want that. In that moment, I expect the backspace key to act like a delete key (which is what the key is actually called on the Mac, anyway). So I agree -- we should go with whatever is safest for now. We can map out all the scenarios in the future when we deal in depth with selected text behavior. |
keymanapp-test-bot
bot
added
the
user-test-required
|
@keymanapp-test-bot retest TEST_SELECTION_REPLACEMENT TEST_SELECTION_BACKSPACE The selection-text removal code has been changed, so it's best to double-check that the new pattern holds for all cases. A couple of new unit tests have been added, now that new potential "wrinkles" have been identified. Ideally, I should also add an extra test case that includes use of a key that deletes more than one character yet isn't a backspace - that would be a real stress-test. Worth noting: we treat the context as nil if there's an active text selection, so that key must not depend on existing context... but wait. If the context acts as nil, the key probably can't request to delete text it doesn't believe exists, so... huh. That test might not be possible at the moment. |
Test Results
|
keymanapp-test-bot
bot
removed
the
user-test-required
|
@jahorton It appears that the last test, TEST_GENERAL_USE, seems to be resolved in Keyman 17.0.307-beta-test-11179 and is functioning correctly. Could you please reassign this test? Thanks. |
Have we reported this to Apple as a bug? May be worth doing so. |
ios/engine/KMEI/KeymanEngine/Classes/Keyboard/InputViewController.swift
Outdated
Show resolved
Hide resolved
| Compare to Web: Web states an exact number of characters to delete | ||
| before the start of the currently-selected range... and it does this | ||
| completely unaware of the nuances listed above for .deleteBackward(). | ||
| Keyman keyboard rules are likewise unaware of Apple's nuances. |
There was a problem hiding this comment.
This is a bit of a side-track.
| Compare to Web: Web states an exact number of characters to delete | |
| before the start of the currently-selected range... and it does this | |
| completely unaware of the nuances listed above for .deleteBackward(). | |
| Keyman keyboard rules are likewise unaware of Apple's nuances. | |
| Our policy (#9073) on handling the backspace key when there is a | |
| text selection is to just delete the selection. We have to override | |
| the special case of space being deleted by .deleteBackward() ourselves. |
| In order to maintain proper synchronization between app context and | ||
| internal Web-engine context, we need to force selected-text deletion | ||
| to NEVER delete preceding spaces. Any attempts to adjust and include | ||
| the aforementioned nuance will need considerable design work to "get | ||
| right" due to the risk for adverse affects with Keyman keyboard rules. |
There was a problem hiding this comment.
| In order to maintain proper synchronization between app context and | |
| internal Web-engine context, we need to force selected-text deletion | |
| to NEVER delete preceding spaces. Any attempts to adjust and include | |
| the aforementioned nuance will need considerable design work to "get | |
| right" due to the risk for adverse affects with Keyman keyboard rules. |
I don't think this comment is needed. Just the technical background.
There was a problem hiding this comment.
... this is technical background, just from a different starting point. As the primary maintainer of the Web/mobile-app interface, this was the reason I implemented the solution as-is, after all - I wasn't even aware of the decision from #9073 when I made this. It just so happens that both reasons align.
ios/engine/KMEI/KeymanEngine/Classes/Keyboard/InputViewController.swift
Outdated
Show resolved
Hide resolved
| // `true` if there was selected text to be deleted | ||
| let deletedSelection = self.deleteSelection() |
There was a problem hiding this comment.
We only need to do this call to deleteSelection() if we (a) there is a selection, and (b) we have no new text to insert, right? We really don't want to jump through this hoop if we don't have to.
There was a problem hiding this comment.
That's why the guard exists at the start of deleteSelection. I placed it within deleteSelection rather than outside it.
Refer to #11179 (comment).
| if numCharsToDelete <= 0 || deletedSelection { | ||
| textDocumentProxy.insertText(newText) |
There was a problem hiding this comment.
This seems that if (a) we have no selection, and (b) no deletion, and (c) text to insert, then it won't insert the new text?
There was a problem hiding this comment.
No, it just comes later, after deleting preceding text. In the case that no such deletions are needed, this section allows us to return early before all of the delete-left handling kicks in. No edits were made near the later code block, so it doesn't show up in the default code-review code-windows.
There was a problem hiding this comment.
But... I think there is something off here; we probably shouldn't condition on the deletedSelection bit.
There was a problem hiding this comment.
Oh right. If we do have a selection, we currently act as if the context is nil and thus can't back delete, so it's probably a moot point.
There was a problem hiding this comment.
The "enforcer" for "selected text" = "empty context", at least within the internal Web engine:
keyman/common/web/keyboard-processor/src/text/kbdInterface.ts
Lines 302 to 306 in cffabd7
For a history trace, git blame points to #6272.
I have now submitted FB13734388 in regard to this. |
| // `true` if there was selected text to be deleted | ||
| let deletedSelection = self.deleteSelection() |
There was a problem hiding this comment.
That's why the guard exists at the start of deleteSelection. I placed it within deleteSelection rather than outside it.
Refer to #11179 (comment).
| @@ -333,6 +333,60 @@ open class InputViewController: UIInputViewController, KeymanWebDelegate { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func deleteSelection() -> Bool { | |||
| if let selected = textDocumentProxy.selectedText, selected.count > 0 { | |||
There was a problem hiding this comment.
This guard ensures we only try to delete selected text only if there actually is selected text - a selection exists with length > 0. If it doesn't exist, we exit and indicate that no deletion happened.
|
Changes in this pull request will be available for download in Keyman version 17.0.310-beta |
Fixes #11095.
User Testing
TEST_SELECTION_REPLACEMENT: Attempt to repro #11095.
Something beautiful- it's fine to use predictive-text suggestions to helpExpected result:
Something B, with a space betweenSomethingandB.TEST_SELECTION_BACKSPACE: Using the iOS Keyman app, verify that pressing backspace to delete a selection acts as intended - the selected text should be removed, with no further edits. Any preceding spaces should remain.
TEST_SELECTION_REPLACEMENT_MID: Replace selected text that starts in the middle of a word.
Something beautiful- it's fine to use predictive-text suggestions to helpTEST_SELECTION_BACKSPACE_MID: Delete selected text that starts in the middle of a word.
Something beautiful- it's fine to use predictive-text suggestions to helpTEST_GENERAL_USE: Check that general usage of the iOS Keyman app acts as expected.