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 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 52.77%. Comparing base (7865f41) to head (36e72ac).

Files with missing lines	Patch %	Lines
src/components/NcModal/NcModal.vue	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8093      +/-   ##
==========================================
- Coverage   52.77%   52.77%   -0.01%     
==========================================
  Files         103      103              
  Lines        3348     3350       +2     
  Branches      976      978       +2     
==========================================
+ Hits         1767     1768       +1     
- Misses       1333     1334       +1     
  Partials      248      248              

☔ 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?