fix(NcModal): prevent focus trap race condition

[nikhil2297]

• resolves useFocusTrap: fallbackFocus was specified but was not a node, or did not return a node #8090

☑️ Resolves

• Fix focus trap race condition where fallbackFocus error occurs when onMounted calls useFocusTrap() before template refs are ready
• Reference : useFocusTrap: fallbackFocus was specified but was not a node, or did not return a node #8090

🖼️ Screenshots

🏚️ Before	🏡 After
Focus trap error: fallbackFocus was specified but was not a node occurring ~30% of the time during modal mounting	Error prevented by null check - focus trap waits for DOM elements to be ready

🚧 Tasks

• Add null check in useFocusTrap() to prevent race condition
• Add test cases to verify the fix
• Ensure existing functionality is preserved

🏁 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

[susnux]

Should we not also remove the onMounted(() => useFocusTrap()) ?

[nikhil2297]

Should we not also remove the onMounted(() => useFocusTrap()) ?

I added a null check in useFocusTrap() which fixes the issue and the useFocusTrap() already has guards for edge cases, so there's no downside to keeping the onMounted() call.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 54.41%. Comparing base (e00f649) to head (96de871).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
src/components/NcModal/NcModal.vue	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8093      +/-   ##
==========================================
- Coverage   54.57%   54.41%   -0.17%     
==========================================
  Files         104      104              
  Lines        3399     3398       -1     
  Branches      992      992              
==========================================
- Hits         1855     1849       -6     
- Misses       1305     1309       +4     
- Partials      239      240       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[susnux]

so there's no downside to keeping the onMounted() call.

Well its a useless function call as it will always fail 😉

But anyway: Can you please fix the failing linter?

[nikhil2297]

so there's no downside to keeping the onMounted() call.

Well its a useless function call as it will always fail 😉

But anyway: Can you please fix the failing linter?

Perfect, I've fixed the linter part and did the npm run lint test everything is good. But I'm skeptical about the new line which I have added at EOF but idk why I'm not able to see it in the github commit file changes.

[susnux]

Still think that the known-to-fail onMounted call should be removed but the fix itself seems to work fine!

[Antreesy]

Otherwise good. Although I'm also not seeing a point in keeping onMounted hook

[nikhil2297]

This change doesn’t skip focus trap initialization if maskElement isn’t available it defers it until the element is present.

Changes:

• Removed the onMounted call.
• Added a watcher to initialize the focus trap when maskElement becomes available
• Kept the null check for maskElement to handle cases where NcModal is used with v-if

[Sector6759]

Would this fix #7946?

[susnux]

For @ShGKme I pushed one commit that solves the review comment.
Meaning focus trap is managed by the transition events, no need to custom safe guards or lifecycle events.
Except for unmounting, we need to enforce its removed on unmount.

[susnux]

/backport to stable8

[backportbot]

The backport to stable8 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable8
git pull origin stable8

# Create the new backport branch
git checkout -b backport/8093/stable8

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 82881d9e d9e5508f 96de8716

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/8093/stable8

Error: Failed to check for changes with origin/stable8: No changes found in backport branch

---

Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.