flutter_form_builder icon indicating copy to clipboard operation
flutter_form_builder copied to clipboard

Unhandled Exception: A FocusNode was used after being disposed.

Open grundid opened this issue 3 years ago • 9 comments

I was facing the above error with several FormBuilder widgets especially those that manage its own FocusNode like FormBuilderCheckbox.

Our standard usage of the FormBuilder is to show a form, let the user enter data, once the use presses the save button we show a progress full screen (removing the FormBuilder from the widget tree) and then popping the route. Once we call Navigator.pop() we see the above exception.

After some debugging I noticed that the FocusManager still kept references of the previously focused checkboxes in its focusedChildren list after the FormBuilder was removed from the widget tree and all FocusNodes were disposed. This was only an issue if the user checked at least one of the checkboxes. Also if the use just hid the back button there was no problem.

After some more research I realized that the FocusNodes where not attached to the context. When the widget tree was rebuild without the FormBuilder the once focused FocusNodes still stayed with the FocusManager.

Here is a small example of the whole issue. In this example if you check the checkbox and then save the form you will get the FocusNode exception. You can see the difference if you disable the processing = true; statement in the setState callback then the error is gone. Please note that live reload will not clear the error. After making any changes to this app you always have to hot reload.

My solution was to attach the FocusNode to the context in the initState method of FormBuilderFieldState class. The dispose method of the FocusNode will automatically detach from the context and remove the focus node from the manager.

  @override
  void initState() {
    super.initState();
    // Register this field when there is a parent FormBuilder
    _formBuilderState = FormBuilder.of(context);
    _formBuilderState?.registerField(widget.name, this);
    // Register a touch handler
    effectiveFocusNode.addListener(_touchedHandler);
    effectiveFocusNode.attach(context);
    // Set the initial value
    setValue(initialValue);
  }

Demo App:

import 'package:flutter/material.dart';
import 'package:flutter_form_builder/flutter_form_builder.dart';
import 'package:form_builder_validators/form_builder_validators.dart';
import 'package:flutter_localizations/flutter_localizations.dart';

void main() {
  runApp(FocusNodeApp());
}

class FocusNodeApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      localizationsDelegates: [
        FormBuilderLocalizations.delegate,
        GlobalMaterialLocalizations.delegate,
        GlobalWidgetsLocalizations.delegate,
        GlobalCupertinoLocalizations.delegate,
      ],
      title: 'Focus Node Testing',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: MainPage(),
      debugShowCheckedModeBanner: false,
    );
  }
}

class MainPage extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text('Focus Node used after disposed App'),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            ElevatedButton(
                onPressed: () {
                  Navigator.of(context).push(MaterialPageRoute(
                    builder: (context) => SimpleEditorWidget(),
                  ));
                },
                child: Text("Go")),
          ],
        ),
      ),
    );
  }
}

class SimpleEditorWidget extends StatefulWidget {
  const SimpleEditorWidget({Key? key}) : super(key: key);

  @override
  _SimpleEditorWidgetState createState() => _SimpleEditorWidgetState();
}

class _SimpleEditorWidgetState extends State<SimpleEditorWidget> {
  final GlobalKey<FormBuilderState> formKey = GlobalKey();
  bool processing = false;
  String title = "Simple Editor";

  @override
  initState() {
    super.initState();
    debugFocusChanges = true;
  }

  _handleSave() async {
    setState(() {
      title = "Saving...";
      processing = true;
    });
    await Future.delayed(Duration(seconds: 1));
    Navigator.pop(context);
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(
          title,
        ),
      ),
      body: SingleChildScrollView(
        child: processing
            ? Center(child: CircularProgressIndicator())
            : FormBuilder(
                key: formKey,
                child: Padding(
                  padding: const EdgeInsets.all(16.0),
                  child: Column(
                    children: [
                      FormBuilderCheckbox(
                          name: "testCheckbox1",
                          initialValue: false,
                          title: Text("Test1")),
                      FormBuilderTextField(
                        name: "name1",
                        decoration: InputDecoration(labelText: "Name1"),
                      ),
                      ButtonBar(
                        children: [
                          ElevatedButton(
                              onPressed: _handleSave, child: Text("Save"))
                        ],
                      ),
                      Text(
                          "Check Test1, click into Name1 field and save will give the error.\nOnce you see the error always use hot restart.")
                    ],
                  ),
                ),
              ),
      ),
    );
  }
}

grundid avatar Nov 12 '21 09:11 grundid

This issue occurs because of #902. Sorry!!! My Bad.

Here in this PR I just disposed of the focusNode without checking from where it came from.

To solve this we need to check if the focusNode is handled by the parent or not, if it is passed from parent widget then we should not be disposed of it, else vice versa.

ashim-kr-saha avatar Nov 14 '21 04:11 ashim-kr-saha

@ashim-k-saha Thanks for looking into it. Disposing of the focusNode from the widget is one problem, but this is not the issue here. The focusNode that is created in the FormBuilderFieldState is not attached to the context. When the FormBuilder is removed from the widget tree the last focused focusNode is still in the FocusManagers' focusedChilderen list. I'm not sure if this is a problem with the Flutter framework. One could definitely build an example that produces this error without the FormBuilder. However for the FormBuilder to work correctly the effectiveFocusNode.attach(context); should be called in initState.

Please have a look at the docs: https://api.flutter.dev/flutter/widgets/FocusNode/attach.html

grundid avatar Nov 14 '21 08:11 grundid

This still an issue after #909 ?

deandreamatias avatar Jun 10 '22 14:06 deandreamatias

Yes, this is still a problem. As described above the attach method has to be called. At some point the "shouldRequestFocus" option was added to the form elements, basically to hide this issue because it is by default "false". For the user this is an issue because the keyboard does not disappear if the user checks a checkbox after being in a text field, because the checkbox does not request focus and thus does not hide the keyboard.

Here is the code from the FormBuilderFieldState class that I'm using in my own fork. It would be great if you could add this to the main repo.


  late FocusNode effectiveFocusNode;
  FocusAttachment? focusAttachment;

  @override
  void initState() {
    super.initState();
    // Register this field when there is a parent FormBuilder
    _formBuilderState = FormBuilder.of(context);
    // Set the initial value
    _formBuilderState?.registerField(widget.name, this);

    effectiveFocusNode = widget.focusNode ?? FocusNode(debugLabel: widget.name);
    // Register a touch handler
    effectiveFocusNode.addListener(_touchedHandler);
    focusAttachment = effectiveFocusNode.attach(context);
  }

  @override
  void didUpdateWidget(covariant FormBuilderField<T> oldWidget) {
    super.didUpdateWidget(oldWidget);
    if (widget.focusNode != oldWidget.focusNode) {
      focusAttachment?.detach();
      effectiveFocusNode.removeListener(_touchedHandler);
      effectiveFocusNode = widget.focusNode ?? FocusNode();
      effectiveFocusNode.addListener(_touchedHandler);
      focusAttachment = effectiveFocusNode.attach(context);
    }
  }

grundid avatar Jun 14 '22 07:06 grundid

ok, I understand. Thank for your replay @grundid You can create a pull request with this changes? Would be very nice to resolve this issue

deandreamatias avatar Jun 14 '22 07:06 deandreamatias

Sure, will do that.

Could we please find a better solution and get rid of the "shouldRequestFocus" option? If the focus is working OK the option is not really needed. Right now I would have to enable this option on each checkbox since I have mixed forms with checkboxes and text fields. Maybe make it true by default or at least as a global option in FormBuilder class so we need to set it only once?

grundid avatar Jun 14 '22 08:06 grundid

The reason to implement shouldRequestFocus is this issue #880. We can't create a global option because not all of form_builder components has property shouldRequestFocus. But, I agree that we can improve the solution.

Maybe we can discuss a solution with @WilliamCunhaCardoso and other people to improve this solution.

PS: I started a discussion to improve this package and that to be a community driven package. If you want, you can contribute to this discussion

deandreamatias avatar Jun 14 '22 08:06 deandreamatias

Oh yeah, issue #880. I fixed this issue like this in my fork because users stared complaining about the list jumping back and forth ;)

  void requestFocus() {
    FocusScope.of(context).requestFocus(effectiveFocusNode);
    //Scrollable.ensureVisible(context);
  }

I think that once a user is able to click on something there is no need to ensure that it is visible. This could be interesting if an option is modified by code but in this case didChange should have an optional parameter to enable visibility of the selected option.

Thank you for the link to the discussion. I'm creating an app that uses more than 40 forms with form builder so I have some ideas how to improve the lib.

grundid avatar Jun 14 '22 08:06 grundid

Yes, this is still a problem. As described above the attach method has to be called. At some point the "shouldRequestFocus" option was added to the form elements, basically to hide this issue because it is by default "false". For the user this is an issue because the keyboard does not disappear if the user checks a checkbox after being in a text field, because the checkbox does not request focus and thus does not hide the keyboard.

Here is the code from the FormBuilderFieldState class that I'm using in my own fork. It would be great if you could add this to the main repo.

  late FocusNode effectiveFocusNode;
  FocusAttachment? focusAttachment;

  @override
  void initState() {
    super.initState();
    // Register this field when there is a parent FormBuilder
    _formBuilderState = FormBuilder.of(context);
    // Set the initial value
    _formBuilderState?.registerField(widget.name, this);

    effectiveFocusNode = widget.focusNode ?? FocusNode(debugLabel: widget.name);
    // Register a touch handler
    effectiveFocusNode.addListener(_touchedHandler);
    focusAttachment = effectiveFocusNode.attach(context);
  }

  @override
  void didUpdateWidget(covariant FormBuilderField<T> oldWidget) {
    super.didUpdateWidget(oldWidget);
    if (widget.focusNode != oldWidget.focusNode) {
      focusAttachment?.detach();
      effectiveFocusNode.removeListener(_touchedHandler);
      effectiveFocusNode = widget.focusNode ?? FocusNode();
      effectiveFocusNode.addListener(_touchedHandler);
      focusAttachment = effectiveFocusNode.attach(context);
    }
  }

The feature shouldRequestFocus was implemented by me. It was a quick fix that I managed to do since I was facing issues in manipulating chips in a scrollable list. Seeing opened issues turn out that many users didn't want a mandatory focus, that's when I came out with such solution. Unfortunately, I missed some forms, so not every one has this option.

The point of focus being optional is that. We had devs that didn't want a mandatory focus on form change.

If another solution is proposed, I am open to it. Just contextualizing here what happened.

@deandreamatias @grundid

WilliamCunhaCardoso avatar Jun 14 '22 12:06 WilliamCunhaCardoso