ai / keyux

JS library to improve keyboard UI of web apps
https://ai.github.io/keyux/
MIT License
391 stars 18 forks source link

Implement Escape Management #33

Closed uzurpastor closed 2 weeks ago

uzurpastor commented 1 month ago

This PR addresses issue #32 by implementing enhanced keyboard focus management. I'm unsure if the style fully aligns with our codebase and would appreciate feedback.

Looking forward to your insights and recommendations.

uzurpastor commented 1 month ago

Yes. I forgot to run the tests on my local. There is problems with package size. No idea how to reduce it.

Also I didn't implement tests for the feature. Will do it.

ai commented 1 month ago

Can you explain what this module does (“manager” is too common idea) and your design decision?

  1. Why it is a separated module and not fix for jumps?
  2. What is use cases? Who will use it, and who don’t need it?
uzurpastor commented 1 month ago
  1. I have found that a separate module is the easiest way to implement a function prototype. Agree, the best way to implement logic is to put it in transitions.
  2. I've thought about this a lot, but I can't give a definite opinion. In my opinion, it's bad UX when users don't know which element the app will focus on after blurring.

Maybe it would be better to rewrite the module to focus on the last focused element. Your opinion?

Some bugs I've discovered:

ai commented 1 month ago

I've thought about this a lot, but I can't give a definite opinion. In my opinion, it's bad UX when users don't know which element the app will focus on after blurring.

In this case it should be a fix for existing module.

Maybe it would be better to rewrite the module to focus on the last focused element.

How does browser works in the most similar use case?

uzurpastor commented 1 month ago

How does the browser behave in the most similar use case?

I've examined the default browser behaviour without any scripting, as well as the GitLab/Google behaviour:

This means that all my work was unnecessary, because we can achieve the same behaviour just by removing the blur() method call in jump.js:16.

My bad.

uzurpastor commented 1 month ago

I think I need to publish an comment for action.

From the beginning, I wanted to achieve default browser behaviour on pages with KeyUX libs. But I didn't investigate this issue before implementing it. To achieve better behaviour you should remove the line jump.js:16(I can create a PR with changes if you also think this behaviour is more correct)

The function implemented in the current PR returns focus to the first focused element on the page. This function is not necessary for KeyUX. The PR should be removed.

Thanks for your time. I will keep an eye out for assignments from EvilMartians. Best, Andrey

ai commented 1 month ago

To achieve better behaviour you should remove the line jump.js:16

Why it is “better”?

The idea of current behavior is to return to origin state. The origin state is no focus on the page.

uzurpastor commented 1 month ago

Sorry for late answer.

Okey, let's forgot all my previous comments. And update the problem description

Let's look at the problem with an example: https://github.com/user-attachments/assets/d41ecc57-b6f7-471d-880d-3ae61b85750b

AR:

ER:

I think I can solve the problem described above. What do you think?

ai commented 1 month ago

tab : focus on Home input

Yes, we may want to reset menu after leaving the focus. Can I ask you to send PR?