Skip to content

feat(NcModal): expose initialFocus prop for focus-trap options#7587

Open
Antreesy wants to merge 1 commit intomainfrom
fix/noid/ncmodal-initial-focus
Open

feat(NcModal): expose initialFocus prop for focus-trap options#7587
Antreesy wants to merge 1 commit intomainfrom
fix/noid/ncmodal-initial-focus

Conversation

@Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Oct 1, 2025

☑️ Resolves

🖼️ Screenshots

🚧 Tasks

🏁 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.1 milestone Oct 1, 2025
@Antreesy Antreesy self-assigned this Oct 1, 2025
@Antreesy Antreesy added enhancement New feature or request 3. to review Waiting for reviews feature: modal Related to the modal component labels Oct 1, 2025
@Antreesy Antreesy force-pushed the fix/noid/ncmodal-initial-focus branch from c834c99 to 17df305 Compare October 1, 2025 08:42
@susnux susnux modified the milestones: 9.0.1, 9.0.2 Oct 6, 2025
@susnux susnux modified the milestones: 9.0.2, 9.1.0 Oct 14, 2025
@susnux susnux modified the milestones: 9.1.0, 9.2.0 Nov 4, 2025
@ShGKme ShGKme modified the milestones: 9.2.0, 9.3.0 Nov 14, 2025
@susnux susnux modified the milestones: 9.3.0, 9.4.0 Nov 20, 2025
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/noid/ncmodal-initial-focus branch from 17df305 to 8afa991 Compare November 25, 2025 15:51
@codecov
Copy link

codecov bot commented Nov 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.20%. Comparing base (49763e7) to head (8afa991).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7587   +/-   ##
=======================================
  Coverage   52.20%   52.20%           
=======================================
  Files         101      101           
  Lines        3174     3174           
  Branches      871      872    +1     
=======================================
  Hits         1657     1657           
  Misses       1271     1271           
  Partials      246      246           

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

@Antreesy
Copy link
Contributor Author

I forgot about that PR🪦 Shall we get it in?

@DorraJaouad
Copy link
Contributor

Was it for the translation dialog in chat?

@Antreesy
Copy link
Contributor Author

Antreesy commented Dec 3, 2025

Yes, in general it fits for any dialog that does not need to have an initial focus on the child element

@ShGKme
Copy link
Contributor

ShGKme commented Dec 4, 2025

  • (e.g. in case first element in the trap is NcSelect and we don't want it to be open)

With a small CSS fix, and by using available props and events we can make NcSelect open only manually but not by focus.

Would it also work for you?

@Antreesy
Copy link
Contributor Author

Antreesy commented Dec 5, 2025

we can make NcSelect open only manually but not by focus. Would it also work for you?

Depends on how it changes current default behaviour, and what app developers should do to implement it. I'd take a look =)

@susnux susnux modified the milestones: 9.4.0, 9.5.0 Jan 22, 2026
@susnux susnux removed this from the 9.5.0 milestone Feb 6, 2026
@susnux susnux added this to the 9.6.0 milestone Feb 6, 2026
@susnux
Copy link
Contributor

susnux commented Feb 6, 2026

Whats the current state of this PR?

@Antreesy
Copy link
Contributor Author

Antreesy commented Feb 6, 2026

Whats the current state of this PR?

Ready to be merged for two months already 👀

@susnux
Copy link
Contributor

susnux commented Feb 6, 2026

What I am not sure about is whether we should focus the first element.
Depending on how to interpret the accessibility guidelines it even makes sense to focus the dialog itself.

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.

IMHO this feature makes sense in some cases, for those I approve.
But if its really only about NcSelect then we can also fix that component

/** Additional elements to add to the focus trap */
additionalTrapElements?: Array<string | HTMLElement>

/** Set element to return focus to after focus trap deactivation */
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better

Suggested change
/** Set element to return focus to after focus trap deactivation */
/** Set element to return focus to after dialog was closed */

/** Set element to return focus to after focus trap deactivation */
setReturnFocus?: FocusTargetOrFalse

/** Specify an element to receive initial focus after focus trap activation */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** Specify an element to receive initial focus after focus trap activation */
/** Specify an element to receive initial focus after dialog was opened */

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 enhancement New feature or request feature: modal Related to the modal component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants