Migrate to KeyEvent from RawKeyEvent #1593

gspencergoog · 2023-11-10T23:39:17Z

Description

This does a major migration for SuperEditor to use KeyEvent instead of using RawKeyEvent-based events.

The RawKeyEvent system will shortly be deprecated by the Flutter team, and this PR fixes SuperEditor to handle that deprecation.

Unfortunately, this requires changing the public interface of SuperEditor, since some of the keyboard event handlers need to now take KeyEvents instead of RawKeyEvents.

The RawKeyEvent-based system has had deprecation warnings ("this will be deprecated" in the comments, not actual @Deprecated annotations) for a couple of years, and Flutter is finally officially deprecating that system, since the replacement has proven to be robust.

@matthew-carroll I haven't bumped any version numbers or anything here, since I'm not quite sure how you'd like to proceed with that, or how you want to handle this breaking change.

Related Issues

RawKeyEvent and RawKeyboard, et al should be deprecated and removed flutter/flutter#136419

Related PRs

Tests

Modified tests to match.

matthew-carroll · 2023-11-10T23:41:22Z

@gspencergoog is this only a breaking change in terms of API? Or are we losing behaviors that we depend upon? If it's just an API breaking change, can you please make the replacements and ensure all tests still pass?

@miguelcmedeiros @brian-superlist @Jethro87 @jmatth - Please be aware of this PR from the Flutter team

matthew-carroll · 2023-11-10T23:42:29Z

PS - Goldens are expected to fail at the moment due to some minor adjustments for mobile controls. But I think all other tests should be made to pass.

gspencergoog · 2023-11-11T00:04:38Z

As far as I can tell, I don't think you're losing any behaviors that you depend upon.

To be clear, removing the old system does remove some functionality (things having to do with specific platform implementations of key events in the RawKeyEventData subclasses), but I don't think you're using any of them.

Tests did pass locally for me. I'll look into the ones that aren't passing on CI.

gspencergoog · 2023-11-11T00:06:42Z

Oh, actually, I know why they're failing: they depend on a (breaking) change that hasn't landed yet in Flutter: flutter/flutter#136854

matthew-carroll · 2023-11-11T00:14:25Z

Ok, can you let us know in this PR when the other change is merged? We wouldn't want to merge a PR that breaks us against master.

gspencergoog · 2023-11-11T00:26:15Z

Ok, can you let us know in this PR when the other change is merged? We wouldn't want to merge a PR that breaks us against master.

Yes, of course. How exactly do you want to do "the dance" here? At some point, either when I merge the breaking change to the framework, or when we merge this change here before the framework is updated, your repo won't build against master for a short period of time. Once either one happens, presumably you wouldn't be publishing the package until they are in sync again and passing.

It sounds like you'd prefer me to push the framework change first, and then update this change to validate against the new framework master before you commit this change here. Does that sound right?

matthew-carroll · 2023-11-11T01:22:53Z

It sounds like you'd prefer me to push the framework change first, and then update this change to validate against the new framework master before you commit this change here. Does that sound right?

Yeah go ahead and update master for Flutter. As we know, it's possible that those changes will be reverted and reworked for any number of unexpected reasons. So I'd rather have you and the team get everything squared away in the framework and then we can adjust super_editor accordingly.

gspencergoog · 2023-11-13T18:19:12Z

It looks like there are about 50 different APIs in SuperEditor that take a RawKeyEvent that I've converted to use KeyEvent here, but in order to land the change in the Framework, I'll either need to convert all of those to dynamic (and have duplicate code to handle either), commit them here, and then convert them all back again and remove the duplicate code once the framework PR lands, or we'll need to disable the SuperEditor customer tests temporarily until both the Framework change and this PR land.

Would you be OK with disabling the customer tests temporarily to save us some time and avoid the churn in your code?

matthew-carroll · 2023-11-13T18:57:09Z

Would you be OK with disabling the customer tests temporarily to save us some time and avoid the churn in your code?

Yes, as long as we re-enable them ASAP.

gspencergoog · 2023-11-13T19:34:08Z

Yes, the plan would be to re-enable them ASAP (we want them enabled as much as you do!)

I wouldn't disable them until both of the PRs were ready and approved, and then I'd submit the framework PR, and then rebase this PR, submitting it (with your cooperation) as soon as the presubmits passed, and then re-enable the customer tests right after that.

