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(NcModal): temporary deactivate focus-traps on modal open #5783

Merged
merged 3 commits into from
Jul 22, 2024

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Jul 5, 2024

☑️ Resolves

Test cases:

Normal (works fine) Mobile (has issues)
image image

Important

  1. Navigation footer: Button to open Modal:
    works correctly from keyboard and mouse click ✅

Important

2. Navigation header: Actions -> ActionButton (single button, don't force menu):
after closing the modal (with ESC) focus jumps to header input for a second, then to ActionButton
keyboard and mouse works fine ✅

Important

3. Navigation header: Actions -> ActionButton (close after click):
can't focus input, focus trap is inside NcAppNavigation 🐞

Important

4. Navigation header: Actions -> ActionButton (handle only click):
after mouse click on button, then on input -> focus jumps to header input 🐞
keyboard works fine

🖼️ Screenshots

🏚️ Before

Screencast.from.18.07.2024.11.30.24.webm

🏡 After

Screencast.from.18.07.2024.11.31.39.webm

🚧 Tasks

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@Antreesy Antreesy added bug Something isn't working 2. developing Work in progress feature: app-navigation Related to the app-navigation component feature: actions Related to the actions components labels Jul 5, 2024
@Antreesy Antreesy added this to the 8.14.1 milestone Jul 5, 2024
@Antreesy Antreesy requested a review from susnux July 5, 2024 09:53
@Antreesy Antreesy removed the feature: app-navigation Related to the app-navigation component label Jul 5, 2024
@Antreesy Antreesy force-pushed the fix/5385/focus-trap-mobile branch from 5ef58f6 to 8023e46 Compare July 5, 2024 10:40
@Antreesy
Copy link
Contributor Author

Antreesy commented Jul 5, 2024

Referred to the library demo and other examples in vue-lib
Don't know what I can break with it, so waiting on expert review

@Antreesy Antreesy requested review from skjnldsv and ShGKme July 5, 2024 10:43
@Antreesy Antreesy self-assigned this Jul 5, 2024
@Antreesy Antreesy changed the title wip(NcAppNavigation): provide reproducible example for focus-trap bug fix(NcModal): temporary deactivate focus-traps on modal open Jul 5, 2024
@Antreesy Antreesy added feature: app-navigation Related to the app-navigation component feature: modal Related to the modal component and removed feature: actions Related to the actions components labels Jul 5, 2024
@Antreesy Antreesy force-pushed the fix/5385/focus-trap-mobile branch from 8023e46 to 1f80896 Compare July 18, 2024 10:10
@Antreesy Antreesy added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 18, 2024
@Antreesy Antreesy marked this pull request as ready for review July 18, 2024 10:10
@susnux susnux modified the milestones: 8.14.1, 8.15.0 Jul 22, 2024
@susnux susnux merged commit 3fb2836 into master Jul 22, 2024
20 checks passed
@susnux susnux deleted the fix/5385/focus-trap-mobile branch July 22, 2024 15:29
@susnux
Copy link
Contributor

susnux commented Jul 22, 2024

@Antreesy we need to backport, right?

@Antreesy
Copy link
Contributor Author

/backport to next

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working feature: app-navigation Related to the app-navigation component feature: modal Related to the modal component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants