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

Web/Desktop Focus should follow the DOM Tree or Widget Tree as a default #55033

Closed
phackwer opened this issue Apr 17, 2020 · 20 comments
Closed
Labels
a: desktop Running on desktop c: new feature Nothing broken; request for a new capability c: proposal A detailed proposal for a change to Flutter f: focus Focus traversal, gaining or losing focus framework flutter/packages/flutter repository. See also f: labels. platform-web Web applications specifically

Comments

@phackwer
Copy link

phackwer commented Apr 17, 2020

Use case

As a Web Developer, we are used to have the Focus of the tab key to follow one of those two things:

1 - DOM tree: deafault behaviour, when you hit TAB the focus moves to next input in the page in the order they were added to the DOM tree, so if I have a div with 3 fields acting as a column, and another div with 4 fields in another column, the tab order would be to go to the first 3 items in the same column and then go to the next 4 in the second column. In a table, it would go to the first cell and then the second cell in the same row and so on. This is a default standard on web inputs behaviour.

2 - Tab Index: this overwrites the standard behaviour on a page and make the tab to follow the order.

At the moment we don't have such approach on Flutter. We have a left to right top to down approach which causes the following problem: on a page that has 2 widgets with inputs as children and a drawer on the right with input itens, the focus goes to the first item of the first column/widget, then the first item of the second column/widget, and then to the first item on the drawer. If we consider that those widgets are groups of information (Personal Data - First Name, Last Name, Email - Organisation Data - Org Name, Org Address - and Page Settings - theme, wallpaper), this means that the navigation ends to be:

First Name -> Org Name -> Page Theme -> Last Name, -> Org Address -> Wallpaper -> Email -> Submit button

This is quite broken from a user perspective. Since those information were grouped as widgets the navigation should be

First Name -> Last Name -> Email -> Org Name -> Org Address -> Submit button (and if you keep hitting tab, Theme and Wallpaper).

Proposal

Tab key should go to the next sibling input into the children list of a widget, and if none is found, go to next widget till it reaches the end of the inputs, and goes back to the first in the page. The Widget structure is already a tree so the Tab Navigation should follow the context of the parent Widget of the Inputs till it reaches the end and needs to go to next widget or cycle back to the start of the tree.

So, full navigation logic for the presented case would be like:
Anybody request focus first? Go to that input (that way a search input on top that is not requesting the focus would not get it as the first input, only when it cycles back to the top of the tree)
PersonalInfoWidget
FirsNameInput gets focus
Tab key pressed in the context of PersonalInfoWidget. Is there a sibling? Goes to it.
LastNameInput gets focus
Tab key pressed in the context of PersonalInfoWidget. Is there a sibling? Goes to it.
EmailInput gets focus
Tab key pressed in the context of PersonalInfoWidget. Is there a sibling? No? Goes to next Widgett o
OrgInfoWidget
OrgNameInput gets focus

And so on until last there is no more siblings, so we go back to the top of the tree.

@phackwer
Copy link
Author

Obs: I'm aware the strategy can be overwritten by me, but I still believe default behaviour should be in there and not to be done by developers (they can overwrite it, but the current default is just not good.

@geriby23
Copy link

geriby23 commented Apr 17, 2020

I agree with @phackwer, currently, I have similar troubles in my project, it's a small thing but it's really important. There should be some elegant solution for tabbing direction in the form.

@phackwer
Copy link
Author

@geriby23 , thanks. :-D So, I'm not alone. 👍

@iapicca iapicca added a: desktop Running on desktop f: focus Focus traversal, gaining or losing focus framework flutter/packages/flutter repository. See also f: labels. platform-web Web applications specifically c: proposal A detailed proposal for a change to Flutter c: new feature Nothing broken; request for a new capability labels Apr 17, 2020
@ferhatb
Copy link
Contributor

ferhatb commented Apr 17, 2020

/cc @gspencergoog

@gspencergoog
Copy link
Contributor

It is possible to change the default for web to be WidgetOrderTraversalPolicy, but it often doesn't turn out to be that good of a default ordering, and it would make your app act inconsistently on other platforms by default.

However, you can try out that ordering yourself by adding this in your app:

Widget build(BuildContext context) {
  return MaterialApp(
    home: FocusTraversalGroup(
      policy: WidgetOrderTraversalPolicy(),
      child: MyApp(),
    ),
  );
}

If you want to specify the traversal order explicitly (as in your tab order case), you can use OrderedTraversalPolicy as shown in the example there (although currently DartPad isn't working with it, but that's unrelated to this issue).

[Note: these APIs are only available on the beta channel or newer at the moment, but soon will be on stable.]

@phackwer
Copy link
Author

