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
Fix text selection in SearchAnchor/SearchBar
#137636
Fix text selection in SearchAnchor/SearchBar
#137636
Conversation
16b47c0
to
d54ad44
Compare
SearchAnchor
SearchAnchor/SearchBar
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 looks good to me! Just left some questions:)
padding: effectivePadding, | ||
child: TextField( | ||
autofocus: widget.autoFocus, | ||
onTap: () { |
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.
Can we just do onTap: widget.onTap
?
@@ -831,6 +827,7 @@ class _ViewContentState extends State<_ViewContent> { | |||
top: false, | |||
bottom: false, | |||
child: SearchBar( | |||
autoFocus: true, | |||
constraints: widget.showFullScreenView ? BoxConstraints(minHeight: _SearchViewDefaultsM3.fullScreenBarHeight) : null, | |||
focusNode: _focusNode, |
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 just remove _focusNode
and the dispose part because I think it's unused?
@@ -69,6 +69,13 @@ class _TextFieldSelectionGestureDetectorBuilder extends TextSelectionGestureDete | |||
void onSingleTapUp(TapDragUpDetails details) { | |||
super.onSingleTapUp(details); | |||
_state._requestKeyboard(); | |||
} |
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.
Seems _state.widget.onTap?.call();
is removed from onSingleTapUp
. Just wonder if this is intended?
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 is intended, it has been moved to onUserTap
so there can be some differentiation between the framework onTap
callback and a user provided onTap
callback. This is a non-breaking change because onUserTap
will still be called with onSingleTapUp
in TextSelectionGestureDetector
as it was before this change. Extenders of TextSelectionGestureDetectorBuilder
will be okay because TextSelectionGestureDetector
retains its previous behavior as the default (A user can still choose to put _state.widget.onTap?.call()
in onSingleTapUp
and it will work as expected).
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! Thanks so much for this fix and all the unit tests😲!
…g the focus change as an internal change instead of a focus change as a result of a tab
a235b09
to
399ccfb
Compare
This changes fixes text selection gestures on the search field when using
SearchAnchor
.Before this change text selection gestures did not work due to an
IgnorePointer
in the widget tree.This change:
IgnorePointer
so the underlyingTextField
can receive pointer events.TextField.onTapAlwaysCalled
andTextSelectionGestureDetector.onUserTapAlwaysCalled
, so a user provided on tap callback can be called on consecutive taps. This is so that the user provided on tap callback forSearchAnchor/SearchBar
that was previously only handled byInkWell
will still work if a tap occurs in theTextField
s hit box. TheTextField
s default behavior is maintained outside of the context ofSearchAnchor/SearchBar
.Fixes #128332 and #134965
Pre-launch Checklist
///
).