keyman
keyman copied to clipboard
keymanapp
change(web): input-event sequentialization ðŸª
Fixes #10592. Fixes #10646.
Continues from #10840.
This change introduces a "sequentialization" queue for all gesture-engine touch input, ensuring that all incoming touch-based events are processed in the proper sequence despite the engine's internally-asynchronous structure. In particular, it helps to ensure that:
- gesture model-matching will update after every relevant event before the next input-event is dequeued
- a gesture source's events will be delayed until all possible models for matching have had instances initialized that are able to receive and utilize update events
As a result, the internals of the gesture engine now work with three layers of task queues:
- The microtask queue - is used by gesture model-matching and gesture-selection processes.
- The macrotask queue - is used during gesture-match initialization of new sources (within
MatcherSelector) and for gesture-source updates once initialized. - Two Promise-based "lock" queues, working together:
- An implicit, private one within
MatcherSelector.matchGesture, ensuring that gesture-match initialization acts atomically with regard to incoming gesture sources - One within
TouchInputEngine(and unlatched withinTouchpointCoordinator) ensuring that all gesture-source updates are paused whilematchGesture's "lock" is engaged.
- An implicit, private one within
There's a shot we can eliminate the "implicit" one (from #10840) and rely solely on the one from this PR, but #10840's is much simpler and likely helps add robustness, so I'd rather keep it around. If naught else, though, #10840 made a great stepping stone upon which to build further with this sequentialization queue.
User Testing
TEST_RAPID_TYPING_STABILITY: Using the Keyman for Android app, attempt to reproduce #10592 and #10646.
- Set "EuroLatin (SIL)" as the active keyboard.
- Rapidly type 10 random keys. Verify that 10 characters are emitted, possibly after a brief wait.
- Don't worry about accuracy - just try to type 10 characters as fast as humanly possible, whatever they are.
- Keep in mind that a space (
- Rapidly type while utilizing functions like the globe key, space bar and predictive texts intermittently. (Refer to https://github.com/keymanapp/keyman/pull/10641#issuecomment-1935882421 if needed.)
- Try to rapidly type something specific while being precise.
- Perhaps
typing rapidly should not skip keys; just make sure it's something you feel comfortable typing rapidly. - If any of the letters appear out-of-order or appear to be skipped entirely, FAIL this test.
- Do not fail this test if you suspect you simply fat-fingered a key, hitting a close neighbor instead of the intended key.
- Perhaps
- FAIL this test if any of the following happens:
- Stuck key-highlighting
- Stuck key-previews
- Stuck subkey menu
- Stuck modipress
User Test Results
Test specification and instructions
- 🟥 TEST_RAPID_TYPING_STABILITY (FAILED): Retested with the attached PR build (17.0.271-beta-test-10843) on an Android 14 / API 34 emulator and here is my observation: 1. Able to reproduce the keyboard error message during rapidly typing using the OSK. 2. Also, noticed that after continuing the same process (rapidly typing, long pressing keys) along with the globe key, the enter key, num key, and Shift key leads to closing the keyman application. 3. Reopened the Keyman-Inapp. Noticed that the keys stuck-key highlighted and the long press menu appeared on the screen. (notes)
Retesting Template
@keymanapp-test-bot retest TEST_RAPID_TYPING_STABILITY
Test Artifacts
- Android
- Developer
-
iOS
- Keyman for iOS (simulator image)
- FirstVoices Keyboards for iOS (simulator image)
-
TestFlight internal PR build version -
17.0.296 (0.10843.10628)
- Keyboards
- Web
- Windows
![keymanapp-test-bot[bot] avatar](https://avatars.githubusercontent.com/in/133554?v=4 "Published by a pub.dev verified publisher"){kind=small}
Test Results
- TEST_RAPID_TYPING_STABILITY (FAILED): Tested with the attached PR build (Keyman 17.0.271-beta-test-10843) on an Android (ver 13) Mobile device and here is my observation: 1. Set "EuroLatin (SIL)" as the active keyboard. 2. Rapidly typing (more than) 10 characters as fast as possible (including space bar) leads to an error message (easily reproducible). 3. To reproduce #10646, set Keyman as the system-wide keyboard. 4. Opened Chrome browser. 5. Switched to Keyman keyboard. 6. Started rapidly typing using the Keyman keyboard. Clicked the globe key. It switched to the default English (Gboard) keyboard. 7. Again switching to Keyman keyboard leads to highlighting the spacebar key and other keys. But this issue seems to be a random one since it is not reproducible easily. I have attached the Video file (from the mobile) as well as the Screenshot (from the emulator) for reference.
..keyboard error
https://github.com/keymanapp/keyman/assets/19683143/87c2eccf-9594-49f5-84a9-84f4b65f2373
..stuck key highlighting
- The new queue really needs unit tests.
That's an entirely fair point. I'll get on that.
it buries async behaviour inside its triggerEvent function so that you cannot chain on any promises contained therein.
This is fully engine-internal and is not exposed as part of the API; the only chaining that should happen is handled within the queue itself.
I have a lot of comments about naming and purpose -- sorry about that.
No, don't be - I could tell the nomenclature needed workshopping. Didn't want that to get in the way of implementing the actual fix, though.
There are places where exceptions are masked -- but why are we not addressing the exceptions themselves?
Most of the newly-placed locations with try-catch statements exist to handle exceptions from outside of the gesture-recognizer. If KeymanWeb makes a mistake when handling a gesture produced by the gesture-engine, that shouldn't break the gesture-engine's processing. Separate module.
Also, console.warn and console.error are our current Sentry logging statements for Web.
Ideally, I'd like to add one more unit test - one with things being added to the closure queue while it's actively running. The issue is that such a unit test looks fairly tricky to write, especially in a clear and maintainable way.
I'm not too worried about it, though - with a little glass-box testing (via use of its new .ready property), the logic for such a case is already covered. The extra unit test would make for a nice black-box test of that logic, though.
@keymanapp-test-bot retest TEST_RAPID_TYPING_STABILITY
Marc caught a notable bug I made when relocating certain critical changes from their original version; that bug could have very easily caused the test failure noted above.
Test Results
- TEST_RAPID_TYPING_STABILITY (FAILED): Retested with the attached PR build (Keyman 17.0.271-beta-test-10843) on an Android 13 mobile device and here is my observation: 1. Set "EuroLatin (SIL)" as the active keyboard. 2. Rapidly typing (more than) 10 characters as fast as possible leads to an error message (easily reproducible). (#10592) 3. Not able to reproduce the issue #10646. 4. Followed the steps from 1 to 4 (from this PR 10843) and noticed that a keyboard error message appeared while rapidly typing using the OSK. 5. I already posted my result on 26th Feb as "Failed". Waiting for an updated build. 6. On 28th Feb, the same PR was available for retesting in the User Test section. 7. Retested with the attached PR build and still seeing the same error message. Seems to be an issue. As per Josh's suggestion, I failed this test. Thanks.
Oh good, we got Sentry events for the user-test failure.
- https://keyman.sentry.io/share/issue/d7da366830a343d8a1852bf42405b783/
- https://keyman.sentry.io/share/issue/42b63ce83955443e86914d67c6438b77/
... and it's something that I remember seeing on occasion during development of the fix. Thought I'd fixed it... maybe I missed another important line or two when I was cleaning things up?
@keymanapp-test-bot retest TEST_RAPID_TYPING_STABILITY
OK, I think I found out what was causing that error, and I believe I've fixed it up. Sadly, I'm still not able to deliberately reproduce it, so I can't confirm this without a retest.
My suspicion was that it might be easier to repro when throwing in up-flicks during the rapid typing session in order to trigger early longpress -> subkey use. Alas, I didn't even get one trigger after trying at least 5 different rounds of at least 5 second bursts of rapid typing (with the same commit as failed the user test).
The good news: the longpress triggers I attempted during the rapid-typing sessions did fully complete, even outputting appropriate subkeys. For good measure, I even just threw in some modipress use, and even that seems to work fine.
@jahorton can you link those two sentry issues to this PR if you think they'll be fixed by this?
![sentry-io[bot] avatar](https://avatars.githubusercontent.com/in/12637?v=4 "Published by a pub.dev verified publisher"){kind=small}
Already put a link to it here, but not as a "formal" link.
The cross-linking "formal" link helps when triaging crash reports from the Sentry side.
Test Results
- TEST_RAPID_TYPING_STABILITY (FAILED: Retested with the attached PR build (17.0.271-beta-test-10843) on an Android 14 / API 34 emulator and here is my observation: 1. Able to reproduce the keyboard error message during rapidly typing using the OSK. 2. Also, noticed that after continuing the same process (rapidly typing, long pressing keys) along with the globe key, the enter key, num key, and Shift key leads to closing the keyman application. 3. Reopened the Keyman-Inapp. Noticed that the keys stuck-key highlighted and the long press menu appeared on the screen.
..Keyboard error message
..Stuck-key highlighted
Ok... so that same Sentry-reported error is still happening, even after I thought I fixed it. Must have missed an angle.
There was something else I tried at one point that I back-tracked since it could make things seem more complex... looks like it's time to revisit that idea.
At this point, it looks like the best way forward... is to add a new child PR due to the scope of the prospective changes. See #10960 for the next step. I've specified the same user test on it.
