Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

mdebbar
Copy link
Contributor

@mdebbar mdebbar commented Apr 15, 2020

Before these changes, if a user clicks a browser short (e.g. cmd+alt+i which is common for developers) the browser would send the following sequence of events:

keydown(cmd) -> keydown(alt) -> keydown(i) -> keyup(alt) -> keyup(cmd)

Notice the absence of a keyup(i). Because the shortcut is handled by the browser, a keyup isn't reported.

This puts the framework in a corrupt state because it thinks "i" is still held down, and any subsequent "Tab" presses won't trigger a focus change (because isn't pressed alone).

In order to fix this, we need to synthesize a keyup when we know for sure that the user isn't holding down the key. That can be achieved by watching for repeat events. If no repeat events have been fired for a specific duration, we assume the user isn't holding the key down anymore.

Could be related to flutter/flutter#52482

@mdebbar mdebbar added the platform-web Code specifically for the web engine label Apr 15, 2020
@mdebbar mdebbar requested a review from yjbanov April 15, 2020 22:19
@mdebbar mdebbar self-assigned this Apr 15, 2020
@@ -19,6 +20,8 @@ class Keyboard {
static Keyboard get instance => _instance;
static Keyboard _instance;

final Map<String, Timer> _keydownTimers = <String, Timer>{};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a doc comment describing the keys and (briefly) the timer logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm trying to think if we really want just one timer and a list of keys that are pending a keyup. With every key activity we just push the timer farther out. We cancel the timer when all keys received a keyup or the timer fires.

My reasoning is that there's really only one key combination that can be entered at any given point in time, and conceptually we are tracking the whole key combination, not individual keys. Either the whole combination completes successfully, or the whole combination times out and we backfill the keyups. The corner case I'm imagining is a key combination that's being entered slowly such that you add a new key to the combo in <1 second, but the overall combo goes past the 1-second timer. What we'll end up doing in that case is start timing out some of the initial keys while there are more keys being entered, which doesn't seem correct.

BTW, see also the alarm_clock.dart utility that does something very similar for touch event debouncing. I think if you add a public cancel method to it it will do what you want, with just one instance of the class. As a bonus it makes sure to not use more than one timer at a time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think a single timer would work in this case.

Let's say I'm typing in a text field, and I do cmd+c to copy something (we don't receive a keyup for "c" because it's a browser/system shortcut). Then I immediately continue to type (the timer keeps getting pushed and meanwhile the framework still thinks "c" is down). Then I quickly hit Tab. The framework won't move focus to the next text field because it thinks both "c" and "Tab" are down.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting case. Does having per-key timers fully solve the problem, especially after increasing it to 2.5 seconds. If you type fast enough, the Tab can come in before "c" expires. Maybe we should keep it at 1 second?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't fully solve the problem. It only solves some of the common cases. I keep going back and forth between 1 or 2.5 seconds. I can't make up my mind one way or another. You vote for 1 second?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if we're looking for the least bad solution, what do you think of sending keyup for everything that's pending when we see any non-meta keydown?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work for games, right? In games, it's not uncommon for multiple non-meta keys to be down at the same time (e.g. moving and shooting at the same time).

Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The other comments are non-blocking.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants