change(web): reworks nearest-key detection to avoid layout reflow 🪠 #11129
Conversation
keymanapp-test-bot
bot
added
has-user-test
user-test-required
User Test ResultsTest specification and instructions
Retesting TemplateTest Artifacts
|
|
A couple of profiles with these changes in place, on the same device (SM-T350 / Galaxy Tab A, released May 2015):
This run did include some actual layer changes, and it's those which seemed to have the largest run-time impact. (With this test device, even triggering a layer change in isolation has perceptible lag.)
The second-highest run-time contribution came from gesture processing itself. In order of their listing:
There certainly is more overhead from ensuring proper event-handling order than we can consider negligible, but even then, it's only a contributing factor. For comparison, here's a row with only a single layer-shift (due to start-of-sentence) and a similar number of keystrokes (~20). Same device.
Any layer-shifting operations will have a greater performance hit than the remainder of the code. |
|
Diving even deeper in, it's worth note that every keystroke does manipulate the DOM a bit, especially when predictive text is active. Each suggestion has corresponding DOM elements that get maintained. Also, here's an extreme close-up of a single layer-change operation:
7120 ms - 6940 ms = 180ms - almost 0.2 seconds. This is definitely enough to be perceptible.
Waaaait a second. The keyboard's size dimensions shouldn't be affected here, so maybe we could Other notable layout aspects:
|
Test Results
I have attached a video file (test_flick_during_modipress) represents there is a lack of motion in the animation and it does not show the expected output Úq on the text input screen. modipressmultitap.mp4 |
keymanapp-test-bot
bot
added
user-test-failed
and removed
user-test-required
|
From that test report... what I glean is that the flick-locking mechanism appears to be interfering with the test. There's a chance we may need some improvements to flick-resetting. The animations look smooth to me until they stop for a while, then a "jump" occurs to a different flick - which is indicative of this. |
|
Per discussion with @mcdurdin, that user-test failure seems acceptable, noting issue #11139 for further tracking for potential improvement. The aspects being addressed by this PR are working as intended, with the noted issue being largely unrelated to these changes. That said, I can't actually merge this PR yet; it hasn't been reviewed. |
| if (minDistance < Number.MAX_VALUE) { | ||
| let fudgeFactor = closestKey.spec.proportionalWidth * NEAREST_KEY_HORIZ_FUDGE_FACTOR; | ||
| fudgeFactor = fudgeFactor > minHorizFudge ? fudgeFactor : minHorizFudge; | ||
|
|
||
| if (((x1 - x) >= 0 && (x1 - x) < dxMax) || ((x - x2) >= 0 && (x - x2) < dxMax)) { | ||
| return <KeyElement>t.firstChild; | ||
| if(minDistance <= fudgeFactor) { | ||
| return closestKey.btn; | ||
| } | ||
| } |
There was a problem hiding this comment.
Shouldn't the acceptable area be a fixed value, not relative to the size of the key? That is, pressing on the edge of spacebar shouldn't have a different outcome to pressing the edge of the Z key in terms of how close the finger has to be.
There was a problem hiding this comment.
I'm just working with "what was" here. What you're saying certainly makes sense, though.
Given most of our keyboards tend to have 10 keys in their top row, using its full width... that implies a default total key width + padding of 10% computedWidth. (Thus, 0.1 "proportional width + padding"). Would a 6% * computedWidth fudge factor be reasonable?
There was a problem hiding this comment.
yep, sounds like we came to the same conclusion 🤣 in my previous comment #11129 (comment)
So that's a good sign!
| const distance = distanceFromCenter - keyRadius; | ||
| if(distance < minDistance) { | ||
| minDistance = distance; | ||
| closestKey = key; | ||
| } |
There was a problem hiding this comment.
Once the distance starts to increase again, we can break out of the loop, right?
|
Changes in this pull request will be available for download in Keyman version 17.0.301-beta |
Addresses the performance concerns noted at #10960 (comment).
This changes the Web engine's 'nearest key' detection method to run entirely based upon precomputed layout values, completely sidestepping any need to trigger any (costly) layout reflow operations. This significantly improves keyboard-responsiveness in rapid-typing / "key-mash" scenarios.
User Testing
TEST_GESTURE_REGRESSIONS: Run the full set of Web's gesture-related regression tests and report back on any errors. Please give details for any error encountered.