fix(web): proper linkage of sources to events 🪠 #10960
Conversation
keymanapp-test-bot
bot
added
has-user-test
user-test-required
User Test ResultsTest specification and instructions
Retesting TemplateTest Artifacts
|
| /* | ||
| Using the Promise map built in touchStart, we can retrieve a Promise for the source linked | ||
| to this event and closure-capture it for the closure queued below. | ||
| */ | ||
| const capturedSourcePromises = new Map<number, Promise<GestureSource<ItemType, StateToken>>>(); | ||
| for(let i = 0; i < event.touches.length; i++) { | ||
| const touchId = event.touches.item(i).identifier; | ||
| capturedSourcePromises.set(touchId, this.pendingSourcePromises.get(touchId).corePromise); | ||
| } |
There was a problem hiding this comment.
The big difference from #10843 is that we can closure-capture the Promises at the original time of the event, unlike the GestureSource instances. #10843 tried to make the mapping work asynchronously, but there were cases that arose in which the former, more loosely-bound style simply couldn't work consistently.
That was likely because the mapping could see an entry erased by a different GestureSource (conceptually) that was reusing the same identifier.
Test Results
|
keymanapp-test-bot
bot
added
user-test-failed
and removed
user-test-required
There was a problem hiding this comment.
I understand why you are making these changes and in principle they seem fine. This is a significant change to the structure of the gesture engine for beta, however, so we will definitely need to make sure we do extra testing for stability and unforeseen edge cases.
The only user-visible impact (apart from fixing instability, which is obviously good) seems to be the introduction of a delay? That seems less than ideal -- can you describe the impact?
| The easiest way to ensure the exact order involves programmatic delay of their | ||
| processing, essentially "sequentializing" the events into a deterministic order. |
There was a problem hiding this comment.
Is this delay visible to the end user?
There was a problem hiding this comment.
It should be imperceptible; it's a matter of queuing tasks against the microtask queue, the macrotask queue, and the custom queue from #10843. Each of those should take negligible amounts of time.
The only delay added in this PR (at present) is due to the need to await a few pre-resolved Promises... so a couple of microtask delays per queued event should be the only extra wait here. (They're not resolved when closure-captured, but they should already be resolved at every point they're being await-ed.)
There was a problem hiding this comment.
If desired, I probably could 'rig' a way to avoid using Promises and the microtask queue for this, which would remove even the microtask queue waits. That might actually make the code that little bit more difficult to parse for maintenance, though - I feel that Promise semantics work in our favor here for code clarity.
A simple object (providing a closure-capturable root reference) with a mutatable field would be enough. Said field would start off undefined but be assigned the source's reference when it's available (during the final onTouchStart closure). Of course, we'd have to explicitly document and maintain that connection, which is why I feel it a bit less "simple."
There was a problem hiding this comment.
My preference is almost always for synchronous code where possible -- re-entrant code makes code flow more complicated with potential for other interactions (from other promises and events) which all goes away without it. But I'd need to compare the two solutions here to be sure.
I think most of the wrinkles we've had in the new gestures have been async-related?
keymanapp-test-bot
bot
added
the
user-test-required
|
I didn't see anything about an Android error this time, which is a plus. Though... that might be due to the more common error that surfaced. It'd surface quickly, so if the Android error never super-consistent to begin with, it may have just "not happened (yet)". Turns out that I made a fairly simple mistake in one of the event handlers - I forgot that ending touches don't show up in their event's tl;dr: The error I found when I went looking quite naturally made the last key touched act "sticky", among other things. I've fixed it and made it easier for unit tests to catch the same mistake in the future. @keymanapp-test-bot retest all |
Test Results
..Android 5.0 / API 21 emulator 
..Android 7.1.1 / API 25 emulator 
..Android 9.0 / API 28 emulator 
..Android 14.0 / API 34 emulator 
..Android 13.0 / Samsung Galaxy A23 Mobile device 
|
|
Sentry Issue: KEYMAN-WEB-HY |
keymanapp-test-bot
bot
added
user-test-failed
and removed
user-test-required
|
I've been noticing a few gesture things along the lines of the failed test recently too on my personal iPhone... but hadn't realized it was happening that extensively. Interestingly, I'm not seeing some of the issues in iOS Simulator. Did some digging via TestFlight and found that 17.0.261-beta seems to be comparatively fine, with a number of breaks happening in 17.0.262-beta. ... and after a bit of investigation, for iOS, the stuff I noticed is due to context issues that resolve with #10956. That doesn't exactly address the Android bits noted here, though. (17.0.262 did involve some change to iOS's context handling, so it does track somewhat.) Context resets generally involve layer resets, after all, so gestures involving layer manipulation could definitely be affected by context issues. |
…x/web/queued-closure-source-linking
|
@keymanapp-test-bot retest TEST_GESTURE_REGRESSIONS I've gone ahead and merged in recent For any tests that fail, please describe the nature of the test failures for each. Please make it explicitly clear if the reason for failure is that things are happening correctly, but too slowly. "Certain calculations" - we rely on key layout information from the DOM when noting the 'item' for each ongoing touchpath. To get this for layers currently not visible, we make them visible temporarily, which triggers layout reflows - and that appears to be a major contributing factor to lagginess on the local test device.
I see the video, but I'm not clear on the reason this was labeled as a failure. Is it due to each tap taking a while to process, rather than executing quickly? The individual |
keymanapp-test-bot
bot
added
user-test-required
Hmm, could we cache this information when the keyboard is first loaded? (Perhaps being smart enough to think about rotation as well?) This may not be a 17.0-beta change but I'm all for avoiding layout reflows during processing! |
|
In regard to that last point...
Drilling down the other high-percentage contributors reveals similar figures - a lot of time was spent on layout-reflow. I may want to spend some time seeing if that can be optimized. Interestingly, I don't see anything even close to this level of lagginess within the iOS app/keyboard; maybe Safari webviews are caching layout calculations in a manner that Chrome webviews aren't? |
Sounds good to me! Let's make sure we get the other beta PRs merged before you get too deep into this one though.
Potentially -- but also iPhones are much faster hardware so it may just be much less visible. |
I could see caching each layer's layout info as it's loaded. That would be well within reason. (Then dumping the cache on a rotation or keyboard size-shift.) |
Agreed. If this is the only real source of problems at present, that can certainly be done separately, allowing this chain to merge. |
|
Just wondering: can we not calculate the regions without doing layout at all? We have all the numbers in the touch layout data and we have the bounding box size? |
At the very least, we can approximate it with that data - and we might even be able to perfectly match it. Sounds like a wise optimization path to investigate. |
This is 6 PRs in on the 🪠 chain. This is the one that pushes things over on the filesize warning, I guess. |
Test Results
ModiPressHold.mp4 |
keymanapp-test-bot
bot
added
user-test-failed
and removed
user-test-required
|
The remaining issues seem to mostly be performance-related. There is one notable, though ambiguous, functionality point of failure... but this does resolve a lot of far, far worse behaviors and will let us get |
|
Changes in this pull request will be available for download in Keyman version 17.0.301-beta |
Continues from #10843.
Despite my efforts to avoid it, it appears necessary to retool the event engines to ensure a proper linkage between original event identifier and the corresponding
GestureSourcethat will be generated and updated. The previous "identifier" to "source" mapping style appears to be too "loose" due to asynchronicity - it's time to update to a strategy that can directly use closure-capture mechanics to ensure proper binding.Extra details: By default, browsers will reuse touch identifiers after they are freed - identifiers aren't unique. If this replacement happens while related events are deferred, we get problems like what can be seen in #10843's user test reports. Thus, it's necessary to establish the link between identifier and source while the identifier is still current for the touchpath represented by the source, then "lock in" that link - something that closure-capturing is well-suited for.
As the actual construction is queued for later, the best way I can see to leverage closure-capture mechanisms to resolve the issue is to creating Promises for the
GestureSourcesynchronously during the actual touchStart handler - not within its closures. Resolution of thePromises can occur within closures, but assignments leveraging thePromiseneed to be maintained synchronously, as it can then be accessed synchronously within the other events for closure-capture in their handlers. (After all, under extreme rapid-typing situations, it's quite possible thePromisewill only be fulfilled long after the fact, but this Promise will always be available, unambiguously, in time with the original events.)User Testing
TEST_RAPID_TYPING_STABILITY: Using the Keyman for Android app, attempt to reproduce #10592 and #10646.
typing rapidly should not skip keys; just make sure it's something you feel comfortable typing rapidly.TEST_GESTURE_REGRESSIONS: Run the full set of Web's gesture-related regression tests and report back any errors.
TEST_RAPID_TYPING_STABILITYto that set of tests.