Skip to content

Conversation

shleewhite
Copy link
Contributor

@shleewhite shleewhite commented Sep 24, 2025

📌 Summary

This PR upgrades ember-a11y-refocus to v5.1.0, which required a few related dependency, type, and style updates to ensure everything compiles and functions correctly.

Changes

  • Upgrade @glimmer/component to v2.0
    • Required for the EmberA11yRefocusRegistry to correctly import type signatures for NavigationNarrator.
    • Without this, NavigationNarrator would remain untyped.
    • Added a global override in package.json to force v2.0.0. This prevents build failures in Showcase and Website caused by mismatched versions.
  • Fix types for @a11yRefocusRouteChangeValidator arguments
    • In SideNav and AppHeader, this arg was previously typed as string.
    • It now correctly uses (transition: Transition) => boolean since it is forwarded to the NavigationNarrator component.
  • Correct Owner typing in th-resize-handle
    • @glimmer/component@2.0.0 throws if Owner is not typed correctly.
    • Updated the last remaining usage of Owner being typed as unknown.
  • Add NavigationNarrator styles to Website CSS
    • ember-a11y-refocus@5.1.0 no longer auto-imports its styles for the website nav.
    • Explicitly added them via website/ember-cli-build.js and website/app/styles/app.scss following the recommended usage.

Open discussion

There is still a question around what is the best way to handle the ember-a11y-refocus styles. Before, the way the build worked made it so the consumer would install ember-a11y-refocus by installing the design system and that would implicitly import the styles. This is no longer the case, so either we need to export the styles for them or they need to manually import them.

In this instance, we can treat the showcase app as a demo consumer of the design system. The easiest way to verify that the styles are imported are to check the SideNav skip link example in the showcase. If the link has an underline, the styles are not imported.

The solution in this PR is Lee's preference
Ultimately, the main style we need is text-decoration: none and ember-a11y-refocus is not likely to change the styles frequently so I think it is reasonably safe to copy their styles and include them in our exported stylesheet.

Option 2: Require all consumers add ember-a11y-refocus as a dependency

SPIKE PR: #3251

In this option, we don't export the ember-a11y-refocus styles so the consumer has to include the path in their ember-cli-build and add it as a dependency. This approach is similar to how the tokens styles work.

This would require all consumers to update their ember-cli-build file and we'd have to change the guidance on our website. If you don't do this, the design system package builds fine, but the showcase fails to build because it does not have access to the stylesheet.


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

📋 PCI review checklist
  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
  • If applicable, I've worked with GRC to document the impact of any changes to security controls.
    Examples of changes to controls include access controls, encryption, logging, etc.
  • If applicable, I've worked with GRC to ensure compliance due to a significant change to the in-scope PCI environment.
    Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.

@shleewhite shleewhite requested a review from a team as a code owner September 24, 2025 18:18
Copy link

vercel bot commented Sep 24, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Updated (UTC)
hds-showcase Ready Ready Preview Oct 7, 2025 7:47pm
hds-website Ready Ready Preview Oct 7, 2025 7:47pm

@aklkv
Copy link
Collaborator

aklkv commented Sep 24, 2025

we need to bring it styles manually now as addon it self now v2 which means styles will not be auto included https://github.com/ember-a11y/ember-a11y-refocus?tab=readme-ov-file#usage

also my comments on original PR is still valid #2950 (comment)

Copy link
Contributor

@zamoore zamoore left a comment

Choose a reason for hiding this comment

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

Nice!

@zamoore zamoore merged commit 69a6434 into main-5.0.0 Oct 7, 2025
20 of 21 checks passed
@zamoore zamoore deleted the chore/upgrade-ember-a11y-focus branch October 7, 2025 21:46
zamoore pushed a commit that referenced this pull request Oct 7, 2025
Co-authored-by: shleewhite <lee.white@hahicorp.com>
Co-authored-by: Alexey Kulakov <uni_que@me.com>
zamoore pushed a commit that referenced this pull request Oct 8, 2025
Co-authored-by: shleewhite <lee.white@hahicorp.com>
Co-authored-by: Alexey Kulakov <uni_que@me.com>
zamoore pushed a commit that referenced this pull request Oct 9, 2025
Co-authored-by: shleewhite <lee.white@hahicorp.com>
Co-authored-by: Alexey Kulakov <uni_que@me.com>
@zamoore zamoore mentioned this pull request Oct 9, 2025
5 tasks
zamoore pushed a commit that referenced this pull request Oct 10, 2025
Co-authored-by: shleewhite <lee.white@hahicorp.com>
Co-authored-by: Alexey Kulakov <uni_que@me.com>
zamoore pushed a commit that referenced this pull request Oct 10, 2025
Co-authored-by: shleewhite <lee.white@hahicorp.com>
Co-authored-by: Alexey Kulakov <uni_que@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants