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.

Sign up for GitHub

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 #818

Merged
merged 6 commits into from
Nov 19, 2024
Merged

Issues with keyboard navigation on KDropdownMenu #818

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c1419de
added logic in handleOpenNavigation to ignore is-divider while in dro…
Sahil-Sinha-11 Nov 6, 2024
1371cd9
edited the the isdivider method to tabIndex method for a generalized …
Sahil-Sinha-11 Nov 10, 2024
e530df6
added functionality to focus last element after drpdown closes
Sahil-Sinha-11 Nov 11, 2024
19d51bc
refactored focus code and added getnext and get prv focused element u…
Sahil-Sinha-11 Nov 11, 2024
4c242e3
refactoerd the code
Sahil-Sinha-11 Nov 12, 2024
e92a739
removed the focuson button method
Sahil-Sinha-11 Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions lib/KDropdownMenu.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,19 +175,36 @@
const popoverIsOpen = popover.clientWidth > 0 && popover.clientHeight > 0;
// set current element and its siblings
let focusedElement = document.activeElement;
let sibling = focusedElement.nextElementSibling;
let prevSibling = focusedElement.previousElementSibling;
// let sibling = focusedElement.nextElementSibling;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// let prevSibling = focusedElement.previousElementSibling;

//function to check if the sibling is divider
const isDivider = element => {
return element && element.classList.contains('is-divider');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

// manage rotating through the options using arrow keys
// UP arrow: .keyCode is depricated and should used only as a fallback
if ((event.key == 'ArrowUp' || event.keyCode == 38) && popoverIsOpen) {
event.preventDefault();

// Checking if previous sibling is divider and if yes then skip it
let prevSibling = focusedElement.previousElementSibling;
while (prevSibling && isDivider(prevSibling)) {
prevSibling = prevSibling.previousElementSibling;
}
prevSibling
? this.$nextTick(() => prevSibling.focus())
: this.$nextTick(() => lastChild.focus());
// DOWN arrow
} else if ((event.key == 'ArrowDown' || event.keyCode == 40) && popoverIsOpen) {
event.preventDefault();

//Chekcing if next sibling is divider and skipping it
let sibling = focusedElement.nextElementSibling;
while (sibling && isDivider(sibling)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @AlexVelezLl , I've defined prevSibling Already here
Screenshot 2024-11-11 at 12 21 48 AM
and the variable sibling Is for next focused element.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sibling = sibling.nextElementSibling;
}
sibling ? this.$nextTick(() => sibling.focus()) : this.$nextTick(() => this.setFocus());
// if a TAB key, not an arrow key, close the popover and advance to next item in the tab index
} else if ((event.key == 'Tab' || event.keyCode == 9) && popoverIsOpen) {
Expand Down