Skip to content
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.

Sign up for GitHub

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

TextField and TextFormField can use a MaterialStatesController #133977

Merged
merged 20 commits into from
Nov 29, 2023
Merged

TextField and TextFormField can use a MaterialStatesController #133977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
ffda5df
TextField and TextFormField can use MaterialStatesController
Renzo-Olivares Sep 4, 2023
b54c5dc
make private
Renzo-Olivares Sep 4, 2023
b6f078c
update docs
Renzo-Olivares Sep 4, 2023
be2acbb
update docs
Renzo-Olivares Sep 4, 2023
f225a63
do not need to update states controller in TextFormField, TextField w…
Renzo-Olivares Sep 4, 2023
9740e96
address comments
Renzo-Olivares Sep 5, 2023
116aba9
Use internal state controller to manage internal states instead of us…
Renzo-Olivares Sep 5, 2023
e9bc016
fix tests
Renzo-Olivares Sep 5, 2023
c25790d
remove post frame callback
Renzo-Olivares Sep 5, 2023
3ce3219
updates
Renzo-Olivares Sep 5, 2023
18bccf2
readd tests
Renzo-Olivares Sep 5, 2023
b23e3cc
add macro for states controller details
Renzo-Olivares Sep 19, 2023
0fc36f6
update docs
Renzo-Olivares Sep 19, 2023
b53d17f
initialize error state in initStatesControllers
Renzo-Olivares Sep 19, 2023
244269c
remove template
Renzo-Olivares Sep 19, 2023
f9caa8d
fix analyzer
Renzo-Olivares Nov 8, 2023
03d59fb
Address comments
Renzo-Olivares Nov 8, 2023
7e0e4ff
Address comments
Renzo-Olivares Nov 28, 2023
ac59d06
updates
Renzo-Olivares Nov 28, 2023
a6f5170
make analyzer happy
Renzo-Olivares Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 71 additions & 14 deletions packages/flutter/lib/src/material/text_field.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ class TextField extends StatefulWidget {
this.toolbarOptions,
this.showCursor,
this.autofocus = false,
this.statesController,
this.obscuringCharacter = '•',
this.obscureText = false,
this.autocorrect = true,
Expand Down Expand Up @@ -457,6 +458,27 @@ class TextField extends StatefulWidget {
/// {@macro flutter.widgets.editableText.autofocus}
final bool autofocus;

/// Represents the interactive "state" of this widget in terms of a set of
/// [MaterialState]s, including [MaterialState.disabled], [MaterialState.hovered],
/// [MaterialState.error], and [MaterialState.focused].
///
/// Classes based on this one can provide their own
/// [MaterialStatesController] to which they've added listeners.
/// They can also update the controller's [MaterialStatesController.value]
/// however, this may only be done when it's safe to call
/// [State.setState], like in an event handler.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(what's the difference between this one and the existing doc template for this in ink_well.dart?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one just makes sure to point out the states reported by TextField. { MaterialState.disabled, MaterialState.hovered, MaterialState.error, MaterialState.focused }. ink_well.dart points out MaterialState.pressed which TextField does not report at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: consider linking the part of the material state controller doc that explains that this is not the intrinsic states and if you change the value, future intrinsic states change may overwrite the change?(

///
/// The controller's [MaterialStatesController.value] represents the set of
/// states that a widget's visual properties, typically [MaterialStateProperty]
/// values, are resolved against. It is _not_ the intrinsic state of the widget.
/// The widget is responsible for ensuring that the controller's
/// [MaterialStatesController.value] tracks its intrinsic state. For example
/// one cannot request the keyboard focus for a widget by adding [MaterialState.focused]
/// to its controller. When the widget gains the or loses the focus it will
/// [MaterialStatesController.update] its controller's [MaterialStatesController.value]
/// and notify listeners of the change.
final MaterialStatesController? statesController;

/// {@macro flutter.widgets.editableText.obscuringCharacter}
final String obscuringCharacter;

Expand Down Expand Up @@ -970,7 +992,11 @@ class _TextFieldState extends State<TextField> with RestorationMixin implements

int get _currentLength => _effectiveController.value.text.characters.length;

bool get _hasIntrinsicError => widget.maxLength != null && widget.maxLength! > 0 && _effectiveController.value.text.characters.length > widget.maxLength!;
bool get _hasIntrinsicError => widget.maxLength != null &&
widget.maxLength! > 0 &&
(widget.controller == null ?
!restorePending && _effectiveController.value.text.characters.length > widget.maxLength! :
_effectiveController.value.text.characters.length > widget.maxLength!);

bool get _hasError => widget.decoration?.errorText != null || widget.decoration?.error != null || _hasIntrinsicError;

Expand Down Expand Up @@ -1055,6 +1081,7 @@ class _TextFieldState extends State<TextField> with RestorationMixin implements
}
_effectiveFocusNode.canRequestFocus = widget.canRequestFocus && _isEnabled;
_effectiveFocusNode.addListener(_handleFocusChanged);
_initStatesController();
}

bool get _canRequestFocus {
Expand Down Expand Up @@ -1096,6 +1123,20 @@ class _TextFieldState extends State<TextField> with RestorationMixin implements
_showSelectionHandles = !widget.readOnly;
}
}

if (widget.statesController == oldWidget.statesController) {
_statesController.update(MaterialState.disabled, !_isEnabled);
_statesController.update(MaterialState.hovered, _isHovering);
_statesController.update(MaterialState.focused, _effectiveFocusNode.hasFocus);
_statesController.update(MaterialState.error, _hasError);
} else {
oldWidget.statesController?.removeListener(_handleStatesControllerChange);
if (widget.statesController != null) {
_internalStatesController?.dispose();
_internalStatesController = null;
}
_initStatesController();
}
}

@override
Expand Down Expand Up @@ -1128,6 +1169,8 @@ class _TextFieldState extends State<TextField> with RestorationMixin implements
_effectiveFocusNode.removeListener(_handleFocusChanged);
_focusNode?.dispose();
_controller?.dispose();
_statesController.removeListener(_handleStatesControllerChange);
_internalStatesController?.dispose();
super.dispose();
}

Expand Down Expand Up @@ -1172,6 +1215,7 @@ class _TextFieldState extends State<TextField> with RestorationMixin implements
// Rebuild the widget on focus change to show/hide the text selection
// highlight.
});
_statesController.update(MaterialState.focused, _effectiveFocusNode.hasFocus);
}

void _handleSelectionChanged(TextSelection selection, SelectionChangedCause? cause) {
Expand Down Expand Up @@ -1220,7 +1264,29 @@ class _TextFieldState extends State<TextField> with RestorationMixin implements
setState(() {
_isHovering = hovering;
});
_statesController.update(MaterialState.hovered, _isHovering);
}
}

// Material states controller.
MaterialStatesController? _internalStatesController;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually need an internal states controller if the user didn't supply one? The reason we're adding it is that users want to add listeners to it right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point we are currently not using it internally. We do keep track of our _materialState using a private member

flutter/packages/flutter/lib/src/material/text_field.dart

Lines 1215 to 1222 in ff5b5b5

Set<MaterialState> get _materialState {
return <MaterialState>{
if (!_isEnabled) MaterialState.disabled,
if (_isHovering) MaterialState.hovered,
if (_effectiveFocusNode.hasFocus) MaterialState.focused,
if (_hasError) MaterialState.error,
};
}
maybe we should just use the controller to track this in one place c1d4cdf.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this change is to let developers monitor material states changes, but changing the material states from outside of the TextField class shouldn't be allowed, since the source of the truth is TextFieldState I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does makes sense that TextField would be the source of truth for its material state, but I also see the use-case for being able to programmatically update a TextField's material state to add support for more states.

import 'package:flutter/material.dart';

void main() {
  runApp(const MaterialApp(home: Home()));
}

class PressedInputField extends StatefulWidget {
  const PressedInputField({
    super.key,
    this.style,
    required this.pressed,
    required this.onPressed,
    required this.onReleased,
  });

  final bool pressed;
  final TextStyle? style;
  final VoidCallback? onPressed;
  final VoidCallback? onReleased;

  @override
  State<PressedInputField> createState() => _PressedInputFieldState();
}

class _PressedInputFieldState extends State<PressedInputField> {
  late final MaterialStatesController statesController;
  final TextEditingController controller = TextEditingController(text: 'some text');

  @override
  void initState() {
    super.initState();
    statesController = MaterialStatesController(
        <MaterialState>{if (widget.pressed) MaterialState.pressed});
  }

  @override
  void dispose() {
    statesController.dispose();
    controller.dispose();
    super.dispose();
  }

  @override
  void didUpdateWidget(PressedInputField oldWidget) {
    super.didUpdateWidget(oldWidget);
    if (widget.pressed != oldWidget.pressed) {
      statesController.update(MaterialState.pressed, widget.pressed);
    }
  }

  @override
  Widget build(BuildContext context) {
    return Listener(
      behavior: HitTestBehavior.translucent,
      onPointerDown: (PointerDownEvent event) {
        widget.onPressed?.call();
      },
      child: TextField(
        controller: controller,
        statesController: statesController,
        style: widget.style,
        onTap: () {
          widget.onReleased?.call(); 
        },
      ),
    );
  }
}

class Home extends StatefulWidget {
  const Home({super.key});

  @override
  State<Home> createState() => _HomeState();
}

class _HomeState extends State<Home> {
  bool pressed = false;

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: PressedInputField(
          pressed: pressed,
          style: MaterialStateTextStyle.resolveWith((Set<MaterialState> states) {
            if (states.contains(MaterialState.pressed)) {
              return const TextStyle(color: Colors.white, backgroundColor: Colors.indigo);
            }
            return const TextStyle(color: Colors.teal);
          }),
          onPressed: () {
            setState(() {
              pressed = !pressed;
            });
          },
          onReleased: () {
            setState(() {
              pressed = !pressed;
            });
          }
        ),
      ),
    );
  }
}

I'm okay with holding off on that functionality until it is requested, but I also think when a user updates state through the controller using _myController.update(MaterialState.pressed, true) they might expect that the receiving widget reflects the value change, similar to other controllers like TextEditingController and doing _myController.value = myNewValue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when a user updates state through the controller using _myController.update(MaterialState.pressed, true) they might expect that the receiving widget reflects the value change, similar to other controllers like TextEditingController and doing _myController.value = myNewValue.

Do you mean if a TextField has focus but if the user calls update(.focused, false) on the controller then the text field should tell its focus node to unfocus? That sounds pretty strange to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user adds the focused MaterialState after the component loses focus, then the component's visuals will make it appear that the component has the focus.

And if the component regains focus and loses focus again? Should the visual state of the component update as the intrinsic state of the component changes?

  • If the answer is no then it sounds like we need an additional switch that the user can turn on to suppress the intrinsic state changes from affecting the visual state
  • otherwise the controller will be pretty hard to use on a TextField. For instance the app user can switch the focus to a different component, and to obscure the underlying state change I guess you'll have to listen to the state controller and change the material state back? That usually won't happen until the next frame. Average users may not notice that but that can make tests flaky.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you really wanted to prevent the MaterialStatesController from tracking the textfield's intrinsic focus (or whatever) state you could override the controller's update method. That said, this isn't a use case that has ever come up or seems worth designing for.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok makes sense. I guess this should be in the documentation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's not explained well (at all) and some examples are needed. If you create an issue, you can assign it to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the MaterialStatesController docs in #134592


void _handleStatesControllerChange() {
// Force a rebuild to resolve MaterialStateProperty properties.
setState(() { });
}

MaterialStatesController get _statesController => widget.statesController ?? _internalStatesController!;

void _initStatesController() {
if (widget.statesController == null) {
_internalStatesController = MaterialStatesController();
}
_statesController.update(MaterialState.disabled, !_isEnabled);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does it only update .disabled but not the other properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and didUpdateWidget seem like the only reasonable places to listen for changes in _isEnabled, while the other state properties like focus (listens to changes in FocusNode) and hover (listens to MouseRegion callbacks) actively listen for changes. I'll also initialize the error state here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Focus is edge-triggered so you have to set the correct initial value otherwise it won't reflect the correct state until the next time you get notified of a focus state change right? The TextField can be given a FocusNode that may or may not have the focus when initState is called.

_statesController.update(MaterialState.hovered, _isHovering);
_statesController.update(MaterialState.focused, _effectiveFocusNode.hasFocus);
_statesController.update(MaterialState.error, _hasError);
_statesController.addListener(_handleStatesControllerChange);
}

// AutofillClient implementation start.
Expand All @@ -1246,19 +1312,10 @@ class _TextFieldState extends State<TextField> with RestorationMixin implements
}
// AutofillClient implementation end.

Set<MaterialState> get _materialState {
return <MaterialState>{
if (!_isEnabled) MaterialState.disabled,
if (_isHovering) MaterialState.hovered,
if (_effectiveFocusNode.hasFocus) MaterialState.focused,
if (_hasError) MaterialState.error,
};
}

TextStyle _getInputStyleForState(TextStyle style) {
final ThemeData theme = Theme.of(context);
final TextStyle stateStyle = MaterialStateProperty.resolveAs(theme.useMaterial3 ? _m3StateInputStyle(context)! : _m2StateInputStyle(context)!, _materialState);
final TextStyle providedStyle = MaterialStateProperty.resolveAs(style, _materialState);
final TextStyle stateStyle = MaterialStateProperty.resolveAs(theme.useMaterial3 ? _m3StateInputStyle(context)! : _m2StateInputStyle(context)!, _statesController.value);
final TextStyle providedStyle = MaterialStateProperty.resolveAs(style, _statesController.value);
return providedStyle.merge(stateStyle);
}

Expand All @@ -1275,7 +1332,7 @@ class _TextFieldState extends State<TextField> with RestorationMixin implements

final ThemeData theme = Theme.of(context);
final DefaultSelectionStyle selectionStyle = DefaultSelectionStyle.of(context);
final TextStyle? providedStyle = MaterialStateProperty.resolveAs(widget.style, _materialState);
final TextStyle? providedStyle = MaterialStateProperty.resolveAs(widget.style, _statesController.value);
final TextStyle style = _getInputStyleForState(theme.useMaterial3 ? _m3InputStyle(context) : theme.textTheme.titleMedium!).merge(providedStyle);
final Brightness keyboardAppearance = widget.keyboardAppearance ?? theme.brightness;
final TextEditingController controller = _effectiveController;
Expand Down Expand Up @@ -1490,7 +1547,7 @@ class _TextFieldState extends State<TextField> with RestorationMixin implements
}
final MouseCursor effectiveMouseCursor = MaterialStateProperty.resolveAs<MouseCursor>(
widget.mouseCursor ?? MaterialStateMouseCursor.textable,
_materialState,
_statesController.value,
);

final int? semanticsMaxValueLength;
Expand Down
3 changes: 3 additions & 0 deletions packages/flutter/lib/src/material/text_form_field.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import 'package:flutter/widgets.dart';

import 'adaptive_text_selection_toolbar.dart';
import 'input_decorator.dart';
import 'material_state.dart';
import 'text_field.dart';
import 'theme.dart';

