Update TextSelectionOverlay #142463
Update TextSelectionOverlay #142463
Conversation
github-actions
bot
added
a: text input
| /// Draws a single text selection handle with a bar and a ball. | ||
| class _TextSelectionHandlePainter extends CustomPainter { | ||
| const _TextSelectionHandlePainter(this.color); | ||
| class _CupertinoTextSelectionHandlePainter extends CustomPainter { |
There was a problem hiding this comment.
I changed this name just so that I could uniquely refer to it in a test (there is another _TextSelectionHandlePainter in material/text_selection.dart).
|
|
||
| if (_selectionOverlay != null | ||
| && (widget.contextMenuBuilder != oldWidget.contextMenuBuilder | ||
| || widget.selectionControls != oldWidget.selectionControls |
There was a problem hiding this comment.
extremely trivial nit and feel free to ignore: if the ||s are on the right hand side, rather than the left, things line up slightly more clearly
| || widget.magnifierConfiguration != oldWidget.magnifierConfiguration)) { | ||
| final bool shouldShowToolbar = _selectionOverlay!.toolbarIsVisible; | ||
| final bool shouldShowHandles = _selectionOverlay!.handlesVisible; | ||
| _selectionOverlay!.dispose(); |
There was a problem hiding this comment.
is the dispose necessary? it's omitted on line 4290.
There was a problem hiding this comment.
I played around with this and yes it's necessary, things from the old overlay stick around on the screen if it's not there. It looks like 4290 should have made that assignment only when it was null, which I'll fix.
| SchedulerBinding.instance.addPostFrameCallback((Duration _) { | ||
| if (shouldShowToolbar) { | ||
| _selectionOverlay!.showToolbar(); | ||
| } | ||
| if (shouldShowHandles) { | ||
| _selectionOverlay!.showHandles(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
what does this look like from an animation perspective? will it cause weird effects if someone is trying to, say, animate the magnifierConfiguration?
There was a problem hiding this comment.
I tried it and it does as I feared. I've created an issue to track this: #142763
Screen.Recording.2024-02-01.at.4.21.51.PM.mov
Manual roll Flutter from e02e207 to 0b5cd50 (46 revisions) Manual roll requested by [email protected] flutter/flutter@e02e207...0b5cd50 2024-02-05 [email protected] fix AppBar docs for backgroundColor & foregroundColor (flutter/flutter#142430) 2024-02-04 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Update gradle lockfiles template" (flutter/flutter#142889) 2024-02-04 [email protected] Update gradle lockfiles template (flutter/flutter#140115) 2024-02-04 [email protected] Roll Flutter Engine from 20742e37e54e to f34c658b9600 (1 revision) (flutter/flutter#142876) 2024-02-03 [email protected] Roll Flutter Engine from 23763db72272 to 20742e37e54e (1 revision) (flutter/flutter#142850) 2024-02-03 [email protected] Roll Flutter Engine from fee02145da8c to 23763db72272 (3 revisions) (flutter/flutter#142848) 2024-02-03 [email protected] Roll Flutter Engine from 9869d47a2736 to fee02145da8c (2 revisions) (flutter/flutter#142847) 2024-02-03 [email protected] Roll Flutter Engine from 78c63d3c2c68 to 9869d47a2736 (1 revision) (flutter/flutter#142842) 2024-02-02 [email protected] Roll Flutter Engine from 266d5d0b5588 to 78c63d3c2c68 (1 revision) (flutter/flutter#142836) 2024-02-02 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.23.2 to 3.24.0 (flutter/flutter#142839) 2024-02-02 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 3.1.6 to 4.0.1 (flutter/flutter#142838) 2024-02-02 [email protected] Update TextSelectionOverlay (flutter/flutter#142463) 2024-02-02 [email protected] Roll Flutter Engine from e29263212bfd to 266d5d0b5588 (5 revisions) (flutter/flutter#142832) 2024-02-02 [email protected] Fix CupertinoTextSelectionToolbar clipping (flutter/flutter#138195) 2024-02-02 [email protected] Reland "Add support for Gradle Kotlin DSL (#140744)" (flutter/flutter#142752) 2024-02-02 [email protected] Support navigation during a Cupertino back gesture (flutter/flutter#142248) 2024-02-02 [email protected] Avoid depending on files from build_system/targets other than from top level entrypoints in flutter_tools. (flutter/flutter#142760) 2024-02-02 [email protected] Roll Packages from 5b48c44 to d37fb0a (14 revisions) (flutter/flutter#142812) 2024-02-02 [email protected] Add a link the different possible Android virtual device configs (flutter/flutter#142765) 2024-02-02 [email protected] Allow all iOS tests to use either iOS 16 or 17 (flutter/flutter#142714) 2024-02-02 [email protected] Roll Flutter Engine from b35153d00b2e to e29263212bfd (2 revisions) (flutter/flutter#142799) 2024-02-02 [email protected] Roll Flutter Engine from dd4c79a6c864 to b35153d00b2e (10 revisions) (flutter/flutter#142783) 2024-02-02 [email protected] Wasm/JS Dual Compile with the flutter tool (flutter/flutter#141396) 2024-02-02 [email protected] Reland: Added ButtonStyle.foregroundBuilder and ButtonStyle.backgroundBuilder (flutter/flutter#142762) 2024-02-01 [email protected] Use proto name for emulator version and show cipd package version (flutter/flutter#142262) 2024-02-01 [email protected] [github actions] ping actor of workflow on cherry pick pr creation (flutter/flutter#142676) 2024-02-01 [email protected] Marks Linux_android_emu android views to be unflaky (flutter/flutter#142590) 2024-02-01 [email protected] Implement `switch` expressions in `lib/src/material/` (flutter/flutter#142634) 2024-02-01 [email protected] Roll Flutter Engine from 9beb7e82e081 to dd4c79a6c864 (1 revision) (flutter/flutter#142749) 2024-02-01 [email protected] Write Tests for API Example of `form.0.dart` (flutter/flutter#142635) 2024-02-01 [email protected] Make leak_tracking bots sticked to the left even if bot thinks they are non-flacky. (flutter/flutter#142744) 2024-02-01 [email protected] Upload DerivedData logs in CI (flutter/flutter#142643) 2024-02-01 [email protected] Test codesigning xcframeworks in artifacts (flutter/flutter#142666) 2024-02-01 [email protected] Fix gen_defaults test randomness (flutter/flutter#142743) 2024-02-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Added ButtonStyle.foregroundBuilder and ButtonStyle.backgroundBuilder" (flutter/flutter#142748) 2024-02-01 [email protected] Roll Flutter Engine from 39415c3eed42 to 9beb7e82e081 (5 revisions) (flutter/flutter#142745) 2024-02-01 [email protected] Remove unused deprecated autoroll mirror-remote flag (flutter/flutter#142738) 2024-02-01 [email protected] Fix leaks in tests. (flutter/flutter#142677) 2024-02-01 [email protected] Roll Flutter Engine from 8c43332c6ffc to 39415c3eed42 (1 revision) (flutter/flutter#142740) 2024-02-01 [email protected] Remove verbose-system-logs on iOS perf tests (flutter/flutter#142739) 2024-02-01 [email protected] Remove outdated arm64_armv7 check (flutter/flutter#142737) 2024-02-01 [email protected] fix CupertinoTabView's Android back button handling with PopScope (flutter/flutter#141604) 2024-02-01 [email protected] Roll Flutter Engine from 68943afd62d1 to 8c43332c6ffc (8 revisions) (flutter/flutter#142726) 2024-02-01 [email protected] Unpin test (flutter/flutter#141427) ...
* master: (45 commits) Reverts "Update gradle lockfiles template" (flutter#142889) Update gradle lockfiles template (flutter#140115) Roll Flutter Engine from 20742e37e54e to f34c658b9600 (1 revision) (flutter#142876) Roll Flutter Engine from 23763db72272 to 20742e37e54e (1 revision) (flutter#142850) Roll Flutter Engine from fee02145da8c to 23763db72272 (3 revisions) (flutter#142848) Roll Flutter Engine from 9869d47a2736 to fee02145da8c (2 revisions) (flutter#142847) Roll Flutter Engine from 78c63d3c2c68 to 9869d47a2736 (1 revision) (flutter#142842) Roll Flutter Engine from 266d5d0b5588 to 78c63d3c2c68 (1 revision) (flutter#142836) Bump github/codeql-action from 3.23.2 to 3.24.0 (flutter#142839) Bump codecov/codecov-action from 3.1.6 to 4.0.1 (flutter#142838) Update TextSelectionOverlay (flutter#142463) Roll Flutter Engine from e29263212bfd to 266d5d0b5588 (5 revisions) (flutter#142832) Fix CupertinoTextSelectionToolbar clipping (flutter#138195) Reland "Add support for Gradle Kotlin DSL (flutter#140744)" (flutter#142752) Support navigation during a Cupertino back gesture (flutter#142248) Avoid depending on files from build_system/targets other than from top level entrypoints in flutter_tools. (flutter#142760) Roll Packages from 5b48c44 to d37fb0a (14 revisions) (flutter#142812) Add a link the different possible Android virtual device configs (flutter#142765) Allow all iOS tests to use either iOS 16 or 17 (flutter#142714) Roll Flutter Engine from b35153d00b2e to e29263212bfd (2 revisions) (flutter#142799) ...
|
So the problem with this approach (I just discovered, when trying it out with blankcanvas) is that if any of the settings do change, the menu disappears. In blankcanvas, the text field is rebuilt in an animation when the mouse enters or exits the text field. This means the context menu builder is recreated every frame. This means I can't bring up the menu -- if I right click the field, the menu is visible for one frame, then is immediately disposed. There are workarounds I could apply in blankcanvas, e.g. caching the context menu builder, but fundamentally I don't think there's any reason the context menu builder itself shouldn't be animated, which right now just wouldn't work. Instead I think we need to have some approach that updates the selection overlay with the new values. |
|
(The code does attempt to avoid this issue but it doesn't seem to be working for some reason... I wonder if it's something to do with the context menu fade in / fade out animations or something...) |
|
Continuing my stream of consciousness: shouldShowToolbar and shouldShowHandles are both false when it happens in my case... |
|
in |
|
Flipping the test in bool get toolbarIsVisible {
return selectionControls is! TextSelectionHandleControls
? _contextMenuController.isShown || _spellCheckToolbarController.isShown
: _toolbar != null || _spellCheckToolbarController.isShown;
}...however "as intended" is no good in this case, because what it does in the blankcanvas case is every time you move the mouse near the text field, the context menu disappears and fades back in! |
|
Ah bummer. Well it should be doable with some refactoring of widgets/text_selection.dart, it just doesn't seem to have been built with these kinds of updates in mind. I think we can track this in #142969. I think the logic flipping you did is not right. TextSelectionHandleControls is used to indicate that the _contextMenuController approach should be used. Sorry, it's my weird 2 step deprecation striking again. |
|
Yeah I think the bug is that selectionControls can be null and this test doesn't check for that. |
Previously, dragging to select with an Apple Pencil on an iPad (Scribble) caused the context menu to rapidly hide and show. Sometimes this even caused an assertion error when using SystemContextMenu due to showing two context menus in one frame. After this PR, the flicker and crash are gone. The flicker happened on both the Flutter-rendered context menu and SystemContextMenu, but the error only happened with SystemContextMenu due to a safeguard that prevents two from showing at the same time. The flickering is likely a regression caused by #142463. | Before this PR | After this PR | | --- | --- | | <video src="https://github.com/user-attachments/assets/e35f36f5-350d-41fb-b878-ee7b7820699d" /> | <video src="https://github.com/user-attachments/assets/262cb8d3-6670-4765-ace8-2d9bf61ae112" /> | Flutter's behavior isn't perfect compared to native (below), but it's a major improvement. If we want to match native, I think we might have to mess with the engine and see why it's calling showToolbar so much. I checked and scribbleInProgress is false during this selection gesture, so we can't use that. <details> <summary>Scribble native video</summary> https://github.com/user-attachments/assets/207e208a-ac36-4c9e-a8ed-9e90e6ef9e3a </details> Fixes #159259
2 participants
TextSelectionOverlay wasn't getting updated when parameters that affect it changed in EditableText. This PR recreates the class when those parameters change.
Fixes #142077