gspencergoog · 2023-11-15T18:01:55Z

@matthew-carroll Okay, the Flutter change is ready to be committed. Once you approve flutter/tests#312, I'll submit that, let the presubmit pass on flutter/flutter#136854 and then submit it. Once that's submitted, I'll push an empty change to this PR to re-run the presubmits. Then, once this PR is approved, you can submit it and I'll re-enable the customer_testing tests with an updated ref. Sound good?

## Description This temporarily disables the super_editor tests while flutter/flutter#136854 and superlistapp/super_editor#1593 are landed, to avoid a lot of churn in the super_editor repo. ## Related Issues - flutter/flutter#136419

gspencergoog · 2023-11-15T23:39:51Z

It looks like 1) there are the (expected, I think) golden file failures, and 2) the deploy tests are failing because it's trying to build this PR with the stable branch. Is that expected?

gspencergoog · 2023-11-16T01:01:02Z

Okay, I made this PR more compatible with the stable branch by adding some extension methods onto KeyEvent for accessing which keys are down, but that isn't enough to fix the stable breakage (because of the breaking change that precipitated all of this). It looks like there are other breaks unrelated to this PR in the three failing tests, however.

super_editor/example/lib/demos/in_the_lab/popover_list.dart

super_editor/lib/src/infrastructure/focus.dart

super_editor/lib/src/infrastructure/keyboard.dart

super_editor/lib/src/infrastructure/raw_key_event_extensions.dart

super_editor/lib/src/super_reader/read_only_document_keyboard_interactor.dart

super_editor/lib/src/infrastructure/raw_key_event_extensions.dart

super_editor/test/super_editor/text_entry/tagging/action_tags_test.dart

super_editor/test/super_editor/text_entry/text_test.dart

matthew-carroll · 2023-11-21T01:22:04Z

@angelosilvestre @rutvik110 - Please do a review pass on this PR to see if you find anything that I missed.

@miguelcmedeiros @jmatth @Jethro87 - I've done an initial review of this PR from the Flutter team. Once that the review completes, I plan to merge this in. So if you have any input, now is the time.

rutvik110 · 2023-11-21T05:03:50Z

LGTM!

angelosilvestre

@matthew-carroll I don't have any other comments besides what you've already pointed out.

gspencergoog · 2023-11-27T20:23:54Z

@matthew-carroll PTAL: I think I've addressed all the issues, and the tests should pass this time.

gspencergoog · 2023-11-27T20:38:22Z

The tests didn't pass because of some golden test failures, and BlinkTimingMode being missing (no idea why), and because the symbols in the Flutter master branch on HardwareKeyboard that I'm using are missing. They are available in the master branch, so is there a ref or version or something that needs to be updated so that the tests run against that?

matthew-carroll · 2023-11-27T20:55:08Z

The tests didn't pass because of some golden test failures, and BlinkTimingMode being missing (no idea why), and because the symbols in the Flutter master branch on HardwareKeyboard that I'm using are missing. They are available in the master branch, so is there a ref or version or something that needs to be updated so that the tests run against that?

@gspencergoog I see all tests as green except the goldens. Which failures are you referring to?

gspencergoog · 2023-11-27T21:15:48Z

I see all tests as green except the goldens. Which failures are you referring to?

The suspicious-kepler ones, for example: https://app.netlify.com/sites/suspicious-kepler-28398e/deploys/6564fc545c05840008faeb79

gspencergoog · 2023-11-27T21:44:41Z

Looks like all of the golden tests that are failing here are also failing on main against stable (with seemingly identical differences), so we can probably ignore those.

matthew-carroll · 2023-11-27T21:50:05Z

@gspencergoog I think we're ignoring the marketing website CI checks. We should probably fix that at some point, but I don't think we need to worry about it here. All relevant tests appear to be passing. I'll do another PR review when I get a minute.

matthew-carroll · 2023-11-29T01:21:51Z

