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

fix(files_sharing): select sharing in sidebar breaks Viewer focus trap #40273

Merged

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Sep 5, 2023

Summary

What's happening in the mentioned PR:

  1. User opens select
  2. SharingInput emits global event viewer:trapElements:changed sending the NcSelect instance
  3. Viewer handles this event and adds this NcSelect instance to the @nextcloud/vue/NcModal additional trap elements

Why there was a problem:

Each time a select is open, a global event triggers viewer which then adds this select as additionalTrapElement. As the result:

  1. 10 times opened select adds 10 selects to additional trap elements
  2. select becomes a new tabbableGroup which is supposed to be already a part of sidebar group.

On first open it works fine. On the second and following it breaks. When a user is in the open select again and presses TAB the following happens:

  1. focus-trap searches the first tabbableGroup with select
  2. Then searches for the next element in this group (nothing found). Then search for the next tabbableGroup
  3. Next tabbableGroup is the a group with select again.
  4. focus-trap focuses the same select again.

image

See also: https://github.com/focus-trap/focus-trap/blob/c68a1ca87f2dfd905d9f97b91e2d0318268b5ea3/index.js#L529-L535

Why the fix from the mentioned PR is not needed now:

It is fixed globally by nextcloud-libraries/nextcloud-vue#4406 (see: nextcloud-libraries/nextcloud-vue@f6a6de1).

When additionalTrapElements are taken into account on init, select works as a part of the sidebar which is already added into additionalTrapElements.

TODO

  • Remove emitting viewer:trapElements:changed on each sharing select open in sidebar

Screenshots

Before After
Tab forward gets stuck on select Tab forward goes forward
Tab backward ends on select Tab backward goes backward
sharing-before sharing-after

Alternative or additional solution

In NcModal or/and Viewer we can check that there are no duplications in additionalTrapElements.

Folowing-up

There is still a problem with a focus on "Copy internal link button". Not related to this PR, but could be a related problem...

Checklist

@ShGKme ShGKme added bug 2. developing Work in progress feature: file sidebar Related to the file sidebar component accessibility labels Sep 5, 2023
@ShGKme ShGKme self-assigned this Sep 5, 2023
@ShGKme ShGKme changed the title [WIP] fix(files_sharing): remove adding select to focus trap on open fix(files_sharing): select sharing in sidebar breaks Viewer focus trap Sep 5, 2023
@ShGKme ShGKme added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 5, 2023
@ShGKme ShGKme force-pushed the fix/39434/sharing-select-breaking-focus-trap-with-viewer branch from 43bcb0f to 9c70924 Compare September 5, 2023 15:51
@ShGKme ShGKme marked this pull request as ready for review September 5, 2023 15:52
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the fix/39434/sharing-select-breaking-focus-trap-with-viewer branch from 9c70924 to cc7d155 Compare September 5, 2023 20:20
Copy link
Member

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

🤯

@Pytal Pytal added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 5, 2023
@Pytal Pytal merged commit 9df4da8 into master Sep 5, 2023
41 checks passed
@Pytal Pytal deleted the fix/39434/sharing-select-breaking-focus-trap-with-viewer branch September 5, 2023 23:34
@AndyScherzinger AndyScherzinger added this to the Nextcloud 28 milestone Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish accessibility bug feature: file sidebar Related to the file sidebar component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BITV]: Focus trap on search field doesn't work well
4 participants