Expand Down Expand Up @@ -167,6 +168,7 @@ class TextFormField extends FormField<String> {
ui.BoxWidthStyle selectionWidthStyle = ui.BoxWidthStyle.tight,
DragStartBehavior dragStartBehavior = DragStartBehavior.start,
ContentInsertionConfiguration? contentInsertionConfiguration,
MaterialStatesController? statesController,
Clip clipBehavior = Clip.hardEdge,
bool scribbleEnabled = true,
bool canRequestFocus = true,
Expand Down Expand Up @@ -212,6 +214,7 @@ class TextFormField extends FormField<String> {
textDirection: textDirection,
textCapitalization: textCapitalization,
autofocus: autofocus,
statesController: statesController,
toolbarOptions: toolbarOptions,
readOnly: readOnly,
showCursor: showCursor,
Expand Down
148 changes: 148 additions & 0 deletions packages/flutter/test/material/text_field_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6822,6 +6822,154 @@ void main() {
expect(editableText.style.color, theme.textTheme.bodyLarge!.color!.withOpacity(0.38));
});

testWidgets('Enabled TextField statesController', (WidgetTester tester) async {
final TextEditingController textEditingController = TextEditingController(
text: 'Atwater Peel Sherbrooke Bonaventure',
);
int count = 0;
void valueChanged() {
count += 1;
}
final MaterialStatesController statesController = MaterialStatesController();
statesController.addListener(valueChanged);
await tester.pumpWidget(
MaterialApp(
home: Material(
child: Center(
child: TextField(
statesController: statesController,
controller: textEditingController,
),
),
),
),
);

final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse);
final Offset center = tester.getCenter(find.byType(EditableText).first);
await gesture.moveTo(center);
await tester.pump();
expect(statesController.value, <MaterialState>{MaterialState.hovered});
expect(count, 1);

await gesture.moveTo(Offset.zero);
await tester.pump();
expect(statesController.value, <MaterialState>{});
expect(count, 2);

await gesture.down(center);
await tester.pump();
await gesture.up();
await tester.pump();
expect(statesController.value, <MaterialState>{MaterialState.hovered, MaterialState.focused});
expect(count, 4); // adds hovered and pressed - two changes.

await gesture.moveTo(Offset.zero);
await tester.pump();
expect(statesController.value, <MaterialState>{MaterialState.focused});
expect(count, 5);

await gesture.down(Offset.zero);
await tester.pump();
expect(statesController.value, <MaterialState>{});
expect(count, 6);
await gesture.up();
await tester.pump();

await gesture.down(center);
await tester.pump();
await gesture.up();
await tester.pump();
expect(statesController.value, <MaterialState>{MaterialState.hovered, MaterialState.focused});
expect(count, 8); // adds hovered and pressed - two changes.

// If the text field is rebuilt disabled, then the focused state is
// removed.
await tester.pumpWidget(
MaterialApp(
home: Material(
child: Center(
child: TextField(
statesController: statesController,
controller: textEditingController,
enabled: false,
),
),
),
),
);
await tester.pumpAndSettle();
expect(statesController.value, <MaterialState>{MaterialState.hovered, MaterialState.disabled});
expect(count, 10); // removes focused and adds disabled - two changes.

await gesture.moveTo(Offset.zero);
await tester.pump();
expect(statesController.value, <MaterialState>{MaterialState.disabled});
expect(count, 11);

// If the text field is rebuilt enabled and in an error state, then the error
// state is added.
await tester.pumpWidget(
MaterialApp(
home: Material(
child: Center(
child: TextField(
statesController: statesController,
controller: textEditingController,
decoration: const InputDecoration(
errorText: 'error',
),
),
),
),
),
);
await tester.pumpAndSettle();
expect(statesController.value, <MaterialState>{MaterialState.error});
expect(count, 13); // removes disabled and adds error - two changes.

// If the text field is rebuilt without an error, then the error
// state is removed.
await tester.pumpWidget(
MaterialApp(
home: Material(
child: Center(
child: TextField(
statesController: statesController,
controller: textEditingController,
),
),
),
),
);
await tester.pumpAndSettle();
expect(statesController.value, <MaterialState>{});
expect(count, 14);
});

testWidgets('Disabled TextField statesController', (WidgetTester tester) async {
int count = 0;
void valueChanged() {
count += 1;
}
final MaterialStatesController controller = MaterialStatesController();
controller.addListener(valueChanged);
await tester.pumpWidget(
MaterialApp(
home: Material(
child: Center(
child: TextField(
statesController: controller,
enabled: false,
),
),
),
),
);
expect(controller.value, <MaterialState>{MaterialState.disabled});
expect(count, 1);
});

testWidgetsWithLeakTracking('Provided style correctly resolves for material states', (WidgetTester tester) async {
final TextEditingController controller = _textEditingController(
text: 'Atwater Peel Sherbrooke Bonaventure',
Expand Down