Skip to content

Conversation

@pksjce
Copy link
Contributor

@pksjce pksjce commented Sep 15, 2025

Closes ##6752

Changelog

The details component seems to swallow the ref value here for some reason. The button ref really doesn't get set here. There seems no issue with the IconButton ref as I initially assumed. I had to workaround to find the right summary element for this feature to work well.

Before

Screen.Recording.2025-09-15.at.11.23.12.pm.mov

After

Screen.Recording.2025-09-15.at.11.41.26.pm.mov

New

Changed

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Copilot AI review requested due to automatic review settings September 15, 2025 13:44
@pksjce pksjce requested a review from a team as a code owner September 15, 2025 13:44
@pksjce pksjce requested a review from joshblack September 15, 2025 13:44
@changeset-bot
Copy link

changeset-bot bot commented Sep 15, 2025

🦋 Changeset detected

Latest commit: c112cf2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@primer/react Patch
@primer/styled-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where the escape button couldn't properly focus on the menu button in the Breadcrumbs component. The problem was that the Details component was preventing the ref from being properly set on the button element.

  • Updated ref type from HTMLElement to HTMLButtonElement for better type safety
  • Implemented a workaround to find the correct summary element using DOM querying instead of direct ref assignment
  • Removed the ref prop from the IconButton since the ref is now set via DOM querying

@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Sep 15, 2025
@github-actions
Copy link
Contributor

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

Fixes an issue where the ESC button could not focus on the menubutton reference in Breadcrumbs.
@github-actions github-actions bot requested a deployment to storybook-preview-6854 September 15, 2025 13:48 Abandoned
@github-actions
Copy link
Contributor

github-actions bot commented Sep 15, 2025

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 89.48 KB (-0.16% 🔽)
packages/react/dist/browser.umd.js 89.71 KB (+0.17% 🔺)

Copy link
Member

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

the reason the ref is getting dumped is because the IconButton does not include the forwardedRef if the button has an external tooltip. Can we work around this instead of hacking a querySelector? options:

  • put the ref on the tooltip in Breadcrumbs.tsx, like the IconButton does
  • use the IconButton api to render the tooltip, instead of rendering it in Breadcrumbs.tsx

@pksjce
Copy link
Contributor Author

pksjce commented Sep 17, 2025

Hi, So this is the route I first went through to fix this issue.

  1. IconButton does include the forwardedRef. With standalone IconButton I had zero issues with ref. Even with the as="summary".
  2. The issue here is how Details component works. The component renders a different summary component before it recognises the one that I've passed in.

Maybe we could pair on this tomorrow and find a different solution?

@joshblack joshblack removed their request for review September 17, 2025 19:29
@pksjce pksjce enabled auto-merge September 18, 2025 04:03
@pksjce pksjce added this pull request to the merge queue Sep 22, 2025
Merged via the queue into main with commit dd8eeed Sep 22, 2025
40 of 41 checks passed
@pksjce pksjce deleted the pk/breadcrumbs-icon-button-fix branch September 22, 2025 17:42
@primer primer bot mentioned this pull request Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants