-
-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(web): segmentation event + Promise resolution ordering 🐵 #7440
Conversation
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
this.fakeClock.restore(); | ||
}); | ||
|
||
it('synthetic sequence: start -> hold -> move -> end', async function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (new) unit test is derived from a first-pass extreme rough-draft/prototype for gesture synthesis control flow handling; its needs motivated most of the other changes in this PR.
/** | ||
* Designed to facilitate creation of 'synthetic' touchpath sample sequences for use | ||
* in unit tests. | ||
*/ | ||
export class TouchpathTurtle { | ||
export class TouchpathTurtle extends EventEmitter<EventMap> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows the class to serve as a source of synthetic touch events for non-segmentation unit tests. It'll definitely come in handy for easily-maintainable synthetic touchpath sequences for unit tests, as seen with the new unit test "synthetic sequence: start -> hold -> move -> end":
keyman/common/web/gesture-recognizer/src/test/auto/headless/trackedPath.js
Lines 357 to 366 in 51c6ece
// START: TOUCHPATH SIMULATION | |
// Move NE 3 pixels over 66ms, sampling every 33ms. | |
turtle.move(45, 3, 66, 33); | |
// Wait for 20 ms | |
turtle.wait(20, 20); | |
// Move S 10 pixels over 100ms, sampling every 16ms. | |
turtle.move(90, 10, 100, 16); | |
// END: TOUCHPATH SIMULATION |
With commentation, it's very easy to see and intuit exactly what sort of touchpath this is, and it's also very easy to modify the touchpath if needed in the future.
... right, I didn't actually double-check for browser-based unit-test breakage; I only focused on the headless-mode stuff. The failed test run for Web is nigh-certainly a "mia culpa." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do wonder if we need to be using promises like this. Can't we just check the gesture state machine on a (a) touch event, or (b) timeout?
The timeout is needed only for longpress, so a setTimeout would be run on touchstart. Any macro movements from the starting point would cancel the timeout, which then cancels the longpress, and a new timeout can be started for a potentially different longpress to cater for roaming touch.
If we abandon the idea of abstracting gestures, as nice as that may seem conceptually, I think the model becomes far, far simpler:
-
Tap -- on touch release, if the end point is on the same key as the start point (and longpress hasn't fired) that's a tap. (multitap assessment, see point 3)
-
Flick -- on touch release, assess if the simple path from start to end point (ignoring transit points!) matches a cardinal or intercardinal direction and is long enough -- for this we need only the two points in question. Now, if we decide we want to be fancier on this, we still only need a single function call on the array of captured points between touch start and touch release, which returns the direction of the flick. Speed of flick should not be considered -- I think we are going to run into usability issues if we make flicks dependent on speed. Again, a longpress timeout cancels any chance of flick.
-
Multitap -- on touch release, if the same key is tapped, and if the duration between the start of the previous tap, and the end of the second tap is within the threshold, match. (Or perhaps duration between end of previous tap and start of second tap, tbd.)
-
Longpress -- once the longpress timeout fires, we enter longpress mode, and all other gestures become irrelevant. Handle as we already do.
-
Roaming -- in initial implementation, I suggest these are only applicable if the keyboard doesn't have flicks. We may be able to tweak this in the future with distance thresholds -- not speed thresholds. Otherwise, these should work in the same way as they currently do. Once a roam has started, flicks and multitaps will not meet their starting conditions, so will naturally be ignored.
Is there any reason why this very simple model won't work? I may not have covered 100% of the conditions, but there are no abstractions needed, and by removing the abstractions, we also remove most of the complexity.
Note that visual feedback on flicks and multitaps would certainly be beneficial -- for example, see what happens on an iPad keyboard when you flick down to get digits and punctuation. This can then help give feedback to the user with roaming touch if the distance of the gesture exceeds the flick threshold, "snapping" off the flick.
This visual feedback can still be catered for in this simple model, on touchmove events.
Even should the rest be simplified... (b)'s one aspect of what the keyman/common/web/gesture-recognizer/src/tools/unit-test-resources/src/timedPromise.ts Lines 8 to 19 in 9e17e18
And really, there's no need for that |
Given our current discussions, I hope you don't mind if I return this to draft to take it out of the review queue temporarily? |
…to fix/web/segment-path-interfacing
…to fix/web/segment-path-interfacing
Took some work, but I've successfully revived the branch; it's now based on the modularized form of the gesture engine. This does nothing to address previous review concerns; I just figured that getting this up and running before making further changes would probably be wise; it's best to start new work from a stable position, after all. |
This (and the 🐵 / gesture work) has been dormant for quite a while, so it'll probably take a bit to reboot discussion and our understandings fully. If memory serves, my perspective on the prior discussion was that I needed to look more at the "forest" of the prior discussion than the "trees" of it; the general concern as opposed to the details.
Looking back, and looking at the code that exists, I'm fairly confident that I can shift gears and go with a "single segment" approach for now. Makes me a bit sad, as I'd actually gotten the more advanced style's foundations working, but I suppose the "more advanced style" would take more time to vet properly. That said, there are still a few concerns and points of difference worth talking about.
This was one of the big contention points, I think. Part of the original gesture design-work aimed to ensure that we can reasonably extend to multi-segment flicks and/or swipe-typing in the future. It may be a "while off", of course. The point I'd like to make here: any expectation we set here for how certain gestures operate will continue to be an expectation if and when we do add the more complex ones.
And these points come to a head when we're talking about flicks. My position:
Your position, I think:
I believe we need to be "fancy" in this regard. We should validate that flicks are in a relatively straight line. Suppose the touchpath bends significantly - say, from 15 pixels straight North to moving another 15 pixels West.
Even worse - 15 pixels straight North, then 20 pixels straight South... for a net of 5 pixels South. Is this reasonable as a "flick to the South"?
Either way, the two sequences above are far better suited to either potential future gesture type; it'd be awkward to have to backtrack a decision to accept that the first as a single-segment NW flick in the future due to backward-compatibility/consistency. Fortunately... we can handle this reasonably with single-segment regression tests for now; "is this a straight line" is a pretty easy question for our segment stats-tracking mechanism to answer. One possible point of concern:
... and when does a 'macro movement' end? That's actually what subsegmentation is designed to detect and answer. Our existing answer in KMW 'til now: when a new key ends up underneath the touchpoint, if we aren't yet locked in on a longpress, consider it a "macro movement".
|
All of that last comment to say... I think it actually is possible to make the basic, earlier-targeted gestures work decently without the subsegmentation engine, and existing pieces are sufficient to fill in the gaps for now. In which case, I guess that means we continue to leave this PR iced 🧊 until we decide to revisit and extend to allow more complex gestures. |
I am still concerned on our differences regarding one notable point, though:
It's a flick - there are going to be usability issues in one direction or another. The speed aspect is also part of the word's standard definition. If we don't consider speed at all, I believe it impossible to fully reconcile with roaming touches - even with a more complicated model. Suppose a straight movement to the east for two keys' width on a touch device, then a 0.5 second pause.
Would your answer (re: option 1 or option 2?) differ if I said...
The problem being... if both are permitted, we probably can't confirm a roam until we've ruled out flicks. Speed would be a useful tool for detecting failed flicks in order to enter roaming mode.
If someone uses fast flicks - like, in the span of 0.2 seconds, a quick motion - I believe it would be harder for that person to consistently constrain the distance to meet such a "max distance" threshold. You'd end up penalizing the people who would otherwise get the most efficiency out of the feature, accidentally creating a "max speed" threshold. I'd see that going one of two ways:
Now, if we auto-cancel on reaching an OSK boundary or device-screen boundary... that's more self-evident and provides an easier-to-recognize pattern for cancellation. They're boundaries that don't "move" based on the original touchpoint. I do think we can move forward with dropping subsegmentation without arriving at a full answer to this concern, at least. The existing stats measurements do facilitate tracking speed, after all; it's just a matter of if we decide not to 'tap' that info. |
Indeed, which is why I have been saying drop roaming touches ... for the last however long. |
This is a critical part of the quoted statement.
I'm concerned about the relationship of accessibility to roaming touch & its key previews; this seems to be dropping a "more accessible" feature for a "less accessible" one, with no plans to reconcile them at any point. I do not believe reconciliation impossible... unless we drop flick speed as a factor in determining "flick or not?". This is part of why I've been adamantly lobbying for it as a flick criterion... for the last however long. To be clear... it does seem like we'll need to drop it for 17.0 anyway for the first wave of flick implementation - though we could offer an option of "disable all flicks" out of concern for accessibility for affected people, which would allow restoring it to those in need of key previews. Succinct through-line:
|
Took a bit of digging, but I found a few reference points that've been sticking in my mind:
That was added in unicode-org/cldr#2360. Even longpress is discouraged by the LDML keyboards doc, in fact:
|
We've triaged the segmentation aspects of this until way later if at all, opting for a different strategy in the meantime. Also, recent PRs have pulled in many of the other related aspects of this PR, such as the |
A part of the epic #7324 line, and the first member PR of "phase 3" for gesture implementation... at least "in spirit."
It's important to make sure that the segmentation engine's events and Promises provide a usable interface with consistent ordering to the upcoming gesture-synthesis layer. While doing very early prototyping for said layer, I came to realize that there were a few issues that made their use - especially in unit tests - far more difficult than would be ideal.
To clarify the issues and ensure they're properly addressed, this changeset also adds a unit test heavily based on said "very early prototyping", the start of which may be seen as follows:
keyman/common/web/gesture-recognizer/src/test/auto/headless/trackedPath.js
Lines 372 to 374 in 62afd6d
Since almost all of the Segments are produced and resolve asynchronously, using a state-machine based model to detect gestures needs to process each segment as it arrives, one at a time. However, the
start
Segment and the first pendingSegment
are produced at the same time, synchronously; this meant that specialized handling would be needed to get the event for bothSegment
s.Rather than requiring significantly complex handling external to
TrackedPath
and the segmentation engine, I believe the simplest solution is simply to provide both within a single 'raise' of the'segmentation'
event, hence the change to the event's signature. This may slightly complicate the synchronous signatures at play, but it prevents much greater complexity for interfacing with the asynchronous aspects of the design.It is then possible to handle each segment in sequence via
async
/await
as follows:@keymanapp-test-bot skip