Well, I've done something crazy here and it's working (also because we wanted ENTER key behaviour for the fields to be like the html submit)

For every input widget we have, I add a focusNode which calls a method to deal with the keys(enter, tab and shift+tab) and at the same time register this focusNode on what I've called KeyboardNavigationService. Since this registration happens on the exact same order widgets are rendered in the screen, this kind of forces the navigation to respect the grouping I've done to the screen and also respect the responsiveness of my components, so, basically, it pretty much makes the app to behave as a standard HTML form with the DOM tree being used like it would on a normal web app.

This onRawKeyboardEvent method then knowing what key or combination has been pressed will call the method to submit the form or
locator().goNext(_inputListenerFocusNode); or locator().goPrevious(_inputListenerFocusNode); This basically tells my service from where to move forward or back in the array of FocusNodes and from where. Some magic done checking the position of _inputListenerFocusNode to go to 0 or end and.. VOILA! :D Depending on where this _inputListenerFocusNode is in the array I will call _inputListenerFocusNode.previousNode() or _inputListenerFocusNode.nextNode() as not necessarily all components will be registerd in this array.

So far it seems to be working.

And now I know there is also a FocusManager that seems to be another option to do this. I definitely will review and refactor what I've done as I don't want to be maintaining this forever, but for now, this will be what we will use.

A bad thing only is that this KeyboardNavigationService needs to be reset on every page reload. :-/ Otherwise FocusNodes are kept there when not in use. :-/

@phackwer
Copy link
Author

@gspencergoog - I've tried your solution and it did not work as we wanted.

So, here is the "input" I've made at the moment (I will read about the FocusManager as I'm feeling that it may already have what I need) and the KeyboardNavigatinService. This is experimental code, but worked like a charm. I'm usng GetIt to create a service layer for my application, so far I have 3, one of them to bypass the slowliness of reading the SharedPreferences storage - I basically read it all to memory before running the app and now gets and setters return values from memory and only call the write on the set (so, set in memory and then write to slow sharedpref async).

Am I missing something or there is no "services" in flutter to share data between widgets and run other tasks like this keyboard navigation?

@phackwer
Copy link
Author

phackwer commented Apr 18, 2020

