-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Add custom system-wide text selection toolbar buttons on Android #139738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add custom system-wide text selection toolbar buttons on Android #139738
Conversation
9cb842d to
19715e3
Compare
19715e3 to
63cce19
Compare
4e0f662 to
070dffb
Compare
70c51ad to
23b4c5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also not build the system wide buttons if the text selection is invalid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check was inspired by the implementation of accessors such as cutEnabled, copyEnabled, etc. See
flutter/packages/flutter/lib/src/widgets/editable_text.dart
Lines 2244 to 2249 in cdc83e5
| bool get copyEnabled { | |
| if (widget.selectionControls is! TextSelectionHandleControls) { | |
| return widget.toolbarOptions.copy && !widget.obscureText; | |
| } | |
| return !widget.obscureText | |
| && !textEditingValue.selection.isCollapsed; |
Those accessors are called in EditablTestState.contextMenuButtonItems just before the call to _textProcessingActionButtonItems I added. Because this is meant to build the text selection toolbar, it would make sense that a check to TextSelection.isValid is done somewhere before.
I did not spot an obvious place where such a check is done so I have updated the PR to add this check on the line you commented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my understanding what does an app developer expect when the system wide button is used on static text/readOnly? And in what cases would processedText be null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the buttons are expected to do nothing on static text, do they still show up when used on static text despite that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are two types of activities registered to Intent.ACTION_PROCESS_TEXT:
- some will use the selected text (
selectedTextvariable in this code) to open their UI and will display a result (for instance, on my phone I have one activity that will read the text loudly, I also have Google Translate and Deepl that will show the translation using their own panels). Such activities does not return a result (processedTextwill be null in this case). - some will use the selected text to return a modified version (on my phone I have an app called 'Encrypt Text' that does that (it opens an UI where I can ask to see the encrypted text and a button to replace the current selection with this encrypted text). Such activities returns a result (
processedTextwon't be null in this case and we use it to replace the current selection).
I have upated the PR with a comment to explain that is expected that some activities does not return a modified text.
23b4c5b to
4af1bd8
Compare
…electableRegion (#141103) ## Description This PR adds custom system-wide text selection toolbar buttons on Android for `SelectableRegion` and `SelectionArea`. #139738 adds those buttons for `EditableText` (which is used by `TextField` and `SelectableText` but not by `SelectionArea`). ## Related Issue Step 5 for #139361 ## Tests Adds 2 tests.
Description
This PR adds custom system-wide text selection toolbar buttons on Android.
This is a WIP until #139479 is merged (potential conflicts).Related Issue
Fixes #139361
Tests
Adds 5 tests.