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

Simple idea for dealing with focus problem #12

Closed
bsouthga opened this issue Sep 10, 2017 · 11 comments
Closed

Simple idea for dealing with focus problem #12

bsouthga opened this issue Sep 10, 2017 · 11 comments

Comments

@bsouthga
Copy link
Contributor

Really cool project!

Simple idea with issue of losing focus -- we could check for an active element that has an id before each render and try to refocus after, basically giving people the option of adding unique ids to elements they want to keep focused between renders:

https://github.com/bsouthga/innerself/blob/maintain-focus/index.js#L22

Thoughts?

@bcruddy
Copy link
Contributor

bcruddy commented Sep 10, 2017

document.querySelector(':focus') also returns the element that is currently "in focus" so it can be captured when calculating state instead of being actively tracked. Keeping track of previously focused elements requires a lot of work but I wouldn't think checking an element CSS state is required as far as DOM diffs are concerned.

@bsouthga
Copy link
Contributor Author

bsouthga commented Sep 10, 2017

Could be my lack of understanding, but I'm not sure I follow your point. My thinking was that each render() is going to blow away any elements within root, so to maintain focus after rendering, we will need to re-focus the last focused element.

As far as I can see, the only consistent way to uniquely identify an element between renders would be via a unique id. We could use querySelector(':focus') but my assumption was that that would be much worse performance wise.

@stasm
Copy link
Owner

stasm commented Sep 10, 2017

While capturing the focused element is certainly possible within the reducer, there's currently no way to hook into the inner workings of the store's render function, after which we'd need to re-focus.

#13 looks like a simple work-around. Thanks for submitting it. Let me think about it for a bit and I'll get back to it tomorrow.

@stasm
Copy link
Owner

stasm commented Sep 11, 2017

I submitted #15 as an alternative solution. In a way it's a more generalized #13. Rather than re-focusing inside of the render function, the store dispatches an event now. This moves the responsibility of re-focusing to the developer. While it's a bit more work for developers, it's also more low-level and powerful. It allows to properly restore selection and caret positions after render, too, with selectionStart and selectionEnd.

@bsouthga I'm curious to hear your thoughts on this approach!

@bsouthga
Copy link
Contributor Author

bsouthga commented Sep 11, 2017

Nice! I like the use of CustomEvent -- this seems much more future proof.

stasm added a commit that referenced this issue Sep 12, 2017
The render event gives the users of innerself a hook to add custom logic
after the render is complete. It allows to restore focus, selection
and caret positions after render.

Fixes #12.
@stasm stasm closed this as completed in 3af6985 Sep 12, 2017
@wrick17
Copy link

wrick17 commented Sep 13, 2017

This fix will solve the focus problem in text inputs. However if we put an audio or video tag in our app that would stop playing with any interaction that causes a re-render. It's not really a problem, but something that would be a scenario for which we can't really use this lib.

@f1yn
Copy link
Contributor

f1yn commented Sep 13, 2017

In that case, might be a better idea to have those elements outside of the innerself render area.

Additionally, assuming you are using this to make an app, it should be possible to create multiple roots as independent renders.

That being said, I'm not sure the current state implementation allows for multiple roots.

@stasm
Copy link
Owner

stasm commented Sep 13, 2017

It does! But I think the trouble is that as soon as you use innerself for some kind of routing (e.g. a tabbed interface), you can't really attach multiple roots independently because you need to control when they show up.

@wrick17
Copy link

wrick17 commented Sep 13, 2017

Right. What I felt is for making something like a video player or audio player, innerself wouldn't be the ideal choice. There'll be a lot more hacking and workarounds than required. Simple text based applications should be the ideal candidates for this.

@stasm
Copy link
Owner

stasm commented Sep 13, 2017

@wrick17 Absolutely. I wrote it with a specific use-case in mind and I'm first to admit that it's not a versatile multitool, like React :)

@wrick17
Copy link

wrick17 commented Sep 13, 2017

And that's what I like about this library. It does one thing, and does that amazingly. Appreciate it!!

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

No branches or pull requests

5 participants