Issues with keyboard navigation on KDropdownMenu

[Sahil-Sinha-11]

…p down position

Description

Issue addressed

Addresses #588

After Video

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

Changelog

• Description: keyboard navigation on KDropdownMenu
• Products impact: bugfix.
• **Addresses:**Issues with keyboard navigation on KDropdownMenu Issues with keyboard navigation on KDropdownMenu #588.
• Components: kDropdownMenu.vue
• Breaking: -
• Impacts a11y: -
• Guidance: -

Steps to test

1. Step 1
2. Step 2
3. ...

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

• Contributor has fully tested the PR manually
• If there are any front-end changes, before/after screenshots are included
• Critical and brittle code paths are covered by unit tests
• The change is described in the changelog section above

Reviewer guidance

• Is the code clean and well-commented?
• Are there tests for this change?
• Are all UI components LTR and RTL compliant (if applicable)?
• Add other things to check for here

Comments

[AlexVelezLl]

This is a good way to determine if the element is a divider, but is a little specific. If anything changes and in the future we get rid of this specific class. This would break this condition.

A more general solution could be to ask if the element has a tabIndex >= 0, so this means it is focusable and with this, we can skip the dividers.

[Sahil-Sinha-11]

Oh right, that seems more logical to skip any other element [like divider] in a more generalized way. Thanks @AlexVelezLl .

[AlexVelezLl]

We could write two separate methods here, to breakdown a little bit this method. so we can just do something like

const sibling = this.getNextFocusableSibling(focusedElement);
const prevSibling = this.getPrevFocusableSibling(focusedElement);

[Sahil-Sinha-11]

Hey @AlexVelezLl , I've defined prevSibling Already here

and the variable sibling Is for next focused element.

[AlexVelezLl]

Yes, my suggestion was more to avoid writing more logic to query the next/previouss element in the same handleOpenMenuNavigation method. So lets define two new methods in this component getNextFocusableSibling and getPrevFocusableSibling where we should do the focusable check and the while sentence.

e.g.

methods: {
  ...
  getNextFocusableSibling(element) {
    let sibling = focusedElement.nextElementSibling;
    while (sibling && !isFocusable(sibling)) {
      sibling = sibling.nextElementSibling;
    }
    return sibling;
  }
}

and lets refactor the definition of these variables prevSibling and sibling to use the result of these methods.

[Sahil-Sinha-11]

Hey @Sahil-Sinha-11, I made the suggested changes. 🤗
PS: sorry for the late reply, actually my university exams are next week so quite loaded with tests. 😅

[Sahil-Sinha-11]

Hey @AlexVelezLl , the focus works like this, should I return it to the last focused button?

Screen.Recording.2024-11-11.at.1.27.50.AM.mov

[AlexVelezLl]

Hey @Sahil-Sinha-11. Yes!! It should be working like that, when we close the dropdown, the last focused element is the element that should be focused. Thank you!! Loking forward to see this change pushed in the PR 💯.

PS: sorry for the late reply, actually my university exams are next week so quite loaded with tests. 😅

No worriees! 😅 There is no rush, please take your time 🤗.

[Sahil-Sinha-11]

Hey @AlexVelezLl, I made the changes, please review it, thanks.

[AlexVelezLl]

Woo 🎉. This is working soo smothly @Sahil-Sinha-11. Thank you!! I have left a couple of comments just for some code clean up, thanks!

[AlexVelezLl]

We can get rid of this line

[AlexVelezLl]

Although there is no problem with having this line as the last line of the handleOpen method, we can move it to be the first line of the method, so the intention of storing the last focused element before we focus anything else is clearer :).

[AlexVelezLl]

Above this, you can see there is a call to the this.focusOnButton() method, which apparently was an old behaviour we had in KDropdownMenu, but its no longer useful as now KDropdownMenu doesnt render any button anymore.

But if in the past, we were making this call in those conditions, it was for some reason (perhaps because of an edge case im not aware of), but to play safe, lets just replace that focusOnButton() call with this lastFocusElement call, for this focus to occur on those conditions too, so we would have something like:

        if (
          popover.contains(focusedElement) &&
          (focusedElement.classList.contains('ui-popover') ||
            focusedElement.classList.contains('ui-popover-focus-redirector') ||
            focusedElement.classList.contains('ui-menu-option'))
        ) {
          if (this.lastFocusElement) {
            this.lastFocusElement.focus();
          }
        }

And then we can remove the focusOnButton method :).

[AlexVelezLl]

Great catch! We dont need to make these calls always. We can remove these lines too.

[AlexVelezLl]

lets define these prevSibling and sibling as const, as we wont need to reassign them anymore.

[Sahil-Sinha-11]

Hey @AlexVelezLl, I made the suggested changes, please review them, thanks.

[AlexVelezLl]

Thank youu @Sahil-Sinha-11!! Code changes look good to me! :) . I have just opened a PR in Kolibri with your changes for QA to test this in Kolibri and confirming that this is the expected behaviour, after that we can merge your PR. Thanks @Sahil-Sinha-11! Good job 🤗

[AlexVelezLl]

Thanks @Sahil-Sinha-11!

[MisRob]

Thanks @Sahil-Sinha-11 and @AlexVelezLl, good chunk of work here :)!