Skip to content

refactor: add useSetFocus controller#11943

Merged
jcfranco merged 41 commits intodevfrom
jcfranco/add-setFocus-controller
Jul 22, 2025
Merged

refactor: add useSetFocus controller#11943
jcfranco merged 41 commits intodevfrom
jcfranco/add-setFocus-controller

Conversation

@jcfranco
Copy link
Copy Markdown
Member

@jcfranco jcfranco commented Apr 11, 2025

Related Issue: N/A

Summary

This adds a controller for consistent and simpler setFocus behavior across components.

Noteworthy changes

  • updates dom#focusElement() to use focusFirstTabbable(el) instead of calling el.focus() if there's no setFocus() available and adds context argument to allow short-circuiting focus logic
  • adds includeContainer to dom#focusFirstTabbable, dom#getFirstTabbable for focus target scope
  • updates dom#focusElementInGroup() to support includeContainer and whether to consider the target element as the focus context
  • adds internal dom#focusFirstFocusable, dom#getFirstFocusable for additional focus options – can expose if deemed useful
  • automatically skips focusing if the component is disabled
    • converts interactive.spec.ts test to interactive.browser.spec.tsx to support ☝️
  • setFocus controller supports customization for advanced use cases, such as
    • defining focus strategy – use first-tabbable (default) vs first-focusable (both from tabbable)
    • setting target scope – excludes container (default) or not when looking for first tabbable/focusable
  • complex focusing cases will still work, but will be external to the controller (e.g., list's focusCell calls).
  • for granular or custom, internal focusing, use focusElement instead of this.setFocus, unless that behavior is desired

Next steps

  • refactor utils to use object parameters for clarity as they are getting complex
  • refactor external focus logic to use setFocus controller, if possible

@github-actions github-actions Bot added the refactor Issues tied to code that needs to be significantly reworked. label Apr 11, 2025
@jcfranco jcfranco force-pushed the jcfranco/add-setFocus-controller branch from 3315ad4 to c19c13b Compare April 11, 2025 21:39
@github-actions
Copy link
Copy Markdown
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions Bot added the Stale Issues or pull requests that have not had recent activity. label Apr 22, 2025
@jcfranco jcfranco force-pushed the jcfranco/add-setFocus-controller branch from c19c13b to 33f8d19 Compare April 30, 2025 20:33
@jcfranco jcfranco changed the title refactor: add setFocus controller refactor: add useSetFocus controller Apr 30, 2025
@jcfranco jcfranco force-pushed the jcfranco/add-setFocus-controller branch from 33f8d19 to 8cae01f Compare April 30, 2025 20:49
@jcfranco jcfranco added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Apr 30, 2025
@jcfranco
Copy link
Copy Markdown
Member Author

Went with focusSetter as the default name for the internal prop holding the controller export, but open to suggestions.

@github-actions github-actions Bot removed the Stale Issues or pull requests that have not had recent activity. label May 4, 2025
@github-actions
Copy link
Copy Markdown
Contributor

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions Bot added the Stale Issues or pull requests that have not had recent activity. label May 16, 2025
@github-actions github-actions Bot removed the Stale Issues or pull requests that have not had recent activity. label Jul 5, 2025
Copy link
Copy Markdown
Contributor

@aPreciado88 aPreciado88 left a comment

Choose a reason for hiding this comment

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

This LGTM!

@jcfranco jcfranco added skip visual snapshots Pull requests that do not need visual regression testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jul 22, 2025
Copy link
Copy Markdown
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

This is nice! awesome ⭐

Comment thread packages/calcite-components/src/controllers/useSetFocus.ts Outdated
Comment thread packages/calcite-components/src/controllers/useSetFocus.ts
@eriklharper
Copy link
Copy Markdown
Contributor

Went with focusSetter as the default name for the internal prop holding the controller export, but open to suggestions.

Yeah, I've been thinking it would be good to standardize the naming structure for the internal component properties meant for storing controller instances. I feel like the word "controller" in the name, however verbose, could be nice because it is direct? I think maybe the name of the controller, plus the word "controller" could be the most direct way we could adopt, so it would be come useSetFocusController.

Comment thread packages/calcite-components/src/controllers/useSetFocus.ts
@jcfranco
Copy link
Copy Markdown
Member Author

Went with focusSetter as the default name for the internal prop holding the controller export, but open to suggestions.

Yeah, I've been thinking it would be good to standardize the naming structure for the internal component properties meant for storing controller instances. I feel like the word "controller" in the name, however verbose, could be nice because it is direct? I think maybe the name of the controller, plus the word "controller" could be the most direct way we could adopt, so it would be come useSetFocusController.

Not sure this is applicable since a controller’s return value can be anything, including nothing.

@jcfranco jcfranco merged commit 706ac8a into dev Jul 22, 2025
10 checks passed
@jcfranco jcfranco deleted the jcfranco/add-setFocus-controller branch July 22, 2025 19:53
@github-actions github-actions Bot added this to the 2025-07-29 - Jul Milestone milestone Jul 22, 2025
@eriklharper
Copy link
Copy Markdown
Contributor

Went with focusSetter as the default name for the internal prop holding the controller export, but open to suggestions.

Yeah, I've been thinking it would be good to standardize the naming structure for the internal component properties meant for storing controller instances. I feel like the word "controller" in the name, however verbose, could be nice because it is direct? I think maybe the name of the controller, plus the word "controller" could be the most direct way we could adopt, so it would be come useSetFocusController.

Not sure this is applicable since a controller’s return value can be anything, including nothing.

True. I also think the usage of controllers across the code base is easily discoverable by searching for the controller imports, so it probably doesn't matter whether we establish naming guidelines for the instances. I do like the name focusSetter because it is communicating the action it is intended to produce.

benelan pushed a commit that referenced this pull request Sep 16, 2025
**Related Issue:** N/A  

## Summary  

This adds a controller for consistent and simpler `setFocus` behavior
across components.

### Noteworthy changes

* updates `dom#focusElement()` to use `focusFirstTabbable(el)` instead
of calling `el.focus()` if there's no `setFocus()` available and adds
`context` argument to allow short-circuiting focus logic
* adds `includeContainer` to `dom#focusFirstTabbable`,
`dom#getFirstTabbable` for focus target scope
* updates `dom#focusElementInGroup()` to support `includeContainer` and
whether to consider the target element as the focus context
* adds _internal_ `dom#focusFirstFocusable`, `dom#getFirstFocusable` for
additional focus options – can expose if deemed useful
* automatically skips focusing if the component is disabled
* converts `interactive.spec.ts` test to `interactive.browser.spec.tsx`
to support ☝️
* `setFocus` controller supports customization for advanced use cases,
such as
* defining focus strategy – use first-tabbable (default) vs
first-focusable (both from `tabbable`)
* setting target scope – excludes container (default) or not when
looking for first tabbable/focusable
* complex focusing cases will still work, but will be external to the
controller (e.g., list's `focusCell` calls).
* for granular or custom, internal focusing, use `focusElement` instead
of `this.setFocus`, unless that behavior is desired

### Next steps

* refactor utils to use object parameters for clarity as they are
getting complex
* refactor external focus logic to use `setFocus` controller, if
possible
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Issues tied to code that needs to be significantly reworked. skip visual snapshots Pull requests that do not need visual regression testing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants