Hide toolbar when selection is out of view by Renzo-Olivares · Pull Request #98152 · flutter/flutter

Renzo-Olivares · 2022-02-09T21:41:20Z

Description

This change makes it so that the text selection toolbar hides when the text selection has moved outside of the view.

Android	Cupertino

Related Issues

Fixes #77889

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.

chunhtai · 2022-02-10T19:53:42Z

packages/flutter/lib/src/widgets/text_selection.dart

    );
    renderObject.selectionStartInViewport.addListener(_updateHandleVisibilities);
    renderObject.selectionEndInViewport.addListener(_updateHandleVisibilities);
+    renderObject.selectionStartInViewport.addListener(_updateToolbarVisibility);


I would merge _updateHandleVisibilities and _updateToolbarVisibility into one callback, and rename the function to be more generic.

chunhtai · 2022-02-10T19:56:48Z

packages/flutter/lib/src/widgets/text_selection.dart

  }

  /// Final cleanup.
  void dispose() {


I think i missed it in previous PR we should also dispose
_effectiveStartHandleVisibility
_effectiveEndHandleVisibility
as well as the new
_effectiveToolbarVisibility
in the dispose method

they should be put after renderObject.selectionStartInViewport.removeListener

chunhtai · 2022-02-10T19:59:11Z

packages/flutter/lib/src/widgets/text_selection.dart

+  @override
+  void didUpdateWidget(_SelectionToolbarOverlay oldWidget) {
+    super.didUpdateWidget(oldWidget);
+    oldWidget.visibility.removeListener(_toolbarVisibilityChanged);


we should check whether visibility change or not before remove/add listener, or simply assert they won't change if this widget would not be reused in the future

True, it's possible that visibility is the same before and after right?

For my understanding, why should we do this?

This method will be called whenever _SelectionToolbarOverlay or its parents rebuilds regardless whether the visibility property changes or not.

and based on the code, the visibility is passed in from the SelectionOverlay, and it should never change.

Thank you that makes sense!

justinmc

Sounds like this is on hold until some refactoring is done in text_selection.dart? Otherwise it looks fine to me besides a few small comments below.

justinmc · 2022-02-11T00:00:40Z

packages/flutter/lib/src/widgets/text_selection.dart

  }
 }

+/// This widget represents a text selection toolbar.


Nit: No triple slash on a private class? Or maybe it doesn't matter.

According to https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#use--for-public-quality-private-documentation if we plan on making this public in the future it is fine (as long as the documentation is quality). I think this would still be flagged if we make it public because the class fields are not documented. I'm okay with either method.

Ah I say keep it triple then. Thanks for looking that up, I never knew exactly what the policy was.

justinmc · 2022-02-11T00:04:01Z

packages/flutter/lib/src/widgets/text_selection.dart

+  @override
+  void didUpdateWidget(_SelectionToolbarOverlay oldWidget) {
+    super.didUpdateWidget(oldWidget);
+    oldWidget.visibility.removeListener(_toolbarVisibilityChanged);


True, it's possible that visibility is the same before and after right?

justinmc · 2022-02-11T00:08:55Z

packages/flutter/lib/src/widgets/text_selection.dart

 }

+/// This widget represents a text selection toolbar.
+class _SelectionToolbarOverlay extends StatefulWidget {


Nit: Is there a better name for this class? Or maybe just describe what it does in the comment above in a bit more detail. I just wouldn't understand the difference between _SelectionToolbarOverlay and TextSelectionOverlay at a glance. And it doesn't seem to have to do with Overlay directly.

I really like the idea of splitting this out into its own class like this by the way, it seems cleaner to me.

_SelectionToolbarPlacement? Something like that, naming is hard...

For this I just followed the naming scheme of _SelectionHandleOverlay since this will be the widget created when we call

_toolbar = OverlayEntry(builder: _buildToolbar); , and _buildToolbar will return _SelectionToolbarOverlay.

to build the toolbar Overlay.

Ah right, that makes sense 👍

justinmc · 2022-02-11T00:12:03Z

packages/flutter/test/widgets/editable_text_test.dart

+    ));
+
+    final EditableTextState state =
+    tester.state<EditableTextState>(find.byType(EditableText));


I think this should be indented.

justinmc · 2022-02-11T00:13:25Z

packages/flutter/test/widgets/editable_text_test.dart

