Skip to content

Web: treat modifier key without location#98460

Merged
fluttergithubbot merged 4 commits intoflutter:masterfrom
moko256:moko256_web_map_no_location_key
Mar 16, 2022
Merged

Web: treat modifier key without location#98460
fluttergithubbot merged 4 commits intoflutter:masterfrom
moko256:moko256_web_map_no_location_key

Conversation

@moko256
Copy link
Contributor

@moko256 moko256 commented Feb 15, 2022 •
edited
Loading

On Chromium on Windows, the event of Shift, Control, Alt, and Meta key may have no location (e.g. #97582).
This PR treats their keys and map to synonym.

Will fix the part of #97582.

No changes in flutter/tests.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard
Copy link

flutter-dashboard bot commented Feb 15, 2022

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 (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@flutter-dashboard flutter-dashboard bot added framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Feb 15, 2022
@moko256 moko256 mentioned this pull request Feb 15, 2022
Open
@moko256 moko256 force-pushed the moko256_web_map_no_location_key branch from 73857c5 to 62163cd Compare February 15, 2022 21:51
@moko256 moko256 changed the title Web: treat modifier key without location as on the left Web: treat modifier key without location Feb 15, 2022
@moko256 moko256 force-pushed the moko256_web_map_no_location_key branch from 62163cd to 1025393 Compare February 15, 2022 21:52
@dkwingsmt
Copy link
Contributor

dkwingsmt commented Feb 16, 2022

Do you mean their location field might be 0? In that case I think it's better to give them XxxLeft. The unsided modifiers are synonyms only and will removedi the future.

Also can you add tests?

@moko256 moko256 force-pushed the moko256_web_map_no_location_key branch from 7fa2638 to e06edce Compare February 17, 2022 21:58
moko256 added a commit to moko256/flutter_engine that referenced this pull request Feb 18, 2022
@moko256
Web: treat modifier key without location (generate key map from flutt…
b4fdbae 
…er/flutter#98460)
@moko256 moko256 mentioned this pull request Feb 18, 2022
Merged
8 tasks
@moko256
Copy link
Contributor Author

moko256 commented Feb 18, 2022

I replaced synonym keys with left keys.
I added the test in flutter/engine#31550, that uses generated key_map.dart from this changes.

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Mar 4, 2022

The change looks great, but can you add a test where a RawKeyEventDataWeb for modifier location 0 results in the correct key? Thank you.

dkwingsmt
dkwingsmt approved these changes Mar 5, 2022
View reviewed changes
Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

LGTM

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Mar 5, 2022

@mdebbar Can you make a second review?

dkwingsmt pushed a commit to flutter/engine that referenced this pull request Mar 7, 2022
@moko256
Web: treat modifier key without location (generate key map from flutt…
12bb741 
…er/flutter#98460) (#31550)
@dkwingsmt dkwingsmt requested a review from mdebbar March 7, 2022 21:30
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 7, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 7, 2022
@engine-flutter-autoroll
12bb741 Web: treat modifier key without location (generate key map from
42ae54c 
flutter#98460) (flutter/engine#31550)
mdebbar
mdebbar approved these changes Mar 8, 2022
View reviewed changes
Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

LGTM!

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Mar 15, 2022

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite analyze-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Mar 15, 2022

Can you try rebasing this PR against the latest master?

@moko256 moko256 force-pushed the moko256_web_map_no_location_key branch from ae94c1a to 2e47c31 Compare March 15, 2022 23:39
@fluttergithubbot fluttergithubbot merged commit 13bf751 into flutter:master Mar 16, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 16, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2022
@engine-flutter-autoroll
13bf751 Web: treat modifier key without location (flutter/flutter#98460)
b438dd2
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 16, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 16, 2022
@engine-flutter-autoroll
13bf751 Web: treat modifier key without location (flutter/flutter#98460)
6b27627
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 16, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2022
@engine-flutter-autoroll
13bf751 Web: treat modifier key without location (flutter/flutter#98460)
4185bfc
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 16, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 16, 2022
@engine-flutter-autoroll
13bf751 Web: treat modifier key without location (flutter/flutter#98460)
262a092
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 16, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 16, 2022
@engine-flutter-autoroll
13bf751 Web: treat modifier key without location (flutter/flutter#98460)
d8f160b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mdebbar mdebbar mdebbar approved these changes

@dkwingsmt dkwingsmt dkwingsmt approved these changes

@yjbanov yjbanov Awaiting requested review from yjbanov

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@moko256 @dkwingsmt @fluttergithubbot @mdebbar