super_editor/lib/src/default_editor/document_hardware_keyboard/document_keyboard_actions.dart

 }) {
-  if (keyEvent is! RawKeyDownEvent) {
+  if (keyEvent is! KeyDownEvent && keyEvent is! KeyRepeatEvent) {


Looks like we still have some KeyRepeatEvent checks in this PR. Can you remove all of those, unless they already exist somewhere?

I can, if you want, but I don't think that's correct. The reason I added them is that it implements the existing behavior.

In the RawKeyEvent system, repeat events are RawKeyDownEvents with a repeat boolean set to true. In the new system, they are a separate event type. If I don't include them in these types of conditionals, then the behavior will change from handling both down and repeat events to just handling down events and ignoring repeat events, or worse, treating them like up events.

Oh, ok. I misunderstood. I thought all the events were the same but with different names. Given that information, the changes make sense.

super_editor/lib/src/default_editor/text.dart

super_editor/lib/src/infrastructure/focus.dart

super_editor/lib/src/infrastructure/keyboard.dart

super_editor/test/super_editor/text_entry/tagging/action_tags_test.dart

matthew-carroll · 2023-11-29T01:31:24Z

super_editor/test/super_editor/text_entry/text_test.dart

@@ -51,52 +53,49 @@ void main() {

    group('TextComposable text entry', () {
      test('it does nothing when meta is pressed', () {
+        // Make sure we're using our fake binding so we can override keyboard
+        // modifier state.
+        assert(ServicesBinding.instance == binding);


I'm confused by this assertion here with the binding being set up above. Is the assert here because this test just so happens to be the first test that runs, and all the other tests assume this test runs first? If so, that seems very fragile, and unlikely that other developers will comprehend that implicit order requirement.

If, on the other hand, this binding is only needed in this test, then let's initialize it here, use it, and reset it, all within the one test that cares.

Once a binding is initialized, it can't be reset. The act of instantiating it initializes the instance, and there isn't an interface to reset the instance.

I added the assert to make sure that the binding initialization wasn't removed, there's no implicit order.

I moved the test to a separate file so that only it is affected by the binding change.

super_editor/test/super_editor/text_entry/text_test.dart

matthew-carroll · 2023-11-29T01:32:40Z

super_editor/test/super_editor/text_entry/text_test.dart

          ),
        );

        // The handler should pass on handling the key.
        expect(result, ExecutionInstruction.continueExecution);
      });

-      test('it does nothing when the key doesn\'t have a character', () {
+      testWidgets('it does nothing when the key doesn\'t have a character', (WidgetTester tester) async {


Why are some of these widget tests and others are Dart tests?

Because having it be a testWidgets test makes sure that the appropriate test binding is initialized. If they are converted to dart tests, then accessing the HardwareKeyboard instance to get the keyboard will fail because the ServicesBinding isn't initialized.

Alternatively, these could all be Dart tests, and you could call AutomatedTestWidgetsFlutterBinding.ensureInitialized() at the top, but then when debugging the tests (which uses LiveTestWidgetsFlutterBinding), you'd have the wrong binding initialized. Using testWidgets ensures that the correct binding is initialized.

matthew-carroll

LGTM

matthew-carroll · 2023-11-29T21:14:05Z

@angelosilvestre if you'd like to try logging Flutter master commits, here's one to track.

…erlistapp#1593)

gspencergoog mentioned this pull request Nov 15, 2023

Temporarily disable super_editor tests flutter/tests#312

Merged

gspencergoog added 2 commits November 15, 2023 15:06

Migrate to KeyEvent from RawKeyEvent

6e146c4

Fix Tests

2860bbb

gspencergoog force-pushed the migrate_key_event branch from 518794e to 2860bbb Compare November 15, 2023 23:06

Empty change to restart CI

9067243

Make compatible with stable branch

5474441

angelosilvestre mentioned this pull request Nov 18, 2023

[SuperTextField][mobile] Migrate popover toolbars to OverlayPortal (Resolves #1602) #1604

Merged

matthew-carroll reviewed Nov 21, 2023

View reviewed changes

gspencergoog added 2 commits November 21, 2023 11:32

Simplify extension methods

6a01414

Fix text_test

8b021f4

angelosilvestre reviewed Nov 22, 2023

View reviewed changes

Fix tests

70e7a8c

Empty change to encourage GitHub to process the update

20bfddc

rutvik110 mentioned this pull request Nov 28, 2023

[SuperTextField] [SuperDesktopTextField] Adds Page-Up/Down, Home/End selectors and handlers to SuperDesktopTextField (Resolves #1438) #1463

Merged

matthew-carroll reviewed Nov 29, 2023

View reviewed changes

Review changes

64d087f

matthew-carroll approved these changes Nov 29, 2023

View reviewed changes

matthew-carroll merged commit 46058e5 into superlistapp:main Nov 29, 2023
7 of 11 checks passed

gspencergoog deleted the migrate_key_event branch November 29, 2023 21:15

rutvik110 mentioned this pull request Dec 4, 2023

[SuperReader][SuperTextField] Applies color attribution on textstyle created through default style builder (Resolves #1579) #1626

Merged

dxvid-pts pushed a commit to dxvid-pts/super_editor that referenced this pull request Feb 11, 2024

Migrate to KeyEvent from RawKeyEvent (Flutter team contribution) (sup…

086f338

…erlistapp#1593)

angelosilvestre mentioned this pull request Feb 16, 2024

Flutter 3.19 updates #1841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to KeyEvent from RawKeyEvent #1593

Migrate to KeyEvent from RawKeyEvent #1593

gspencergoog commented Nov 10, 2023

matthew-carroll commented Nov 10, 2023

matthew-carroll commented Nov 10, 2023

gspencergoog commented Nov 11, 2023

gspencergoog commented Nov 11, 2023

matthew-carroll commented Nov 11, 2023

gspencergoog commented Nov 11, 2023

matthew-carroll commented Nov 11, 2023

gspencergoog commented Nov 13, 2023

matthew-carroll commented Nov 13, 2023

gspencergoog commented Nov 13, 2023

gspencergoog commented Nov 15, 2023 •

edited

Loading

gspencergoog commented Nov 15, 2023

gspencergoog commented Nov 16, 2023 •

edited

Loading

matthew-carroll commented Nov 21, 2023

rutvik110 commented Nov 21, 2023

angelosilvestre left a comment

gspencergoog commented Nov 27, 2023

gspencergoog commented Nov 27, 2023 •

edited

Loading

matthew-carroll commented Nov 27, 2023

gspencergoog commented Nov 27, 2023

gspencergoog commented Nov 27, 2023

matthew-carroll commented Nov 27, 2023

matthew-carroll Nov 29, 2023

gspencergoog Nov 29, 2023

matthew-carroll Nov 29, 2023

matthew-carroll Nov 29, 2023

gspencergoog Nov 29, 2023

matthew-carroll Nov 29, 2023

gspencergoog Nov 29, 2023

matthew-carroll left a comment

matthew-carroll commented Nov 29, 2023

Migrate to KeyEvent from RawKeyEvent #1593

Migrate to KeyEvent from RawKeyEvent #1593

Conversation

gspencergoog commented Nov 10, 2023

Description

Related Issues

Related PRs

Tests

matthew-carroll commented Nov 10, 2023

matthew-carroll commented Nov 10, 2023

gspencergoog commented Nov 11, 2023

gspencergoog commented Nov 11, 2023

matthew-carroll commented Nov 11, 2023

gspencergoog commented Nov 11, 2023

matthew-carroll commented Nov 11, 2023

gspencergoog commented Nov 13, 2023

matthew-carroll commented Nov 13, 2023

gspencergoog commented Nov 13, 2023

gspencergoog commented Nov 15, 2023 • edited Loading

gspencergoog commented Nov 15, 2023

gspencergoog commented Nov 16, 2023 • edited Loading

matthew-carroll commented Nov 21, 2023

rutvik110 commented Nov 21, 2023

angelosilvestre left a comment

Choose a reason for hiding this comment

gspencergoog commented Nov 27, 2023

gspencergoog commented Nov 27, 2023 • edited Loading

matthew-carroll commented Nov 27, 2023

gspencergoog commented Nov 27, 2023

gspencergoog commented Nov 27, 2023

matthew-carroll commented Nov 27, 2023

matthew-carroll Nov 29, 2023

Choose a reason for hiding this comment

gspencergoog Nov 29, 2023

Choose a reason for hiding this comment

matthew-carroll Nov 29, 2023

Choose a reason for hiding this comment

matthew-carroll Nov 29, 2023

Choose a reason for hiding this comment

gspencergoog Nov 29, 2023

Choose a reason for hiding this comment

matthew-carroll Nov 29, 2023

Choose a reason for hiding this comment

gspencergoog Nov 29, 2023

Choose a reason for hiding this comment

matthew-carroll left a comment

Choose a reason for hiding this comment

matthew-carroll commented Nov 29, 2023

gspencergoog commented Nov 15, 2023 •

edited

Loading

gspencergoog commented Nov 16, 2023 •

edited

Loading

gspencergoog commented Nov 27, 2023 •

edited

Loading