+
+    // On web, we don't show the Flutter toolbar and instead rely on the browser
+    // toolbar. Until we change that, this test should remain skipped.
+  }, skip: kIsWeb); // [intended]


Does this apply in the same way to desktop?

MacOS With Hiding MacOS Without Hiding

The native behavior (MacOS) at least in the notes app, is to disable scrolling entirely while the selection menu is present.

Edit: Tested on Windows 11 in the notepad app and scrolling is also disabled while the selection menu is present. Also tested on Ubuntu 21.10 on the gedit app and can observe the same behavior (scrolling is disabled).

I think for now the hiding behavior would be preferred so the selection menu does not obscure any text. And when the native behavior of disabling scroll is implemented then this won't be an issue as scroll will be disabled.

Sounds good. I'm working on the "context menu anywhere" stuff and I think it will work out that scrolling will be disabled while the menu is up.

…bility-fix

justinmc

LGTM 👍

chunhtai

LGTM, just nit on the dispose order

chunhtai · 2022-02-15T20:27:35Z

packages/flutter/lib/src/widgets/text_selection.dart

+    _effectiveToolbarVisibility.dispose();
+    _effectiveStartHandleVisibility.dispose();
+    _effectiveEndHandleVisibility.dispose();
    _selectionOverlay.dispose();


It is more systematically correct to dispose _selectionOverlay first before cleaning up all the visibility objects since the selection overlay may be using the visibility objects. One patten I follow is the dispose order is usually the reverse of the initState order.

That makes sense. Thank you for the explanation!

…8152)

* Hide toolbar when selection is out of view * properly dispose of toolbar visibility listener * Add test * rename toolbarvisibility * Make visibility for toolbar nullable * Properly dispose of toolbar visibility listener * Merge visibility methods into one * properly dispose of start selection view listener * Add some docs * remove unnecessary null check * more docs * Update dispose order Co-authored-by: Renzo Olivares <[email protected]>

Hide toolbar when selection is out of view

17720e5

flutter-dashboard bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels Feb 9, 2022

Renzo Olivares added 2 commits February 9, 2022 13:42

properly dispose of toolbar visibility listener

9a18938

Add test

8403c58

Renzo-Olivares marked this pull request as ready for review February 10, 2022 00:06

Renzo-Olivares requested review from chunhtai and justinmc February 10, 2022 00:10

rename toolbarvisibility

0cfe45f

chunhtai reviewed Feb 10, 2022

View reviewed changes

justinmc reviewed Feb 11, 2022

View reviewed changes

Renzo-Olivares and others added 11 commits February 11, 2022 13:55

Merge branch 'master' into toolbar-visibility-fix

2853803

Merge branch 'master' of github.com:flutter/flutter into toolbar-visi…

cd71f27

…bility-fix

Make visibility for toolbar nullable

0a5de0d

Merge branch 'master' of github.com:flutter/flutter into toolbar-visi…

0153749

…bility-fix

Properly dispose of toolbar visibility listener

cc7382d

Merge visibility methods into one

1e0d38a

properly dispose of start selection view listener

4877b53

Add some docs

0d3c4d2

remove unnecessary null check

0bc6c15

Merge branch 'master' of github.com:flutter/flutter into toolbar-visi…

fa1ddba

…bility-fix

more docs

9d8e7bc

Renzo-Olivares requested review from chunhtai and justinmc February 14, 2022 20:41

justinmc approved these changes Feb 15, 2022

View reviewed changes

chunhtai approved these changes Feb 15, 2022

View reviewed changes

Update dispose order

4cefa71

Renzo-Olivares merged commit 8386344 into flutter:master Feb 15, 2022

engine-flutter-autoroll mentioned this pull request Feb 15, 2022

Roll Flutter from a454da0220f7 to 838634496214 (1 revision) flutter/packages#846

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 15, 2022

8386344 Hide toolbar when selection is out of view (flutter/flutter#9…

87c332a

…8152)

engine-flutter-autoroll mentioned this pull request Feb 15, 2022

Roll Flutter from e9f83cf4f9df to 838634496214 (6 revisions) flutter/plugins#4862

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 15, 2022

8386344 Hide toolbar when selection is out of view (flutter/flutter#9…

cc12a99

…8152)

Conversation

Renzo-Olivares commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Renzo-Olivares commented Feb 9, 2022 •

edited

Loading

Renzo-Olivares Feb 14, 2022 •

edited

Loading

Renzo-Olivares Feb 14, 2022 •

edited

Loading