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

Add useFocus hook to manage input focus #242

Closed
ukabu opened this issue Nov 13, 2019 · 12 comments
Closed

Add useFocus hook to manage input focus #242

ukabu opened this issue Nov 13, 2019 · 12 comments

Comments

@ukabu
Copy link

ukabu commented Nov 13, 2019

Do you have any recommandation for managing focus for multiple input component.
My current use case is to be able to display multiple SelectInput and be able to tab thru them. The component already have a focus prop, but how would I manage the focus when the multiple SelectInput are not within the same react component?

@swyxio
Copy link

swyxio commented Nov 18, 2019

possibly related: jdeniau/ink-tab#14

@vadimdemedes
Copy link
Owner

As you noted, you'd need to use focus prop, but I can see how that can become troublesome when components are elsewhere. Perhaps it's time to introduce something like useFocus() to make Ink manage focus and let 3rd-party components easily benefit from it. I will think about it more, but can't give you an ETA, so until then I'm afraid you'd have to implement a custom solution for this problem using a state for focus above your components, so that you can pass it down to multiple places.

@vadimdemedes vadimdemedes changed the title Managing focus for multiple inputs Add useFocus hook to manage input focus Nov 23, 2019
@jdeniau
Copy link
Contributor

jdeniau commented Nov 23, 2019

I think it's a good approach that ink manage the focus, as does a web browser 👍

@jdeniau
Copy link
Contributor

jdeniau commented Dec 31, 2019

@vadimdemedes I started working on this here : https://github.com/jdeniau/ink-focus-demo (demo.js)

There is still some work to do though, like how to make "inputs" non-responsive to focus-changer key (tab + shift-tab?), managing "tabIndex", and eventually in the future why not attach onFocus and onBlur events (but that may be included in a further reflexion on events).

I think I found bugs whith keypress in the same time (due to the change of keypress paradigm in 2.4.0 I think), I will try to open PRs on that.

The external API is a little too complex I think (register a ref and use it after that in useFocus, the non-hook version is worse). A possible simpler solution may be to use a HOC withFocus instead (but I did not try that approach for now).
(edit: I added the HOC and it seems way better, I just need to test in with refs, as it should not work as I did, probably need to use forwardRef)

As a matter of fact, I do not like the "ref" approach that I used but I did not really think of another viable solution.

I think it is a good start to keep a global focus state. I will try to work on this more and make a PR whenever possible.

@jdeniau
Copy link
Contributor

jdeniau commented Jan 8, 2020

@vadimdemedes I have a working version on https://github.com/jdeniau/ink-focus-demo now.

The API is the following :
Encapsulate every component that may be focusable like that :

import TextInputWithoutFocus from "ink-text-input";
import { withFocus } from "ink";

const TextInput = withFocus(TextInputWithoutFocus);

See TextInput.js

The wrapped component will receive a focus boolean prop (I used the ink-text-input props, but I think isFocused or hasFocus is more clear).

Ink should then manage focus between every focusable elements. App.js

You can switch between focusable elements by pressing "tab" or "shift+tab"

There is one "problem" I still see before making a PR :
I don't see a way to make the wrapped component unresponsive to keyboard input. That's why we need the focus prop, but there still is a conflict with the "tab" key :
As every component registers itself on either "keypress" or "data" listener, there is a weird issue when you switch between two inputs : one of them (the "focused" one) will receive a "tab" input.
It can be easily managed (and required) in every library that use withFocus but it's sad that there are no other options here.

@vadimdemedes
Copy link
Owner

I have had only rough ideas for useFocus, so I'm very curious to check out your take on it, nice!

@vadimdemedes
Copy link
Owner

I checked out your repo and it's really good, very similar to what I had in mind. The only difference would be that I'd prefer users to use useFocus hook directly instead of withFocus HOC.

don't see a way to make the wrapped component unresponsive to keyboard input. That's why we need the focus prop, but there still is a conflict with the "tab" key

Good point, not sure what's the ideal solution here. One way to solve this would be to let text inputs that expect "Tab" to disable focus switching? It's not perfect, but I don't think majority of text inputs would expects those.

@jdeniau
Copy link
Contributor

jdeniau commented Feb 16, 2020 via email

@jdeniau
Copy link
Contributor

jdeniau commented Feb 20, 2020

As I re-read my code about that, I think we might expose both useFocus and withFocus, but useFocus is a little harder here, as you need to provide a ref to your component (but you will have more control afterwards).

Another way around is to return a ref that you might inject in your component this way:

function FooInput() {
  const [hasFocus, ref] = useFocus(possibleRef);
}

but that seems really really weird 🙃

@jdeniau
Copy link
Contributor

jdeniau commented Feb 25, 2020

I managed to remove the ref dependency, I will need to test it further.
This way, the hook is useable. As a matter of fact, it is more in adequation with the tabindex phisolophy I think.

As soon as I manage to handle the TAB key, I will open a PR

jdeniau added a commit to jdeniau/ink that referenced this issue Feb 28, 2020
jdeniau added a commit to jdeniau/ink that referenced this issue Feb 28, 2020
jdeniau added a commit to jdeniau/ink that referenced this issue Feb 28, 2020
@hcharley
Copy link

hcharley commented Jun 2, 2020

This would be really awesome. I feel right now I have to reinvent the wheel here to do this.

@vadimdemedes
Copy link
Owner

vadimdemedes commented Jul 27, 2020

Ink 3 is out with the fix for this issue included! Read the full announcement at https://vadimdemedes.com/posts/ink-3 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants