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

Implement Escape Management #33

Closed
wants to merge 0 commits into from
Closed

Implement Escape Management #33

wants to merge 0 commits into from

Conversation

uzurpastor
Copy link

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 uzurpastor changed the title Implement Escape Management [#32] Implement Escape Management Aug 7, 2024
@uzurpastor uzurpastor marked this pull request as ready for review August 7, 2024 17:49
@uzurpastor
Copy link
Author

uzurpastor commented Aug 7, 2024

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
Copy link
Owner

ai commented Aug 7, 2024

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
Copy link
Author

  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:

  • ignoring the click listener from focus-group.js

@ai
Copy link
Owner

ai commented Aug 8, 2024

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
Copy link
Author

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:

  • focus is not hidden when escape is pressed
  • focus hides when you click

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
Copy link
Author

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
Copy link
Owner

ai commented Aug 11, 2024

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
Copy link
Author

uzurpastor commented Aug 13, 2024

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:

  • focus on Home input
  • esc : no focus
  • tab : focus on Copy

ER:

  • focus on Home input
  • esc : no focus
  • tab : focus on Home input

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

@ai
Copy link
Owner

ai commented Aug 13, 2024

tab : focus on Home input

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

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

Successfully merging this pull request may close these issues.

2 participants