Fixing synthesizing keys for multiple keys pressed down on flutter web #19632
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
/cc: @mdebbar |
I just realised that I just failed to find the test file, but it exists, here it is: https://github.com/erickzanardo/engine/blob/bec9c89fabfecf4ef529ed90214740bfaa6aa52f/lib/web_ui/test/keyboard_test.dart Will try to work on the test case for this change soon |
|
Apologies for the late response, I missed the email notification for the PR. Thanks for working on this issue. Yes, the correct place to add tests for this is in This PR is causing one test to fail: |
Sure thing, I will be working on the tests for this PR someday this week, I will report back here if I succeed or if I have any issue, thanks!! |
|
@mdebbar it took me some time to be able to get back to this sorry. I have added the a new test and fixed the one breaking. That test was breaking because since with this change, only keys affected by the meta modifiers are synthesised, triggering repeat events don't cancel that synthesizing. From what I tested on the navigators, when a short is pressed, repeat events should not follow, so this should not be any problems, nevertheless I changed the test to trigger meta modified repeat events, in the case I missed some case in my tests on the browsers. Please, let me know what do you think. |
lib/web_ui/test/keyboard_test.dart
Outdated
| key: 'i', | ||
| code: 'KeyI', | ||
| repeat: true, | ||
| isMetaPressed: true, |
There was a problem hiding this comment.
This test is trying to simulate the case where the meta keys have been released. I think we shouldn't use isMetaPressed: true because the meta key has been released already (see line 436 above).
lib/web_ui/test/keyboard_test.dart
Outdated
| 'key': 'i', | ||
| 'code': 'KeyI', | ||
| 'metaState': 0x0, | ||
| 'metaState': 0x08, |
There was a problem hiding this comment.
Same thing here. Meta keys have been released so this should remain 0x0.
| if (!_isModifierKey(event)) { | ||
| // Don't synthesize a keyup event for modifier keys, or keys not affected by modifiers, | ||
| // because the browser always sends a keyup event for those. | ||
| if (!_isModifierKey(event) && _isAffectedByModifiers(event)) { |
There was a problem hiding this comment.
To fix the issue that's making tests fail, let's move the new condition inside. This way we guarantee that the syntehsizing timer is always canceled.
Does something like this work:
if (!_isModifierKey(event)) {
_keydownTimers[timerKey]?.cancel();
if (event.type == 'keydown' && _isAffectedByModifiers(event)) {
_keydownTimers[timerKey] = Timer(_keydownCancelDuration, () {
_keydownTimers.remove(timerKey);
_synthesizeKeyup(event);
});
} else {
_keydownTimers.remove(timerKey);
}
}There was a problem hiding this comment.
I remembering trying that, but that didn't solved the issue (flutter/flutter#58727), but right now, I can't remember why, I will give this another try, because looking again, this suggestion should work! Maybe I have confused things.
There was a problem hiding this comment.
@erickzanardo did you have a chance to try it out?
There was a problem hiding this comment.
Hey, I haven't yet, sorry, I have been really overwhelmed at work. But I am planning on getting back to this PR on this saturday or sunday.
There was a problem hiding this comment.
No rush.
If the suggestion doesn't work, please ping me so we can discuss it further.
There was a problem hiding this comment.
I have updated the PR, let me know if this is ok now! Thanks for the instructions on this!
| // Wait for a long-enough period of time and no events | ||
| // should be synthesized | ||
| async.elapse(Duration(seconds: 3)); | ||
| expect(messages, <Map<String, dynamic>>[ |
There was a problem hiding this comment.
@mdebbar Your suggestion worked, and moving the condition deeper inside the if worked fine to solve the original issue.
To fix the test, I needed to remove this last assertion, which I realize, is not needed anymore, since we only synthesise key ups for meta affected keys, and up in the test, we trigger a bunch of repeat events, those would have cleared the synthesising timer, and therefore, no keyup should be synthesise. Let me know if this makes sense.
|
@erickzanardo your branch is based on an old commit in master. Could you please pull master and rebase your branch? Then push your branch. Once that's done, we'll wait for all the tests to pass then I'll merge the PR. Thanks again for the contribution! |
Sure thing, will do that tonight! |
|
@mdebbar Rebased with master! Let me know if that is ok. Thanks |
|
@mdebbar I notice that some tests will failed, which seems unrelated to the change on this PR? Let me know if a new rebase would be required |
|
You are right. Both are unrelated. There's no need to rebase at the moment.
Once the re-run passes, and the |
Great, thanks for clarifying it! |
mdebbar
added
platform-web
* b08c6b9 Fixing synthesizing keys for multiple keys pressed down on flutter web (flutter/engine#19632) * 8308b6a Avoid passing nil to IOS accessibility announcement (flutter/engine#20700) * 950b6a0 Roll Skia from ead4ca052b99 to 5da7327358e3 (1 revision) (flutter/engine#20782) * 87fd0e4 Guard recording_canvas against restore calls without ending recording (flutter/engine#20786) * 388193a Add tests for lerpDouble (flutter/engine#20778)
* b08c6b9 Fixing synthesizing keys for multiple keys pressed down on flutter web (flutter/engine#19632) * 8308b6a Avoid passing nil to IOS accessibility announcement (flutter/engine#20700) * 950b6a0 Roll Skia from ead4ca052b99 to 5da7327358e3 (1 revision) (flutter/engine#20782) * 87fd0e4 Guard recording_canvas against restore calls without ending recording (flutter/engine#20786) * 388193a Add tests for lerpDouble (flutter/engine#20778)
* b08c6b9 Fixing synthesizing keys for multiple keys pressed down on flutter web (flutter/engine#19632) * 8308b6a Avoid passing nil to IOS accessibility announcement (flutter/engine#20700) * 950b6a0 Roll Skia from ead4ca052b99 to 5da7327358e3 (1 revision) (flutter/engine#20782) * 87fd0e4 Guard recording_canvas against restore calls without ending recording (flutter/engine#20786) * 388193a Add tests for lerpDouble (flutter/engine#20778)
4 participants
Description
The current implementation of the keyboard on the web engine had a logic to automatically trigger keyup events, when repeat events stoped occurring for a time.
That works great when only one key is pressed down, but when two keys are being pressed down together, the first key will stop to send repeat events and only the last key been pressed down will send repeat events.
That was causing a strange behaviour, as a keyup event would be triggered, even if the key was still pressed down.
I did some investigation, and due to some comments on the code, I discovered that this logic that auto triggers keyup when repeat event cease, was created to prevent the framework be on a invalid state due to a keyup event not happening because the browser don't trigger keyups when a key is used on a browser/system shortcut. So my solution was built to that same idea, and I add a small new logic, that only synthesise keyup events when a key can be used in a shortcut, which is when a modifier (control, alt, shift or meta) key is pressed together.
Related Issues
Fixes flutter/flutter#58727
Tests
test/keyboard_test.dartChecklist
Breaking Change
Did any tests fail when you ran them? Please read [handling breaking changes].