Skip to content

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

Merged
ShGKme merged 7 commits into
masterfrom
fix/stop-keyboard-navigation
Sep 7, 2023
Merged

Stop propagation of keyboard navigation in a number of components#4501
ShGKme merged 7 commits into
masterfrom
fix/stop-keyboard-navigation

Conversation

@ShGKme

@ShGKme ShGKme commented Sep 4, 2023

Copy link
Copy Markdown
Contributor

☑️ 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
Comment thread src/components/NcSelect/NcSelect.vue Outdated
@ShGKme
ShGKme requested a review from Pytal September 7, 2023 08:23
@GretaD

GretaD commented Sep 7, 2023

Copy link
Copy Markdown
Contributor

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

ShGKme commented Sep 7, 2023

Copy link
Copy Markdown
Contributor Author

Rebased onto master with fixtup, no other changes

@ShGKme
ShGKme enabled auto-merge September 7, 2023 15:57
@ShGKme
ShGKme merged commit 365cbad into master Sep 7, 2023
@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