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.

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

Use of FocusNode is creating issue for other controls. #33

Closed
sachinchavanin opened this issue Feb 8, 2022 · 17 comments
Closed

Use of FocusNode is creating issue for other controls. #33

sachinchavanin opened this issue Feb 8, 2022 · 17 comments
Assignees
Labels
bug Something isn't working fixed
Projects
Milestone

Comments

@sachinchavanin
Copy link

If TextFormField or TextField is used while using ColorPicker not able to use keyboard at all.

On clicking on TextFormField soft keyboard appears and instantly disappears.
If ColorPicker removed above issue is not coming.
Tested on Android.

@rydmike
Copy link
Owner

rydmike commented Feb 9, 2022

Hi @sachinchavanin, thanks for the issue report.
Sounds possible. Due to limitations in older version of Flutter framework I had to do some whacky FocusNode handling in the implementation.

I have not seen the issue though, but then again I have always used the picker control in a dialog with only its own controls present on Android and only in desktop views on the main surface, where you do not have a virtual keyboard (normally).

Do you have a ready made minimum code sample to reproduce the issue? If not, I can try to make one using one of the bundled examples. But I might need some more input if I'm not able to reproduce it.

@rydmike rydmike self-assigned this Feb 9, 2022
@rydmike rydmike added this to To do in TODOs via automation Feb 9, 2022
@rydmike rydmike moved this from To do to In progress in TODOs Feb 9, 2022
@sachinchavanin
Copy link
Author

Sorry for late reply. Will try to create some sample and share.

One more thing, this happens mainly when MediaQuery is used e.g. MediaQuery.of(context).size.
So when keyboard appears screen is resized and due to use of MediaQuery UI rebuilt happens then ColorPicker somehow cancels keyboard.

@sachinchavanin
Copy link
Author

Hi, Any progress?

@rydmike
Copy link
Owner

rydmike commented Mar 4, 2022

Sorry, I was under the impression from your last post that you were going to provide a small sample that recreates the issue.
😄

That would actually be really helpful, since creating a sample that recreates a specific issue can be a bit of hit and miss since I don't know the exact details of the use case/setup where the issue occurs.

Also with a sample that recreates the issue, I can properly verify that any applied fix work correctly for the same sample where you showed the issue.

@sachinchavanin
Copy link
Author

Sorry, I had asked for the repro sample from the team member working on this. But he used flutter_colorpicker and moved on.

Will try to create myself and share.

@rydmike
Copy link
Owner

rydmike commented Mar 24, 2022

Hi @sachinchavanin,
I know you moved one, but I would still like to understand the usage scenario that caused your issue.

Could you at least explain in what type of usage scenario you encountered the problem?
Was this on a surface where the color picker is in a custom dialog with an extra TextField in the dialog?
Or when not using it in a dialog, but on main "screen" with many other controls, including a TextField?

Some thoughts on the matter

I can actually see how both of these use cases might cause a problem with focus handling and virtual keyboards.

The color picker sets and "steals" focus when you activate it, it does so in order to get focus so it can react to keyboard shortcut events for copy/paste, like CTRL-C/V and CMD-C/V. This allows it to when it is opened in its own dialog react to a copy/paste keyboard event without the user first being forced to click/interact with some widget in the picker to activate the copy/paste event listener. I just liked this convenience of being able to copy/past color values in/out of the picker quickly with CTRL-C/V without clicking on any control in it.

This does have the impact it will still focus from fields in the same focus scope, and close their virtual keyboard. Normally when used in its own dialog, this is OK, since there is only its own color code entry field in the dialog's focus scope, and it is activated when you click on it and keyboard appears correctly then. And when in a dialog it would close the virtual keyboard in the scope below it anyway, so that there is room enough to show the picker.

If you are using the picker in a desktop web app or native desktop app, there is no virtual keyboard, so it does not matter that it tries to snag focus there. It does not always manage it on desktop apps, so copy/paste keyboard shortcuts might still not work anyway until you focus on one of its controls so the picker has focus and can listen to keyboard events it tries to handle.

On a phone you would probably not have the picker on the main surface, it is usually too big, so it would be in a dialog, with maybe some extra TextField. I think in this case it should be possible to set the focus to the TextField, if it is built after the picker. Same should apply when it is used on a main surface in a tablet.

In any case I would need to setup a test case to better study those use cases, since neither is one that I have tried and used the picker in myself. It should of course be possible to make it play nice for those cases too.

A relatively easy thing to do would be to add yet another configuration option, something like a "doNotAutoFocus` picker when it is created. Then one can choose if it is desired or not for more convenient copy/paste keyboard event handling to have this behavior.

Maybe I could plug in the keyboard events at some lower level where I don't need focus to be able to listen to copy/paste events, might be possible at lower level in Flutter SDK. Also I should check that it does not autofocus if you have disabled keyboard copy/paste shortcut feature. Because as I recall there should be no need for it then at all.

I do appreciate the feedback, and I suspect the issues you encountered have to do with the things I described above, but without more exact info it is just a guess, but probably a correct one 😃

@sachinchavanin
Copy link
Author

I tried to recreate by creating new project, but I am not able to. Tried everything used in original project, but still issue is not recreated. Give me some more time. Attached gif of issue. If I remove flex_color_picker issue stops. Also I am using consumer from riverpod as root component, this is just for info, I dont know if it is also a factor.
flex_color_picker

@rydmike
Copy link
Owner

rydmike commented Mar 28, 2022

OK, I do see the picker in the same plane on main surface in this design, so yes I think the focus snag it does to support "unfocused" keyboard shortcut copy/paste, intended for dialog usage mostly, that has its own focus scope, is causing the issue.

I will:

  • See to it that it is not present if you do not use keyboard the shortcut copy/paste features.
  • Also offer a flag to turn it off completely, keyboard shortcuts to copy/paste wont work then, until you focus on the widget, so they would still actually work, but not just as "laziliy" as now.

Love the above design btw, very nice looking! 💙

@sachinchavanin
Copy link
Author

That would be great. Thanks.

@rydmike rydmike added the bug Something isn't working label Apr 6, 2022
@rydmike rydmike moved this from In progress to Done in TODOs Apr 15, 2022
@rydmike
Copy link
Owner

rydmike commented Apr 15, 2022

Should be fixed via #46
will be released shortly. The web demo also contains an example of using it, also being built shortly.
Since I don't have access to any code with your actual use case it is difficult for me to know if this actually helps with your case. Please try and let me know.

@rydmike
Copy link
Owner

rydmike commented Apr 15, 2022

At least it works on web, if no keyboard shortcuts are enabled OR even if they are but autofocus is set to false, a field that has or grabs focus will keep/get focused, even if using picker in same scope. Tested on tablet with this demo too and yes the virtual keyboard opens and stays open too.

image

@rydmike
Copy link
Owner

rydmike commented May 6, 2022

@sachinchavanin as mentioned I think this issue is solved now. Since I have not heard back from you and it was solved some time ago, I am going to close this issue now. If you still have problems related to it, let me know and we can revisit it.

@rydmike rydmike closed this as completed May 6, 2022
@rydmike rydmike moved this from Done to Released on pub.dev in TODOs Jul 18, 2022
@jpozo20
Copy link

jpozo20 commented Aug 23, 2022

@rydmike
I'll try to see later if I can isolate the issue, but I'm currently facing the same issue with the color picker and the keyboard.
As far as I can see, in my case, it only happens when I have a modal BottomSheet and in there I have a text field along the color picker. If it's on a material page it doesn't happen.

After investigating for a while before writing this comment, I found that it only happens when the initial color is changed. Somehow it's triggering a rebuild and reselecting the initial color and I guess that's the issue.
I attach a gif of the issue in an Android emulator.

flex_color_issue

@rydmike
Copy link
Owner

rydmike commented Aug 24, 2022

Is this in an issue reproduction sample repo I could clone to study it closer? I could throw together something similar, but if you have a sample repo I can use ready, that would be faster. Thanks 🙂

@jpozo20
Copy link

jpozo20 commented Aug 24, 2022

Is this in an issue reproduction sample repo I could clone to study it closer? I could throw together something similar, but if you have a sample repo I can use ready, that would be faster. Thanks 🙂

For now I changed the sheet to use the picker dialog instead of showing the picker and it works correctly, so I think there is something going on with the BottomSheet.
I attach a zip with a simple app reproducing the issue. It has a screen with a button opening the bottom sheet and another one pushing a page. You will see the picker works correctly when inside the page but the issue appears when inside the bottom sheet.

flex_color_issue.zip

@rydmike
Copy link
Owner

rydmike commented Aug 24, 2022

Hi Jefry @jpozo20,

Thanks for the example. Please try this slightly modified BottomSheetModal.

Your issue is not related to the discussed autofocus in this issue. I did however add an example of what it was about too below. Just in case it is of interest, since you do have a use case where it would apply if you were trying to set focus, when modal dialog or bottom sheets opens, to the text field and keep it there until user clicks outside it.

As I see it, your issue was that you are trying to mutate the input prop to the StatefulWidget.

As stated mentioned here: https://api.flutter.dev/flutter/widgets/StatefulWidget-class.html
"StatefulWidget instances themselves are immutable and store their mutable state in separate State objects that are created by the createState method"

Long story short, the mutation you were trying to do is not correct usage of state in a StatefulWidget, hence the erratic behavior in the bottom sheet.

This versions corrects that, and does not have the issue you demonstrated. Hopefully this helps. 😄

// ignore_for_file: public_member_api_docs, sort_constructors_first
import 'dart:developer';

import 'package:flex_color_picker/flex_color_picker.dart';
import 'package:flutter/material.dart';

// Make the widget prop "selectedColor" final, and widget const.
class BottomSheetModal extends StatefulWidget {
  final Color? selectedColor;
  const BottomSheetModal({Key? key, this.selectedColor}) : super(key: key);

