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

Issues with keyboard navigation on KDropdownMenu #588

Closed
AlexVelezLl opened this issue Mar 25, 2024 · 21 comments · Fixed by #818
Closed

Issues with keyboard navigation on KDropdownMenu #588

AlexVelezLl opened this issue Mar 25, 2024 · 21 comments · Fixed by #818
Assignees
Labels
bug Component: KDropdownMenu help wanted Open source contributors welcome

Comments

@AlexVelezLl
Copy link
Member

Product

Noted in Studio, and in the KDS Docs on the last example of KListWithOverflow.

Expected behavior

Two things are not working very well right now:

  • When the list has dividers, the keyboard navigation should ignore them and focus on the next or previous item in the list.
    • In the options list, We can include dividers with a { type: "Divider" } object.
  • When the user presses tab or esc, we should return the focus to the last element focused before KDropdownMenu was active. This can be done by saving the document.activeElement before focusing the first option of the dropdown menu.

Actual behavior

  • Right now, the focus doesn't change when there are dividers and we want to focus the next item, and we can't jump the dividers.
  • As KDropdownMenu is rendered in the body element, when esc or tab is pressed, the focus returns to the first focusable element on the document after a second. But before the focus returns to the body, the next option in the menu is focused, and after a second, the menu closes, which is a bit misleading since, at first, it seems that we could navigate through the menu with a tab.

Steps to reproduce the issue

  1. Go to the KListWithOverflow documentation.
  2. Click on the box of the last list example.
  3. Navigate with the keyboard through the dropdown menu and note the issues described above.

Additional information

Compartir.pantalla.-.2024-03-25.16_22_53.mp4
@galovdev
Copy link
Contributor

Can I get this assigned?

@MisRob
Copy link
Member

MisRob commented Mar 26, 2024

Hi @galovdev, thank you for volunteering, I will assign you

@nucleogenesis
Copy link
Member

Hello @galovdev, are you still interested in contributing on this issue? If not, we will reassign it.

@galovdev
Copy link
Contributor

Hello @galovdev, are you still interested in contributing on this issue? If not, we will reassign it.

Hi, please give me this weekend, Im working on the other issue

@marcellamaki
Copy link
Member

Thank you @galovdev - no problem. Please reach out if you have any questions.

@MisRob
Copy link
Member

MisRob commented Jun 11, 2024

Hi @galovdev, are you still planning to work on this?

@Sahil-Sinha-11
Copy link
Contributor

Hey @AlexVelezLl may I work on this issue?

@AlexVelezLl
Copy link
Member Author

Hey @Sahil-Sinha-11! Yes, for sure! You are welcome! I will assign this issue to you! :)

@Sahil-Sinha-11
Copy link
Contributor

Sahil-Sinha-11 commented Nov 6, 2024

Hey @AlexVelezLl I am a bit lost in the code can you please guide me a bit.. 😅. I tried using handlekeyNavigation and event listeners on keydown but did not get any progress.

@AlexVelezLl
Copy link
Member Author

Hey @Sahil-Sinha-11. For sure!
For number 1 we should be able to focus the next element even if there is a Divider. For this we should be looking at how we query our next and previous siblings here, and ignore the dividers.

For number 2, we can see how KModal handles the lastFocus element

this.lastFocus = document.activeElement;
and how they return the focus once the component is destroyed
window.setTimeout(() => this.lastFocus.focus());
. Probably with the dropdown a destroyed hook wouldnt work because I think it doesnt get destroyed but just hidden, so we will need to figure out a way of notifying when the dropdown is hidden and then return the focus when that happens.

Hope this helps :), let me know if you need more guidance.

@Sahil-Sinha-11
Copy link
Contributor

Screen.Recording.2024-11-08.at.12.16.34.AM.mov

Hey @AlexVelezLl Thanks a lot for the help. Should this be the expected behavior for issue 1?

I tried to make some changes in kmodel tracking dropdown and resetting focus but I was not able to produce the expected results. Can you please give me some more guidance.

@AlexVelezLl
Copy link
Member Author

Hey @Sahil-Sinha-11, yes that is the expected behaviour 🎉.

I tried to make some changes in kmodel tracking dropdown and resetting focus but I was not able to produce the expected results. Can you please give me some more guidance.

Could you open a PR with your code, please?, so we can discuss better there 🤗

@Sahil-Sinha-11
Copy link
Contributor

Sure @AlexVelezLl , I opened PR#818. 😃

@AlexVelezLl
Copy link
Member Author

Thanks @Sahil-Sinha-11! I will take a look

@MisRob
Copy link
Member

MisRob commented Nov 8, 2024

Thanks @Sahil-Sinha-11 and @AlexVelezLl.

I tried to make some changes in kmodel tracking dropdown and resetting focus but I was not able to produce the expected results. Can you please give me some more guidance.

@Sahil-Sinha-11 Generally, it would be helpful to mention in your PR what approaches you tried or do some debugging and describe what you're observing exactly. We're happy to provide guidance and support, however that will be rather in the form of giving direction and answering concrete questions, rather than resolving the issue in question.

@Sahil-Sinha-11
Copy link
Contributor

Thank you for the suggestion! I realize I should have shared my approach as well, my apologies. Here’s how I’ve been tackling it so far...@MisRob @AlexVelezLl ,
Image
I added a variable to track dropdown,
Image
when the dropdown closes it calls the resetfocus function to return to last focused element,
Image
and this is the function to where it returns the focus
I tried using this but it didn't work..😅.

@MisRob
Copy link
Member

MisRob commented Nov 8, 2024

Thanks @Sahil-Sinha-11, this is helpful. Overall looks like a good direction. Some first suggestions:

(1) Where is the place where the previously focused element gets saved to lastFocus? Ssimilarly to

this.lastFocus = document.activeElement;
? Is it implemented and you just didn't take a screenshot or is it not there yet?

(2) If (1) is satisfied, I'd recommend next steps would be to debug, e.g. console log the active element and lastFocus value, and track the place where exactly the intended logic is breaking. You will probably spot some mismatch between what you wanted to happen. A soon as you are a bit closer what place is causing the issue, let @AlexVelezLl or me know and we can chat about the approach to resolve that.

@AlexVelezLl
Copy link
Member Author

AlexVelezLl commented Nov 8, 2024

Hey @Sahil-Sinha-11! I think there has been a misunderstanding here 😅. The change isnt needed in KModal, but in KDropdownMenu. We need to replicate in KDropdownMenu the lastFocus behaviour that KModal has, but the change needs to be done in KDropdownMenu. The challenge is that, unlike KModal, we cant rely on the mounted hook to know where the last focus was, because KDropdownMenu is always mounted, its not mounted/unmounted each time we open/close it.

So when we open it, we need to store the element that is focused before any of the items in the menu gets the focus. And when we close the dropdown, we need to return the focus to the element that was focused before we open the dropdown.

Hope this helps clarifying what is needed 🤗.

@Sahil-Sinha-11
Copy link
Contributor

Thanks @Sahil-Sinha-11, this is helpful. Overall looks like a good direction. Some first suggestions:

(1) Where is the place where the previously focused element gets saved to lastFocus? Ssimilarly to

kolibri-design-system/lib/KModal.vue

Line 254 in ea611a5

   this.lastFocus = document.activeElement; 

? Is it implemented and you just didn't take a screenshot or is it not there yet?
(2) If (1) is satisfied, I'd recommend next steps would be to debug, e.g. console log the active element and lastFocus value, and track the place where exactly the intended logic is breaking. You will probably spot some mismatch between what you wanted to happen. A soon as you are a bit closer what place is causing the issue, let @AlexVelezLl or me know and we can chat about the approach to resolve that.

Hey @MisRob, Its in the before mount Image
But, Now as @AlexVelezLl suggested, I would try to implement the storage of last focus element in the KDropdownMenu, I am on it.🧑🏻‍💻

@MisRob
Copy link
Member

MisRob commented Nov 11, 2024

@Sahil-Sinha-11 Wonderful, thanks. Yes, KModal is just an example for you to take an inspiration from. KDropdownMenu can be used from many more places, not just from within modals, so it needs to be implemented within the menu itself.

@AlexVelezLl
Copy link
Member Author

Closed by #818

@AlexVelezLl AlexVelezLl linked a pull request Jan 7, 2025 that will close this issue
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Component: KDropdownMenu help wanted Open source contributors welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants