Skip to content

Conversation

@Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Aug 12, 2025

☑️ Resolves

  • Follow-up to fix(NcModal): correctly handle when trying to activate non-existing focus-trap #7084
    • NcModal is working fine with default trap stack, so there's no need for useTrapStackControl
  • To be backported with [stable8] fix(NcModal): use useTrapStackControl for pausing other focus traps #7085
    • Does not change behaviour for main / Vue 3
    • Fixes following scenario for Vue 2:
      • Talk app -> shrink to mobile -> NcAppNavigation creates trap 0 -> trap 0 is active
      • open NcActions next to search bar -> pause trap 0
      • select 'Create open conversation' -> starts to mount NcModal
      • NcModal mounted -> useFocusTrap() -> creates trap 1 -> trap 1 is active
      • NcActions with close-after-click is closed -> unpause pausedStack (it has only [trap 0] from first step) -> trap 0 unpauses
      • trap 0 activates - trap 1 is paused -> focus trap is now in NcAppNavigation instead of NcModal
    • If trap stack was modified outside of stackcontroller, it should be safe to assume, that user action lead to that, and it's currently correctly resolved

🖼️ Screenshots

🏚️ Before (Vue 2 at #7085)

2025-08-12_12h39_00.mp4

🏡 After (Vue 2)

2025-08-12_12h34_55.mp4

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 2️⃣ Backport to stable8 for maintained Vue 2 version or not applicable

@Antreesy Antreesy added this to the 9.0.0-rc.6 milestone Aug 12, 2025
@Antreesy Antreesy requested review from ShGKme and susnux August 12, 2025 10:44
@Antreesy Antreesy self-assigned this Aug 12, 2025
@Antreesy Antreesy added bug Something isn't working 3. to review Waiting for reviews labels Aug 12, 2025
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.

Code makes sense to me

@susnux
Copy link
Contributor

susnux commented Aug 18, 2025

this one can be backported to fix the stable8 issue, right?

Comment on lines 48 to 49
* If the actual stack is different from the paused one, there were changes
* outside of this controller, so we assume it's self-regulated and do not unpause.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an explanation here? Why we only need to unpause if the length is the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the opposite of what is already written, do we need additional explanation here? I currently don't see how it could be expanded, please, advise

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just align with code?

Suggested change
* If the actual stack is different from the paused one, there were changes
* outside of this controller, so we assume it's self-regulated and do not unpause.
* Only if the actual stack is the same as the paused one, there were no changes
* outside of this controller, so we can save assume it's not self-regulated and we need to unpause.

But I also think the current comment is already explaining it well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will put PR description into composable comment, so devs can read and act from there at least. As it's not blocking, we should be able to follow-up with better explanation

…de of controller

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/7084/unpause-autoresolve branch from f9a39bb to 5b0d865 Compare August 18, 2025 17:05
@susnux susnux merged commit d6edf83 into main Aug 18, 2025
25 checks passed
@susnux susnux deleted the fix/7084/unpause-autoresolve branch August 18, 2025 17:24
@susnux
Copy link
Contributor

susnux commented Aug 18, 2025

/backport to stable8

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants