Change semantics of shouldRequestFocus

[grundid]

Description

I would like to pick up on the discussion in flutter-form-builder-ecosystem/flutter_form_builder#908 and suggest a change to the semantics of shouldRequestFocus. In the past we have seen many “A FocusNode was used after being disposed“ exceptions but I thing that the current fix with PR flutter-form-builder-ecosystem/flutter_form_builder#1045 will solve this for good.

In PR flutter-form-builder-ecosystem/flutter_form_builder#856 in the requestFocus() method Scrollable.ensureVisible(context) was added and this introduced new issues. In larger lists with checkboxes the content was jumping back and forth with each check of a checkbox. The semantics of Scrollable.ensureVisible are not clearly defined. As we all experienced this method can also move a context that is already visible and this is very distracting to the user. Also if a user is already interacting with a form field we can assume that it is visible. There are several open issues with this ensureVisible behavior, for example flutter-form-builder-ecosystem/form_builder_extra_fields#10. Here the user is complaining that the dropdown moves to the top because “focus on the form behave strange”. But it is not requestFocus that is causing the problem. It is ensureVisible.

For both issues the solution with shouldRequestFocus (=false) seems to fix all problems as it disables both requestFocus and ensureVisible. At the same time it disables the focus of a form field which is a very important part of every form. With focus disabled it is not possible to navigate through a form with a keyboard and if no focus is given to a checkbox or another form field it cannot hide a soft keyboard if a user was in a text field before that.

If I understand the idea behind PR flutter-form-builder-ecosystem/flutter_form_builder#856 it was so a validation error is visible to a user. I think this is a good point but we should ensure that a validation error is visible only after calling validate(). During auto validation the user is already entering data into a form field so that we can assume that the field and the error message are visible. For this it might be helpful to extend the validate() method with an optional parameter "bool ensureValidationErrorVisible" that would if true call on the first form field with a validation error Scrollable.ensureVisible.

So my suggestion would be:

1. shouldRequestFocus should be true by default
2. requestFocus should not call Scrollable.ensureVisible
3. Scrollable.ensureVisible might be called during validate if an optional parameter is set to true

What do you think?

[WilliamCunhaCardoso]

Thanks for your analysis and your effort on contributing for the package!

About the focus, I believe it should be segregated in subjects. For example, something like focusOnChange, focusOnError, focusOnEditComplete and so on. So, on this way, I believe we can give appropriate focus manage. But this is only an idea.

[WilliamCunhaCardoso]

@deandreamatias @danvick What do you guys think about it?

[deandreamatias]

Thanks for your analysis and your effort on contributing for the package!

About the focus, I believe it should be segregated in subjects. For example, something like focusOnChange, focusOnError, focusOnEditComplete and so on. So, on this way, I believe we can give appropriate focus manage. But this is only an idea.

I like it this, but I don't know how much effort is required.

One thing that all agree (I think), is that Scrollable.ensureVisible should be called by conditional. Can be focusOnError, from property set in validate or another way. We need focus to find this way

[WilliamCunhaCardoso]

@grundid are you in this package discord server?

[grundid]

No, not yet. I don't have a link.

[WilliamCunhaCardoso]

No, not yet. I don't have a link.

https://discord.gg/Vr7Mg5g9 if you don't mind, you can enter on it.

[deandreamatias]

We will discuss some details here and add conclusions or relevant info to this issue

[grundid]

How do we proceed here? If you look at the latest PR #1062 people are trying to solve the strange behavior of requestFocus/ensureVisible and adding new flags (autoScrollToInvalidField) to compensate for it. Instead of adding this flag one could achieve this by adding the above ensureValidationErrorVisible parameter to the validate method.

I was thinking about a large demo form that we could create to demonstrate the different functions of the form builder.

[Brunobnahorny]

I'm open to help if needed. I just had to fork the project to solve this unexpected field view change caused by Scrollable.ensureVisible. Even using the duration parameter to smooth a bit would be acceptable.

In my case, selecting a checkbox option would scroll the checkbox group to top of the page. I understand that focus should be applied but i don't think Scrollable.ensureVisible should be the default behaviour.

I propose removing Scrollable.ensureVisible and adding another method to call Scrollable.ensureVisible whenever needed as @grundid suggested.

[deandreamatias]

Hi @Brunobnahorny I'm will glad if you can contribute with this. I don't have much time to implement this complex things, but can review and discuss if necesary. Thanks!