fix(web): prevents selection-clear for pure layer-switching multitaps #11232
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
keymanapp-test-bot
bot
added
the
user-test-missing
User Test ResultsTest specification and instructions
|
jahorton
commented
Apr 17, 2024
| // Has there been a context change at any point during the multitap? If so, we need | ||
| // to revert it. If not, we assume it's a layer-change multitap, in which case | ||
| // no such reset is needed. | ||
| if(!isEmptyTransform(transcription.transform) || !transcription.preInput.isEqual(Mock.from(outputTarget))) { |
There was a problem hiding this comment.
Why both?
transcription.transform(and in fact,transcriptionin general) does not provide any indication of deadkey changes.- While certainly "bad keyboard design", there's nothing preventing a multitap component of a layer-switch key from manipulating the context. Should that occur, the correct thing to do is still to rewind the context.
Note that it is technically possible to have altered the context since the base tap... but if the current context state matches the original one anyway, there's no need to perform a reset - we've already done the rewind in one manner or other.
We're only safe to bypass the context-rewind mechanism if there truly has been no change to the context whatsoever. Deadkey changes are context changes, and to handle them requires the some of the special handling newly added by this PR.
| return this.text == other.text | ||
| && this.selStart == other.selStart | ||
| && this.selEnd == other.selEnd | ||
| && this.deadkeys().equal(other.deadkeys()); |
There was a problem hiding this comment.
If someone goes and makes a multitap key that only does deadkey stuff, the final check is required to be able to rewind deadkey manipulations. Otherwise, we'd get a lot of excess deadkeys during such a multitap due to not erasing deadkeys from taps before the final one.
keymanapp-test-bot
bot
added
has-user-test
user-test-required
Test Results
|
|
@jahorton Tested the same in an Android 13 Mobile and it seems to be working as expected. |
Test Results
|
keymanapp-test-bot
bot
removed
the
user-test-required
|
Changes in this pull request will be available for download in Keyman version 17.0.309-beta |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #11230.
Previously, we always cleared the selection as part of resetting the context-state for multitaps. Problem is... that has unwanted cross-effects if the only thing the multitap is doing is to layer-switch.
It's possible to shorten the changes with a "naive" fix, but naive solutions don't account for potential deadkey solutions very well. I figure it's best to nip all such issues in the bud now, especially since I don't think there are related preventative warnings and/or errors for keyboard developers in Developer.
User Testing
Any Web-based touch-platform will suffice for these tests.
TEST_CAPS_WITH_SELECTION: Verify that selected text is not erased when double-tapping SHIFT to enter a keyboard's caps-lock state. (A direct repro attempt for #11230.)
TEST_MULTITAP_ROTA: Using a keyboard with multitap keys that output text, verify that the output text is properly replaced on each tap.
ye_old_ten_keyanddiacritic_rotafrom https://jahorton.github.io/ should work.