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

An Option to Use Native DOM Events Instead of Synthetic Events #87

Closed
miketheodorou opened this issue Jul 28, 2023 · 8 comments · Fixed by #105
Closed

An Option to Use Native DOM Events Instead of Synthetic Events #87

miketheodorou opened this issue Jul 28, 2023 · 8 comments · Fixed by #105
Labels
question Further information is requested

Comments

@miketheodorou
Copy link

miketheodorou commented Jul 28, 2023

Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

Similar to the shouldFocusDOMNode option for the lib initialization, would you be able to add a flag like shouldUseNativeEvents? It would be much easier to write accessible HTML and tests if I could use native events (like onClick) instead.

Also, the Link component from react-router-dom does not work by default with this library. We instead have to create a synthetic Link and use the library's navigate function, instead of letting the library natively do its thing.

Describe the solution you'd like
A clear and concise description of what you want to happen.

Either init({ shouldFocusDOMNode: true }) should handle allowing the DOM / React events to come through on the focused nodes, or there should be another flag like init({ shouldFocusDOMNode: true, shouldUseNativeEvents: true }).

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

If we could key off the flag passed into the initialization config, perhaps we could not call event.preventDefault and event.stopPropagation in the bindEventListeners method.

Additional context
Add any other context or screenshots about the feature request here.

@miketheodorou
Copy link
Author

Hey Norigin Media Team, just following up on this to see if you have any thoughts or recommendations.

Thanks!

@predikament
Copy link
Collaborator

predikament commented Oct 9, 2023

Hello @miketheodorou!

Could you give a simple usage example based on your request here?

We've implemented a lot of accessibility related stuff and have not seen this need ourselves, so an example would help to better understand.

Thanks in advance.

@predikament predikament added the question Further information is requested label Oct 9, 2023
@miketheodorou
Copy link
Author

Thanks for the response @predikament! I'll get a little POC built and sent over to you when I get some free time.

@miketheodorou
Copy link
Author

miketheodorou commented Oct 13, 2023

Hey @predikament, I created a small POC to demonstrate the issues that I am having.

When I initialize the library with the following invocation, the onClick handlers on the buttons, as well as the native behavior for the Link component from react-router-dom no longer work when pressing the Enter key. I'm able to get the onKeyUp event to fire on one of the buttons, so it's not all events that are blocked, but the onClick is pretty important to keep enabled when we're actually focusing the DOM nodes.

init({ shouldFocusDOMNode: true });

I would prefer to not have to pass onEnterPress to the useFocusable options when shouldFocusDOMNode is enabled because it's breaking the base behavior of these elements onClick methods.

I would expect the behavior to work the way it does when you comment out the init invocation on line 8 of main.tsx and instead use Tab to move through the elements and press Enter to trigger the onClick handlers as well as the default navigation behavior.

Basically, I would like the library to move through the elements with the same logic it does, but not disable the onClick behavior.

Here are a couple videos as well that outline what I am describing.

With init invocation
https://github.com/NoriginMedia/Norigin-Spatial-Navigation/assets/22715776/0c2b76ad-3d64-4c2f-b1e2-ac40c6e418f1

Without init invocation
https://github.com/NoriginMedia/Norigin-Spatial-Navigation/assets/22715776/d7ce1f38-35a1-406c-87bd-fd2c88794a6b

@predikament
Copy link
Collaborator

@miketheodorou: Thanks for the POC and the videos!

We'll have a look.

(@asgvard: ⬆️)

@predikament
Copy link
Collaborator

@miketheodorou: Hello again! Busy weeks right now, but we've added a task to look into this issue and will be starting on it hopefully soon; Just so you know there's some activity on this topic.

@Braggiouy Braggiouy mentioned this issue Nov 13, 2023
3 tasks
@Braggiouy Braggiouy linked a pull request Nov 14, 2023 that will close this issue
3 tasks
@Braggiouy
Copy link
Contributor

Status Update:

The pull request #105 has been approved. It will be incorporated into the upcoming release, effectively addressing the associated issue.

@miketheodorou
Copy link
Author

@Braggiouy Thank you! Very excited to try this out. I appreciate you all adding this.

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

Successfully merging a pull request may close this issue.

3 participants