-
Notifications
You must be signed in to change notification settings - Fork 230
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
[SuperTextField][iOS] Don't show the mobile toolbar upon first tap (Resolves #2005) #2019
Conversation
@@ -293,6 +294,45 @@ class SuperTextFieldInspector { | |||
"Couldn't find the caret rectangle because we couldn't find a SuperTextField. Finder: $superTextFieldFinder"); | |||
} | |||
|
|||
/// Returns `true` if the mobile text field is configured to display a toolbar. |
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.
Both the name of this method, and the documentation confuses me. I don't know what "is configured to display a toolbar" means, nor do I know what "wantsMobileToolbarToBeVisible" means. Can you please revisit those to help the reader understand what they're querying here?
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 mean that the overlay controller has shouldDisplayToolbar
set to true
. I didn't documented as if this guarantees that the toolbar is actually displayed because we are not checking for any widgets in the tree...
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.
Then should we check for widgets, instead? Is there a reason that you chose to lookup a property variable rather than verify the actual toolbar?
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.
In SuperEditor
, the toolbar builder provides a Key
for the user to attach to the toolbar. With that key we can find the toolbar widget.
In SuperTextField
, there is no provided key that apps can attach to the toolbar. Since the toolbar is fully in control of the app we don't have a easy way to query the toolbar widget.
A possible solution would be to wrap the toolbar with another widget that we could easily find...
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.
Since the toolbar is fully in control of the app we don't have a easy way to query the toolbar widget.
Can you clarify what that means?
We're talking about tests, right? Our tests decide what to build for the toolbar, and our tests should be able to find whatever toolbar they create, right?
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.
I changed to override the toolbar builders, so we can check if the toolbar widget is present. Unfortunately, we cannot configure this at SuperTextField
level, so I had to create SuperAndroidTextField
and SuperIosTextField
directly.
super_editor/test/super_textfield/super_textfield_inspector.dart
Outdated
Show resolved
Hide resolved
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.
LGTM
[SuperTextField][iOS] Don't show the mobile toolbar upon first tap. Resolves #2005
On iOS, tapping to focus the textfield is showing the toolbar. Looking at some iOS app, it seems the toolbar should only be displayed on tap if the textfield already has focus.
This PR changes the iOS textfield to only show the toolbar on tap if it already has focus.
For Android, it seems that doesn't show the toolbar even if the textfield already has focus.