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(NcActions): intercept into current focus trap stack #4953

Merged

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Dec 12, 2023

☑️ Resolves

When the component has its own focus trap, then it is managed by global trap stack by focus-trap.

However if the component has no focus trap and is used inside another focus trap - there is an issue. By default popover content is rendered in body or other container, which is likely outside the current focus trap containers. It results in broken behavior from focus-trap.

We need to pause all the focus traps for opening popover and then unpause them back after closing.

🖼️ Screenshots

🏚️ Before 🏡 After
Header Menu .
before-focus-trap-1 after-focus-trap-1
Sidebar multi-level .
before-focus-trap-2 after-focus-trap-2

Focus trap still works.

Supports multi-level focus traps (header menu focus trap inside sidebar trap):

focus-trap-3

🏁 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 2. developing Work in progress feature: popover Related to the popovermenu component accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Dec 12, 2023
@ShGKme ShGKme self-assigned this Dec 12, 2023
@ShGKme ShGKme added this to the 8.3.1 milestone Dec 12, 2023
@ShGKme ShGKme modified the milestones: 8.3.1, 8.4.1 Dec 27, 2023
@susnux susnux modified the milestones: 8.4.1, 8.5.0 Jan 15, 2024
@Pytal Pytal modified the milestones: 8.5.0, 8.6.0 Jan 23, 2024
@ShGKme ShGKme force-pushed the fix/4910/nc-popover--integrate-with-current-focus-trap branch 2 times, most recently from 9463ffc to 234bec9 Compare January 25, 2024 20:53
@ShGKme ShGKme changed the title fix(NcPopover): integrate with current focus trap fix(NcActions): intercept into current focus trap stack Jan 25, 2024
@ShGKme ShGKme added feature: actions Related to the actions components and removed feature: popover Related to the popovermenu component labels Jan 25, 2024
@ShGKme ShGKme mentioned this pull request Jan 25, 2024
3 tasks
@ShGKme ShGKme force-pushed the fix/4910/nc-popover--integrate-with-current-focus-trap branch from 234bec9 to 7f8fa2d Compare January 25, 2024 21:25
@ShGKme ShGKme added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 25, 2024
@ShGKme ShGKme marked this pull request as ready for review January 25, 2024 21:26
@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 25, 2024

Could be risky. A good review is appreciated 👀

Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

sequence is unfortunately not fully correct ;( -> both directions: forwards and backwards

Peek 2024-01-26 08-15

@JuliaKirschenheuter
Copy link
Contributor

Doesn't work on Photos well ;( -> seems to be a regression, works on try.extcloud

Peek 2024-01-26 09-01

Peek 2024-01-26 09-03

@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 26, 2024

sequence is unfortunately not fully correct ;( -> both directions: forwards and backwards

I cannot reproduce. The gif looks exactly how it was before this PR. Could you check that npm link and rebuild was successful?

On what page what it is recorded?

@ShGKme
Copy link
Contributor Author

ShGKme commented Jan 26, 2024

The issue happens when there are any inline actions. They are also counted in menu actions when determining what kind of menu we have.

Preparing another PR.

@ShGKme ShGKme marked this pull request as draft January 26, 2024 11:38
@ShGKme ShGKme mentioned this pull request Jan 26, 2024
2 tasks
@ShGKme ShGKme force-pushed the fix/4910/nc-popover--integrate-with-current-focus-trap branch from 7f8fa2d to 3297d51 Compare January 26, 2024 21:08
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
@ShGKme ShGKme force-pushed the fix/4910/nc-popover--integrate-with-current-focus-trap branch from 3297d51 to 6060690 Compare January 27, 2024 00:04
@ShGKme ShGKme marked this pull request as ready for review January 27, 2024 00:05
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

works, great!

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

works

@susnux susnux merged commit c39a056 into master Jan 29, 2024
16 checks passed
@susnux susnux deleted the fix/4910/nc-popover--integrate-with-current-focus-trap branch January 29, 2024 17:59
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 accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: actions Related to the actions components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NcActions doesn't work inside an element with a focus trap in a general case
4 participants