Tab Key and Enter aware Input (onEnterKeySubmitDataFunction is mandatory, as we don't do widgets for input as a decorative thing, so, it just made sense for us to have it mandatory)

    import 'package:flutter/cupertino.dart';
    import 'package:flutter/foundation.dart';
    import 'package:flutter/material.dart';
    import 'package:flutter/services.dart';
    import 'package:yoda/app_services.dart';
    import 'package:yoda/common/services/keyboard_navigation_service.dart';
    
    // ignore: must_be_immutable
    class KeyAwareTextFormField extends StatefulWidget {
      String labelText;
      String errorText;
      Function validator;
      Function onChanged;
      Function onEnterKeySubmitDataFunction;
      Icon icon;
      bool obscureText;
      TextInputType keyboardType;
      Border border;
      TextEditingController controller;
    
      KeyAwareTextFormField(
          {this.border,
          this.errorText,
          @required this.onEnterKeySubmitDataFunction,
          @required this.labelText,
          this.onChanged,
          this.controller,
          this.validator,
          this.icon,
          this.keyboardType,
          this.obscureText = false})
          : assert(
              labelText != null,
              'A non-null String must be provided to a Text widget.',
            ),
            assert(
              onEnterKeySubmitDataFunction != null,
              'You MUST inform the function called on the submission of this form so Enter Key acts properly.',
            );
    
      @override
      _KeyAwareTextFormFieldState createState() =>
          _KeyAwareTextFormFieldState(
            border: this.border,
            errorText: this.errorText,
            onEnterKeySubmitDataFunction: this.onEnterKeySubmitDataFunction,
            labelText: this.labelText,
            onChanged: this.onChanged,
            controller: this.controller,
            validator: this.validator,
            icon: this.icon,
            keyboardType: this.keyboardType,
            obscureText: this.obscureText = false,
          );
    }
    
    class _KeyAwareTextFormFieldState
        extends State<KeyAwareTextFormField> {
      String labelText;
      String errorText;
      Function validator;
      Function onChanged;
      Function onEnterKeySubmitDataFunction;
      Icon icon;
      bool obscureText;
      TextInputType keyboardType;
      Border border;
      TextEditingController controller;
    
      FocusNode _inputListenerFocusNode;
    
      _KeyAwareTextFormFieldState(
          {this.border,
          this.errorText,
          @required this.onEnterKeySubmitDataFunction,
          @required this.labelText,
          this.onChanged,
          this.controller,
          this.validator,
          this.icon,
          this.keyboardType,
          this.obscureText = false}) {
        _inputListenerFocusNode =
            FocusNode(onKey: (FocusNode node, RawKeyEvent event) {
          onRawKeyboardEvent(event);
          return true;
        });
        locator<KeyboardNavigationService>().appendToList(_inputListenerFocusNode);
      }
    
      /**
       * Handles key events (enter and tab/shift+tab navigation)
       * When
       */
      onRawKeyboardEvent(RawKeyEvent event) {
        if (event is RawKeyUpEvent) {
          if (event.logicalKey == LogicalKeyboardKey.enter) {
            onEnterKeySubmitDataFunction();
          } else if (event.data is RawKeyEventDataWeb) {
            final data = event.data as RawKeyEventDataWeb;
    
            if (data.keyLabel == "Tab" && !data.isShiftPressed) {
              print('Navigate to next registeres FocusNode');
              locator<KeyboardNavigationService>().goNext(_inputListenerFocusNode);
            }
    
            if (data.keyLabel == "Tab" && data.isShiftPressed) {
              print('Go Back to Previous Node');
              locator<KeyboardNavigationService>()
                  .goPrevious(_inputListenerFocusNode);
            }
    
            if (data.keyLabel == 'Enter') {
              print('Enter key');
              onEnterKeySubmitDataFunction();
            }
          } else if (event.data is RawKeyEventDataAndroid) {
            final data = event.data as RawKeyEventDataAndroid;
    
            if (data.keyCode == 13) {
              onEnterKeySubmitDataFunction();
            }
          }
        }
        return event;
      }
    
      @override
      Widget build(BuildContext context) {
        return TextFormField(
          focusNode: _inputListenerFocusNode,
          textInputAction: TextInputAction.next,
          autofocus: true,
          controller: controller,
          keyboardType: keyboardType,
          onEditingComplete: null,
          decoration: InputDecoration(
              contentPadding: EdgeInsets.all(0),
              labelText: labelText,
              hintStyle: TextStyle(color: Colors.grey[400]),
              icon: icon),
          validator: validator,
          onChanged: onChanged,
          obscureText: obscureText,
          maxLines: 1,
        );
      }
    }

@phackwer
Copy link
Author

phackwer commented Apr 18, 2020

And our KeyboardNavigationService which I still will make RouteAware to subscritbe to RouteObserver and resetList on every route change.

    import 'package:flutter/material.dart';
    
    class KeyboardNavigationService {
      List<FocusNode> _focusNodes = [];
    
      appendToList(FocusNode nodeToAdd) {
        _focusNodes.add(nodeToAdd);
      }
    
      resetList() {
        _focusNodes = [];
      }
    
      goNext(FocusNode currentNode) {
        var nextIndex = 0;
        var currentIndex = _focusNodes.indexOf(currentNode);
    
        currentNode.unfocus();
    
        if (currentIndex == (_focusNodes.length - 1)) {
          /**
           * @TBD - This is to respect the focus tree from Flutter if we reach the end
           * of our list. Maybe we should use just our list?
           */
          currentNode.nextFocus();
        } else if (currentIndex < _focusNodes.length) {
          nextIndex = ++currentIndex;
          _focusNodes[nextIndex].requestFocus();
        }
      }
    
      goPrevious(FocusNode currentNode) {
        var previousIndex = 0;
        var currentIndex = _focusNodes.indexOf(currentNode);
    
        currentNode.unfocus();
    
        if (currentIndex == 0) {
          /**
           * @TBD - This is to respect the focus tree from Flutter if we reach the start
           * of our list. Maybe we should use just our list?
           */
          currentNode.previousFocus();
        } else if (currentIndex > 0) {
          previousIndex = --currentIndex;
          _focusNodes[previousIndex].requestFocus();
        }
      }
    }

@phackwer
Copy link
Author

PS: please, remember, this code was to make sure it would be possible, don't judge it as any final code, this is closer to a POC then to usable code.

@phackwer phackwer reopened this Apr 18, 2020
@phackwer
Copy link
Author

Closed by accident, sorry. I hit the TAB and SPACE and it went directly to CLOSE instead of COMMENT first

@phackwer
Copy link
Author

@gspencergoog OrderedTraversalPolicy seems fine but at the same time too verbosu for the developer. What happens if I have again 2 widgets, one for PersonalInfo and another for OrganizationInfo and 2 different widgets declare the same tab order? We want something implicitly doing the order of tabs according to how inputs (text, radio, check, buttons) are created on code by the developer. If they want something different than they can just not extended form the KeyNavigationAwareWidget we will create here as out base.

@phackwer phackwer reopened this Apr 18, 2020
@phackwer
Copy link
Author

phackwer commented Apr 18, 2020

ARGH!!! I will use CTRL+ENTER from now on as it works better to submit my comments, sorry

@gspencergoog
Copy link
Contributor

Some comments on your code above (I realize it's just a prototype):

  1. you aren't disposing of the focus node, which will cause problems for you in the future.
  2. you are returning "true" from the key handler, which will prevent any keys you don't handle from being handled by something else, like the shortcuts system.

As I stated previously, if you want focus traversal to follow widget creation order, you can also set the overall focus traversal policy to be WidgetOrderTraversalPolicy.

OrderedTraversalPolicy will sort by the explicit order you specify. You can separate sections using FocusTraversalGroup, and they will get sorted separately. If two widgets have the same order in the same traversal group, they will get sorted in the order implied by the value set for secondary on the OrderedTraversalPolicy, which by default is ReadingOrderTraversalPolicy, but you can change it to be WidgetOrderTraversalPolicy if you wish.

If it feels too verbose to specify an order for each widget, you can create your own widget for the controls you use repeatedly, like your KeyAwareTextFormField, that incorporates an order, and just takes it as an argument.

@phackwer
Copy link
Author

" If two widgets have the same order in the same traversal group, they will get sorted in the order implied by the value set for secondary on the OrderedTraversalPolicy, "

So, I will need to keep adding ordering number as many times as I use the same widget on different contexts?

Ok, let's get a real example:

User has
PersonalInfo (4 fields)
AddressInfo (5 fields)

Organisation has
OrgInfo (2 fields)
AddressInfo (same widget)

Braches has
BranchInfo (3 field)
AddressInfo (same widget)

In this case maybe I should start to add to my widgets a "orderStartsFrom" and increment internally the order from there?

That seems like an acceptable solution.

@phackwer
Copy link
Author

phackwer commented Apr 21, 2020

Still it would be better to have Flutter to have a "NaturalOrderTraversalPolicy" or better "TreeOrderedTraversalPolicy" to be used and applied to focusable widgets in the order they are added to the screen. Only challenge here would be to add items that are automatically added to the screen by a user action between ordered widgets.

Suppose a situation like this:

  • There are 2 dropdowns for user to select from that are ordered 1 and 2.
  • User does first selection, and checkboxes are added between the 2 dropdowns (the list comes from the backend and must be realtime as it changes all the time, so, I can't have hidden checkboxes there using the Visibility component)
  • Now, I hit tab. Where does my focus goes to considering that dropdown 2 is ordered as 2?

Most browsers already have this solved when you don't declare the tabIndex (it just goes with the DOM tree order).

@phackwer
Copy link
Author

phackwer commented Apr 21, 2020

@gspencergoog : btw, thanks for the info about the 2 problems. I've been actually seem the list of problems just growing inside my head on trying to deal with it here in our app (like, all my "tabkeyable" widgets would need to inherit from a common widget to add this implementation, I need to resetList on every route change, I have the fact that onKey is not an array but a replaceable call - already found a problem on an autocomplete widget we may need here, and so on, too many problems to handle here).

@gspencergoog
Copy link
Contributor

With respect to your request for TreeOrderedTraversalPolicy, that is exactly what WidgetOrderTraversalPolicy is. It traverses the widgets in the order they are added to the widget tree.

@gspencergoog
Copy link
Contributor

The example code you have here seems like it is fighting with the existing keyboard traversal code, and seems to try and replace it to some extent, bypassing a lot of the code for handling shortcuts and focus traversal. I realize you're just looking for a solution.

You might try looking some more at the docs for those subsystems and seeing how you could work within them instead of trying to work around it: a lot of work has gone into making those systems work well that you could avoid duplicating.

You could use an orderStartsFrom on your wrapped group of fields, but better would be to just add a FocusTraversalGroup inside the AddressInfo widget that groups them in the order you want. Please take a look at the example here to see an example of what I mean.

I do think you can achieve what you are looking for without all of the management of focus nodes and key events. The combination of FocusTraversalGroup using an OrderedTraversalPolicy should do what you want, even if WidgetOrderTraversalPolicy isn't exactly the ordering you're looking for.

In any case, we already have something that orders traversal in the order that widgets are added to the widget tree, which was the original issue you filed, so I'm going to close this issue.

If you have further usage questions, you might want to post them to Stack Overflow, since those questions are often read by more people, and serve as a record for others to find who have similar issues in the future.

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: desktop Running on desktop c: new feature Nothing broken; request for a new capability c: proposal A detailed proposal for a change to Flutter f: focus Focus traversal, gaining or losing focus framework flutter/packages/flutter repository. See also f: labels. platform-web Web applications specifically
Projects
None yet
Development

No branches or pull requests

5 participants