  @override
  State<StatefulWidget> createState() => _BottomSheetState();
}

class _BottomSheetState extends State<BottomSheetModal> {
  // Introduce a state variable for the mutable color.
  late Color selectedColor;

  @override
  void initState() {
    super.initState();
    // Init the mutable prop to widget prop.
    selectedColor = widget.selectedColor ?? Colors.blue;
  }

  // If parent has a way of updating widget.selectedColor while this widget is
  // in the tree, add this. ColorPicker itself has this feature for its
  // color prop, but in your case I don't think you need it, since it
  // won't get modified after modal screen or bottom sheet has been
  // opened, so initState is enough. However, if you ever need it,
  // then add this, keeping it out here now.
  //
  // @override
  // void didUpdateWidget(covariant BottomSheetModal oldWidget) {
  //   super.didUpdateWidget(oldWidget);
  //   if (widget.selectedColor != oldWidget.selectedColor)  {
  //     selectedColor = widget.selectedColor ?? Colors.blue;
  //   }
  // }

  @override
  Widget build(BuildContext context) {
    final headerRow = Row(
      children: [
        const Expanded(
          child: Text('Add new tag',
              style: TextStyle(
                fontSize: 18,
                fontWeight: FontWeight.bold,
              )),
        ),
        TextButton(
          onPressed: () => Navigator.pop(context),
          child: const Text('Cancel',
              style: TextStyle(
                color: Colors.redAccent,
                fontSize: 18,
              )),
        ),
        const TextButton(
          onPressed: null,
          child: Text('Save', style: TextStyle(fontSize: 18)),
        )
      ],
    );

    return Container(
      padding: const EdgeInsets.only(top: 15, left: 15, right: 15, bottom: 30),
      child: Column(
        mainAxisSize: MainAxisSize.min,
        children: [
          headerRow,
          const Padding(
            padding: EdgeInsets.zero,
            child: Divider(
              thickness: 4,
            ),
          ),
          Padding(
            padding: const EdgeInsets.all(4.0),
            child: Column(
              mainAxisSize: MainAxisSize.max,
              children: [
                Row(
                  children: const [
                    Expanded(
                      child: TextField(
                        // Just for info, this autofocus (that I added for
                        // demo purposes) will not have any
                        // affect unless you set ColorPicker to not focus,
                        // further below by uncommenting, the
                        // copyPasteBehavior autofocus further below. I think
                        // you probably don't want that in this case though,
                        // but just as info in case you need/want it later.
                        autofocus: true,
                        decoration: InputDecoration(
                            label: Text('Tag name'),
                            hintText: 'Enter tag name',
                            border: InputBorder.none,
                            contentPadding: EdgeInsets.symmetric(
                              horizontal: 4,
                              vertical: 8,
                            )),
                      ),
                    )
                  ],
                ),
                SizedBox(
                  height: 400,
                  child: ColorPicker(
                    borderRadius: 22,
                    heading: const Text('Select tag color'),
                    subheading: const Text('Select color shade'),
                    color: selectedColor,
                    // If you have set focus to TextField and want to keep it
                    // there when Dialog or BottomSheet opens, set this:
                    //
                    // copyPasteBehavior:
                    //  const ColorPickerCopyPasteBehavior(autoFocus: false),
                    pickersEnabled: const <ColorPickerType, bool>{
                      ColorPickerType.both: true,
                      ColorPickerType.primary: false,
                      ColorPickerType.accent: false,
                      ColorPickerType.bw: false,
                      ColorPickerType.wheel: false,
                    },
                    onColorChanged: (color) {
                      log('SelectedColorChanged: $color');
                      // Now we are only mutating the the mutable
                      // state variable and not the parent widget
                      // that is actually immutable and was causing erratic
                      // behavior and rebuilds when you mutated its property.
                      // This now works in both bottom sheet and modal screen.
                      setState(() {
                        selectedColor = color;
                      });
                    },
                  ),
                ),
              ],
            ),
          )
        ],
      ),
    );
  }
}

@jpozo20
Copy link

jpozo20 commented Aug 25, 2022

@rydmike thank you for the detailed example. I was looking at the code of the ColorPicker while debugging, and I'm amazed at how well documented and easy to understand the code is.

Still, I found that the issue occurs in these two situations.

  1. If I don't set the color property of the picker nor update the state in the onColorChanged event, the issue occurs
  2. If I assign the selectedColor of the state to the color property and don't update the state in the onColorChanged event, the issue occurs

As you said, if the color is assigned and updated on the change event it works correctly. So in the end I think the main culprit is that picker is inside a StatefulWidget and it expects the color to be updated.
Honestly, I'm new to Flutter so that was a mistake on my part about the immutable StatefulWidget and I thank you for taking your time to look at it.

@rydmike rydmike added this to the 2.4.0 milestone Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed
Projects
Status: Released
TODOs
Released on pub.dev
Development

No branches or pull requests

3 participants