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

Conversation

@ferhatb
Copy link
Contributor

@ferhatb ferhatb commented Oct 3, 2020

Description

The web engine, to prevent the crash and keep Tab key working no matter what the lock state is, sets metastate for locks during keydown until handling at framework level is clarified.

metaState as part of RawKeyEvent is currently not making a clear distinction between modifiers and lock states. For example the code requires that NumLock state be true when user presses NumLock key (it will crash otherwise). The lock state on the other hand, won't be set in a browser until keyup on some platforms (see https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/getModifierState).

Past issues,PRs related to this:
#45250
#46718
#14165

So if engine interprets metaState as persistent lock state such as NumLock, shortcuts break since the matcher will fail since there is no entry for NumLock+Tab or CapsLock+NumLock+Tab for example. Ideally? shortcuts should only consider lock states if it is explicitly provided to add specificity. The ambiguity of NumLock+key needs to be resolved to either mean Option1: a key is pressed when in NumLock state or Option2: key is pressed in combination with NumLock key at the same time. It should still allow a developer to distinguish between them by tracking keydown/keyup events.

Related Issues

flutter/flutter#66601

Tests

Added test case to e2etests/web/regular_integration_tests/test_driver/text_editing_integration.dart

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [contributor guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [C++, Objective-C, Java style guides] for the engine.
  • I read the [tree hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read [handling breaking changes].

@ferhatb ferhatb requested review from mdebbar and nturgut October 3, 2020 17:45
@flutter-dashboard
Copy link

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.

@auto-assign auto-assign bot requested a review from GaryQian October 3, 2020 17:45
@ferhatb ferhatb changed the title Capslock2 [web] Fix CapsLock keyevent Oct 3, 2020
@nturgut
Copy link
Contributor

nturgut commented Oct 5, 2020

Question: which framework integration test covers this case?

@ferhatb
Copy link
Contributor Author

ferhatb commented Oct 5, 2020

Question: which framework integration test covers this case?

Should be added to Tab e2e test after engine PR is in.

Copy link
Contributor

@nturgut nturgut left a comment

Choose a reason for hiding this comment

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

As discussed earlier, the e2e test can also go in with this PR.

LGTM.

@ferhatb
Copy link
Contributor Author

ferhatb commented Oct 5, 2020

As discussed earlier, the e2e test can also go in with this PR.

LGTM.

Added test case to e2etests/web/regular_integration_tests/test_driver/text_editing_integration.dart

Copy link
Contributor

@nturgut nturgut left a comment

Choose a reason for hiding this comment

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

Thanks for adding the integration test :)

@ferhatb ferhatb merged commit 284ef22 into flutter:master Oct 5, 2020
@ferhatb ferhatb deleted the capslock2 branch October 5, 2020 19:49
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 5, 2020
gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Oct 20, 2020
* Fix metaState for Key events for lock modifiers
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants