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

Stop propagation of keyboard navigation in a number of components #4501

Merged
merged 7 commits into from Sep 7, 2023

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Sep 4, 2023

☑️ Resolves

#4406 was merged too early, it solved only problems with Arrows in general, but not ESC.

Arrows and ESC are used for keyboard navigation in some components. But the keydown event is not stopped. As a result, arrows and ESC not only manipulate such components but also are handled globally.

For example:

  • NcModal:
    • ESC in the open emoji picker or select inside NcModal should only close popover, but also close the Modal
    • ← → in Tabs should only switch tabs but also switch slides in NcModal

To test add a global event listener and check console:

window.addEventListener('keydown', console.log)

🖼️ Screenshots

🏚️ Before 🏡 After
ESC in open select closes modal ESC in open select closes select
select-before select-after
ESC in open emoji picker closes modal ESC in open emoji picker closes the picker
emoji-before emoji-after

🚧 Tasks

  • Add more items to example of NcModal
  • Simply add .stop modifier:
    • NcAppSidebar
      • Stop propagation of ESC on title edit with nameEditable
    • NcAppSidebarTabs
      • Stop propagation of all keyboard navigation: ESC, ↓ ↑ → ←
    • NcInputConfirmCancel
      • Stop propagation of ESC
  • NcSelect: add interceptor via mapKeydown to stop propagation when select is open
  • NcPopover: manually add keydown event listener similar to how focus trap is added. It can be cleaner with an additional wrapper of popover content
    • It also fixes components, based on NcPopover:
      • NcActions
      • NcEmojiPicker
      • NcUserBubble
      • NcColorPicker
      • NcDatetimePicker

Ignored:

  • NcHeaderMenu:
    • Global ESC handler. It is not a problem, it is never used inside other components with a global handler.
  • NcListItem:
    • Hide actions on ESC when focused. Should not be a problem, it's also not used inside other components with a global handler.

Additional discussion

Both this PR and #4406 fixes Arrows navigation in Sidebar tabs being in a modal viewer. But they do it in a different way:

  • That PR saves NcModal from switching slides by any not stopped arrows outside the modal
  • This PR saves any modal-like component from handling Arrows navigation of its content.

I'd keep both PRs but we can revert #4406.

For example, if we want to switch slides in Viewer when the focus is on Tab panel content div.

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@ShGKme ShGKme added bug Something isn't working feature: popover Related to the popovermenu component feature: app-navigation Related to the app-navigation component feature: select Related to the NcSelect* components feature: app-sidebar Related to the app-sidebar component accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Sep 4, 2023
@ShGKme ShGKme self-assigned this Sep 4, 2023
@ShGKme ShGKme added this to the 8.0.0 milestone Sep 6, 2023
@ShGKme ShGKme marked this pull request as ready for review September 6, 2023 14:20
@szaimen szaimen removed their request for review September 6, 2023 15:29
src/components/NcSelect/NcSelect.vue Outdated Show resolved Hide resolved
@ShGKme ShGKme requested a review from Pytal September 7, 2023 08:23
@GretaD
Copy link
Contributor

GretaD commented Sep 7, 2023

you can Dissmiss changes requested by pytal, if its delivered and they dont review it on time

@ShGKme ShGKme force-pushed the fix/stop-keyboard-navigation branch from 4c7ebb4 to da9582d Compare September 7, 2023 15:43
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the fix/stop-keyboard-navigation branch from da9582d to 2cb60f2 Compare September 7, 2023 15:56
@ShGKme
Copy link
Contributor Author

ShGKme commented Sep 7, 2023

Rebased onto master with fixtup, no other changes

@ShGKme ShGKme merged commit 365cbad into master Sep 7, 2023
16 checks passed
@ShGKme ShGKme deleted the fix/stop-keyboard-navigation branch September 7, 2023 15:59
@Pytal Pytal mentioned this pull request Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: app-navigation Related to the app-navigation component feature: app-sidebar Related to the app-sidebar component feature: popover Related to the popovermenu component feature: select Related to the NcSelect* components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BITV]: Not possible to navigate the tabs in sidebar
4 participants