-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Fix selection not deselected when TextField loses focus #103424
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 selection not deselected when TextField loses focus #103424
Conversation
…no selection is painted when no selectionColor is passed
chunhtai
left a comment
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
| }); | ||
|
|
||
| testWidgets('use DefaultSelectionStyle for selection color', (WidgetTester tester) async { | ||
| testWidgets('selection color can be null with DefaultSelectionStyle as parent', (WidgetTester tester) async { |
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.
Maybe rename to EditableText does not derive selection color from DefaultSelectionStyle
justinmc
left a comment
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 👍
Just as a thought, what if we passed a transparent Color as selectionColor instead of null?
| }, | ||
| ); | ||
|
|
||
| testWidgets('Text field drops selection color when losing focus', (WidgetTester tester) async { |
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.
Nit: Is it relevant to say "Regression test for #103341" here and elsewhere?
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 👍
Just as a thought, what if we passed a transparent Color as selectionColor instead of null?
That works too. Though if someone explicitly passes null as selectionColor to EditableText. I think they would expect no selection to be painted, where as now it defaults to the color defined by DefaultSelectionStyle.of(context).
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 don't have a strong opinion on this, but if I were to redesign this again, passing null as color means disabling the selection seems to be a weird API, In that case I would probably throw if the color is 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.
I agree, though another small difference between null and Colors.transparent is that the TextHighlightPainter skips painting all together when the selectionColor is null vs when it is Colors.transparent it will still paint some values.
| if (range == null || color == null || range.isCollapsed) { |
Description
The original issue was introduced by #101954 because the
selectionColoronEditableTextwas never null. Previously null was passed toEditableTextto paint no selection when it did not have focus.flutter/packages/flutter/lib/src/material/text_field.dart
Line 1244 in 5464c5b
EditableTextto be null again by only takingwidget.selectionColorand not inheritingDefaultSelectionStyle.of(context).Related Issues
Fixes #103341
Tests
Added tests to ensure selectionColor is dropped when focus is lost on TextField, and add regression test.
Pre-launch Checklist
///).