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

[Web, Keyboard] Implement FlutterHtmlKeyboardEvent.keyCode and defaultPrevented, and rename Keyboard to RawKeyboard #34626

Merged
merged 3 commits into from
Oct 3, 2022
Merged

[Web, Keyboard] Implement FlutterHtmlKeyboardEvent.keyCode and defaultPrevented, and rename Keyboard to RawKeyboard #34626

merged 3 commits into from
Oct 3, 2022

Conversation

@dkwingsmt
Copy link
Contributor

@dkwingsmt dkwingsmt commented Jul 13, 2022 •
edited
Loading

This PR implements keyCode and defaultPrevented of FlutterHtmlKeyboardEvent. The keyCode was deemed useful in an earlier PR but was missing from this class, while defaultPrevented replaces a long-standing hack used by keyboard_converter_test.dart. No semantic changes are made to the tests, except for line 613 and line 637 where the results are changed to true because the previous results are no longer cleared.

Also, this PR renames Keyboard to RawKeyboard. Started as the only keyboard processor, Keyboard now handles the legacy "raw keyboard" system, one of the two keyboard processing systems. The rename aligns with its role change, and is a precursor to a later change, where we embed RawKeyboard as part of KeyboardBinding so that the two processing systems can share the same event listeners as entrances.

Other minor changes include:

  • Extract MockKeyboardEvent into a separate file, and implemement its modifierState.
  • Remove KeyboardBinding's now redundant parameter glassPaneElement.

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 and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Jul 13, 2022
@dkwingsmt dkwingsmt changed the title [Web, Keyboard] Merge the callbacks of hardware keyboard and raw keyboard [Web, Keyboard] Rename Keyboard to RawKeyboard Jul 14, 2022
@dkwingsmt dkwingsmt marked this pull request as ready for review July 14, 2022 01:52
@dkwingsmt dkwingsmt requested a review from gspencergoog July 14, 2022 01:52
@dkwingsmt dkwingsmt changed the title [Web, Keyboard] Rename Keyboard to RawKeyboard [Web, Keyboard] Implement FlutterHtmlKeyboardEvent.keyCode, modifierState, and defaultPrevented Jul 14, 2022
@dkwingsmt dkwingsmt changed the title [Web, Keyboard] Implement FlutterHtmlKeyboardEvent.keyCode, modifierState, and defaultPrevented [Web, Keyboard] Implement FlutterHtmlKeyboardEvent.keyCode and defaultPrevented, and rename Keyboard to RawKeyboard Jul 14, 2022
@dkwingsmt
Rename
a926e1a 
Raw use flutter event

All FlutterHtmlKeyboardEvent

Migrate to defaultPrevented

Fix RawKeyboard
@gspencergoog
Copy link
Contributor

gspencergoog commented Oct 3, 2022

Sorry, @dkwingsmt this one slipped through the cracks. Do you still want to submit it?

@gspencergoog
Copy link
Contributor

gspencergoog commented Oct 3, 2022

If so, I'm happy to review it.

@dkwingsmt
Copy link
Contributor Author

dkwingsmt commented Oct 3, 2022

@gspencergoog Yes, please review it, thank you!

gspencergoog
gspencergoog approved these changes Oct 3, 2022
View reviewed changes
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

lib/web_ui/test/keyboard_converter_test.dart Outdated
converter.handleEvent(keyDownEvent('KeyA', 'a'));
converter.handleEvent(keyUpEvent('KeyA', 'a'));
converter.handleEvent(keyUpEvent('KeyA', 'a'));
event = keyDownEvent('KeyA', 'a');
Copy link
Contributor

@gspencergoog gspencergoog Oct 3, 2022

Choose a reason for hiding this comment

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

What's the reason for splitting these into two lines each everywhere instead of one? To me, they seem a lot more readable as one line.

Copy link
Contributor Author

@dkwingsmt dkwingsmt Oct 3, 2022

Choose a reason for hiding this comment

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

It was used to hold the created event object and verify the defaultPrevented. I have changed it by adding a lastDefaultPrevented static field to the mock class.

@dkwingsmt
Rename
3f2e842 
Raw use flutter event

All FlutterHtmlKeyboardEvent

Migrate to defaultPrevented

Fix RawKeyboard
@dkwingsmt
Remove extra event creation line
6e0eb2c
@skia-gold
Copy link

skia-gold commented Oct 3, 2022

Gold has detected about 4 new digest(s) on patchset 5.
View them at https://flutter-engine-gold.skia.org/cl/github/34626

@dkwingsmt dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 3, 2022
@auto-submit auto-submit bot merged commit 4e69f62 into flutter:main Oct 3, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 3, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 3, 2022
@engine-flutter-autoroll
4e69f628b [Web, Keyboard] Implement `FlutterHtmlKeyboardEvent.keyCode…
7d761ac 
…` and `defaultPrevented`, and rename `Keyboard` to `RawKeyboard` (flutter/engine#34626)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@gspencergoog gspencergoog gspencergoog approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

affects: text input autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dkwingsmt @gspencergoog @skia-gold