Focus management

[jdeniau]

This PR tries to start a focus management for ink.
This is a simple start but it should be extensible without breaking the API.

I really tried to write tests for this, but either I got issue with babel and "import React" errors or I did not managed to make hook works 🤔

If you can give me an hint about that ?

Fixes #242

[jdeniau]

I'm not really sure about this line, I am not really used to typescript 🤷‍♂️

[taras]

@jdeniau thank you for working on this. My team and I tried to implement this over the last few days because didn't see this PR. We ended up with a very similar solution with the one exception that we're treating focus as a tree of focusable components rather than a flat list.

Have you considered this approach?

[jdeniau]

I did not. I was inspired by the browser "tabindex" capacity. Does a tree approach gives other interesting possibilities? Is your code available somewhere to give a look at it? Le ven. 6 mars 2020 à 22:29, Taras Mankovski <notifications@github.com> a écrit :

…

@jdeniau <https://github.com/jdeniau> thank you for working on this. My team and I tried to implement this over the last few days because didn't see this PR. We ended up with a very similar solution with the one exception that we're treating focus as a tree of focusable components rather than a flat list. Have you considered this approach? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#262?email_source=notifications&email_token=AAKVNRJJXLL2KP7MGJGT2P3RGFTJ7A5CNFSM4K6BHPZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOC5GSQ#issuecomment-595972938>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKVNRIF375RD5DMNXGX4A3RGFTJ7ANCNFSM4K6BHPZQ> .

[taras]

Tree based focus management is helpful when you want to implement UI where new focus items are added to the screen after initial list of items was rendered. This is common with Detail List view. I made a diagram to show what happens when you manage focus as a tree.

Without a tree,

At step 4, new items will be appended to the end of the list. Which means that getting to details will require tabbing through all previously rendered items. At step 7, it's difficult to know that you have to return to item 2 without doing some explicit tracking of where the user was previously.

We have a spike of an implementation of Focus Management but it's not a good demo yet and the code is quite rough. Feel free to poke around https://github.com/thefrontside/bigtest/blob/cli-mob/packages/ui/src/FocusManager.tsx#L106

[taras]

@jdeniau just checking with you. Did you have any thoughts on this? If you like this approach, I'll be happy to pick it up after #264

[jdeniau]

@taras I did not watch your code for the moment.

About the tree approach, I am torn between using a tabindex-like usage, to be consistent with the browsers, or to use a more complex but evolutive approach 🤔

Maybe @vadimdemedes could share what he thinks about how he want to drive ink ?

[taras]

@taras I did not watch your code for the moment.

No worries. It's no hurry.

About the tree approach, I am torn between using a tabindex-like usage, to be consistent with the browsers, or to use a more complex but evolutive approach 🤔

tabindex works fine if after the initial render, you're only adding focusable items at the end of the list. This is one of challenges of making modern SPAs accessible. If you need more granular control of focusable areas that change over the lifetime of the application then you need another data structure. Something like a tree of tab indexes which is what I described above. So, it's not so much that this approach doesn't use the same ideas, it just expands on them to accommodate more use cases.

[jdeniau]

I know the limitations of what I will say, but you can also insert tabindex between two tabindex as you can see here : https://codepen.io/jdeniau/pen/bGdKmMJ?editors=1010

It is of course limited as you need to know how many items you will insert between two main items 😃

I read briefly your code by the way, but it seems quite complex at 21:00 without knowing bigtest. Il will give it another chance in the next days 😉

[taras]

It's definitely doable because you can express any tree as an array.

I read briefly your code by the way, but it seems quite complex at 21:00 without knowing bigtest. Il will give it another chance in the next days 😉

It's more complicated than it needs to be because we're essentially building a tree in React with Context. It would be a lot simpler if we implemented this functionality at a lower level and just use the DOM tree that's built into Ink.

[jdeniau]

What do you think to be a good public API then ?

I was thinking of improving the current API like this :

useFocus({ tabindex: 1 });
withFocus(Element, { tabindex: 1 })

But how whould this work with a tree ? Do you intend to keep an API like this ? A possible solution is to use a semver equivalent : 1.1.8.2 or as an array [1, 1, 8, 2], but it seems complex to maintain 😃

[taras]

What do you think to be a good public API then ?

We might not even need to specify an index. The render position in the DOM tree might be an acceptable default. We could start by shipping a <Focusable> component and/or useFocusable hook that returns a ref that can be passed into an element. Both of these would be sufficient without using any arguments. We can expand the API as we discover use cases that this API is not able to satisfy.

[vadimdemedes]

I'm not sure we need complicated solutions to be as close as possible to browser behavior, as CLIs are supposed to be simple, clean and easy to use.

My gut tells me we should go with the simplest solution possible at first and perhaps expose it as unstable prefixed feature, so people could test drive it, but also we could retain the freedom to change the API as we go and discover issues and use cases.

I'm going to dive deeper into this topic to have more context and will ping you guys back soon. We should hold off on this PR for a bit anyway, as there's currently a massive PR pending to convert codebase to TypeScript.

What do you all think?

[taras]

Sounds good to me. I’m going to spike this in the CLI that we are building for BigTest. It’ll be easier to discuss specifics once there is a sufficiently complicated example to look at.

[jdeniau]

Looks good to me. The unsable tag seems a good point, as we are not sure of where to go here.

Feel free to ping me if you want to go further with this PR.

[vadimdemedes]

@jdeniau Yeah, I think we should have that in Ink. By the way, have you seen reactjs/rfcs#104? Perhaps it might be interesting for you to check out!

[taras]

RFC for an RFC - explains why there is no actual RFC :)

[jdeniau]

@vadimdemedes I did not. If this PR is on hold, let's can see what comes out of the RFC.

As a personnal level, I do not really like of React (dom version) overriding the browser, but it may seems interesting for the "large" react community.

[apatrida]

Sounds good to me. I’m going to spike this in the CLI that we are building for BigTest. It’ll be easier to discuss specifics once there is a sufficiently complicated example to look at.

@taras do you have this public and working?

[vadimdemedes]

Closing in favor of #295. Thanks @jdeniau for your work! I'm going to make sure to give you credit in release notes, as my implementation is based on yours.