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

angelosilvestre · 2023-11-18T00:27:57Z

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

Currently, the mobile toolbars are displayed in the app Overlay. Because of that, it doesn't have access to InheritedWidgets placed below MaterialApp.

As a result, if an app adds a Theme widget above SuperTextField, the SuperTextField uses this Theme, but the popover toolbar uses the MaterialApp's theme.

This PR changes the popover toolbars to be displayed in an OverlayPortal instead of an Overlay. By doing so, the popovers "see" the same InheritedWidgets that the SuperTextField sees.

In the tests, I had to use the platform textfields directly, as SuperTextField doesn't expose the popoverToolbarBuilder in its public API.

I also noticed that the iOS textfield is always using the _defaultPopoverToolbarBuilder instead of the given popoverToolbarBuilder. This PR fixes that.

angelosilvestre · 2023-11-18T00:34:15Z

@matthew-carroll Tests are failing due to a compilation error:

lib/src/default_editor/document_hardware_keyboard/document_physical_keyboard.dart:125:23: Error: The argument type 'RawKeyEvent' can't be assigned to the parameter type 'KeyEvent'.
 - 'RawKeyEvent' is from 'package:flutter/src/services/raw_keyboard.dart' ('/opt/hostedtoolcache/flutter/master-any-x64/packages/flutter/lib/src/services/raw_keyboard.dart').
 - 'KeyEvent' is from 'package:flutter/src/services/hardware_keyboard.dart' ('/opt/hostedtoolcache/flutter/master-any-x64/packages/flutter/lib/src/services/hardware_keyboard.dart').
      if (key.accepts(keyEvent, RawKeyboard.instance)) {

It seems to be related to this PR: #1593

matthew-carroll · 2023-11-21T01:27:34Z

super_editor/lib/src/super_textfield/android/android_textfield.dart

+  final _popoverController = OverlayPortalController();
+
+  /// Notifies the popover toolbar to rebuild itself.
+  final _popoverRebuildSignal = SignalNotifier();


Is this rebuild signal here because we're scheduling extra frames to position the toolbar? If so, can we use a Leader and Follower instead?

Yes, this is used to position the toolbar. Currently, we are doing _controlsOverlayEntry?.markNeedsBuild(); to do this.

We can use Leader and Follower, but it will require big change. I'm not sure we should do that on this PR.

Ok. We can do it in a follow up. Can you file a ticket for that?

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

super_editor/lib/src/super_textfield/android/android_textfield.dart

-            child: TextScrollView(
-              key: _scrollKey,
-              textScrollController: _textScrollController,
+    return OverlayPortal(


Now that we're bringing an overlay portal into this, let's breakdown this build method so it's clear which pieces are what.

matthew-carroll · 2023-11-21T01:30:14Z

super_editor/test/super_textfield/super_textfield_theme_test.dart

+
+void main() {
+  group('SuperTextField', () {
+    testWidgetsOnIos('applies app theme to the popover toolbar (on iOS)', (tester) async {


You don't need to specify "(on iOS)" when using testWidgetsOnIos()

testWidgetsOnIos itself doesn't add the "(on iOS)" suffix.

Maybe it's time to change that?

Really? I thought I saw "(on iOS)" appended to all sorts of tests that we run? If for some reason that test runner is the only one that isn't adding its name, then yes, we should add that, but I was almost certain that we already have iOS tests that add the suffix.

@matthew-carroll It's the testWidgetsOnMobile and testWidgetsOnAllPlatforms that appends the suffixes. Can we make that change in a separate PR?

matthew-carroll · 2023-11-21T01:35:18Z

This PR mentions that SuperTextField doesn't support any configuration of the overlay controls. We should probably provide that control. The question is, how should we do that?

In SuperEditor, I'm in the middle of moving overlay controls from widget properties over to ancestor composition with "scope" widgets. The rationale for this is that we were accumulating a lot of configurations: control color, handle builder, magnifier builder, toolbar builder, objects to show/hide each of those.

Should we repeat the approach from SuperEditor for SuperTextField and introduce scope widgets? Or should we introduce widget properties? Either way, we should probably do that in a separate PR, but we can decide the best approach while we're here.

angelosilvestre · 2023-11-22T22:44:31Z

Should we repeat the approach from SuperEditor for SuperTextField and introduce scope widgets? Or should we introduce widget properties? Either way, we should probably do that in a separate PR, but we can decide the best approach while we're here.

@matthew-carroll If we want to be consistent, we probably want to use the same approach from SuperEditor. The downsides is that it's a bit more difficult for app developers than just using widget properties and less discoverable (intelisense won't help them). We'll need good docs and probably mention that in the README.

matthew-carroll · 2023-11-23T04:40:45Z

@angelosilvestre can you file an issue about compositional overlay controls to match SuperEditor?

matthew-carroll

LGTM - Pending the "(on iOS)" suffix for the test runner

matthew-carroll · 2023-11-26T21:19:49Z

@angelosilvestre can you take the last step for this PR?

angelosilvestre · 2023-11-26T22:45:19Z

@matthew-carroll Opened Flutter-Bounty-Hunters/testing#1 to add the suffixes directly on the testWidgetsOnXXXXX and removed from the editor tests.

matthew-carroll · 2023-11-26T22:48:43Z

@angelosilvestre I merged your PR in the testing repo

…solves superlistapp#1602)

angelosilvestre · 2023-11-26T22:52:57Z

@matthew-carroll Already removed the suffixes from here.

I think #1593 needs to get merged to fix the tests.

matthew-carroll · 2023-11-26T23:03:50Z

@angelosilvestre did you check all the tests to make sure there aren't any new failures aside from those existing problems?

angelosilvestre · 2023-11-26T23:13:37Z

@matthew-carroll All failing tests are failing due to the compilation error related to the RawKeyEvent -> KeyEvent migration.

…esolves superlistapp#1602) (superlistapp#1604)

…esolves #1602) (#1604)

angelosilvestre requested a review from matthew-carroll November 18, 2023 00:34

matthew-carroll reviewed Nov 21, 2023

View reviewed changes

matthew-carroll approved these changes Nov 23, 2023

View reviewed changes

angelosilvestre added 3 commits November 26, 2023 19:50

[SuperTextField][mobile] Migrate popover toolbar to OverlayPortal (Re…

6418e03

…solves superlistapp#1602)

PR updates

e26a054

Remove test suffixes

e185c0d

angelosilvestre force-pushed the 1602_popover_overlay branch from 897a717 to e185c0d Compare November 26, 2023 22:50

matthew-carroll merged commit ab19396 into superlistapp:main Nov 26, 2023
6 of 11 checks passed

angelosilvestre added a commit to angelosilvestre/super_editor that referenced this pull request Nov 26, 2023

[SuperTextField][mobile] Migrate popover toolbars to OverlayPortal (R…

922ba03

…esolves superlistapp#1602) (superlistapp#1604)

angelosilvestre deleted the 1602_popover_overlay branch November 26, 2023 23:20

angelosilvestre mentioned this pull request Nov 26, 2023

Cherry-pick "[SuperTextField][mobile] Migrate popover toolbars to OverlayPortal (Resolves #1602) (#1604)" to stable #1621

Merged

matthew-carroll pushed a commit that referenced this pull request Nov 26, 2023

[SuperTextField][mobile] Migrate popover toolbars to OverlayPortal (R…

e6cfe5b

…esolves #1602) (#1604)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

angelosilvestre commented Nov 18, 2023

angelosilvestre commented Nov 18, 2023

matthew-carroll Nov 21, 2023

angelosilvestre Nov 22, 2023

matthew-carroll Nov 23, 2023

matthew-carroll Nov 21, 2023

angelosilvestre Nov 22, 2023

matthew-carroll Nov 21, 2023

angelosilvestre Nov 22, 2023

matthew-carroll Nov 23, 2023

angelosilvestre Nov 23, 2023

matthew-carroll commented Nov 21, 2023

angelosilvestre commented Nov 22, 2023

matthew-carroll commented Nov 23, 2023

matthew-carroll left a comment

matthew-carroll commented Nov 26, 2023

angelosilvestre commented Nov 26, 2023

matthew-carroll commented Nov 26, 2023

angelosilvestre commented Nov 26, 2023

matthew-carroll commented Nov 26, 2023

angelosilvestre commented Nov 26, 2023

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

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

Conversation

angelosilvestre commented Nov 18, 2023

angelosilvestre commented Nov 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-carroll commented Nov 21, 2023

angelosilvestre commented Nov 22, 2023

matthew-carroll commented Nov 23, 2023

matthew-carroll left a comment

Choose a reason for hiding this comment

matthew-carroll commented Nov 26, 2023

angelosilvestre commented Nov 26, 2023

matthew-carroll commented Nov 26, 2023

angelosilvestre commented Nov 26, 2023

matthew-carroll commented Nov 26, 2023

angelosilvestre commented Nov 26, 2023