Un-revert "Add `loading` prop for `Button` and `IconButton` (#3582)"

[mperrotti]

Brings back https://github.com/primer/react/pull/3582

---

Copied from #3582:

Adds a loading state to both Button and IconButton. The markup follows the recommended path forward from accessibility design, and mirrors the implementation from the Export CSV button as part of GH org security coverage pages.

Implementation details

• We use a visually hidden label instead of changing the text of the button to handle text announcements better, and to not change the size of the button.
• aria-disabled instead of disabled to maintain focus management

New props: loading: boolean loadingAnnouncement: default to Loading or provide a custom message to be announced on ATs

Closes https://github.com/github/primer/issues/2680

Screenshots

https://github.com/primer/react/assets/18661030/426caee6-4cb6-478f-b5f8-a84ba26e356e

https://github.com/primer/react/assets/18661030/67d49170-974b-4ed3-8e1b-022a6feabfca

Merge checklist

• [ ] Added/updated tests
• [ ] Added/updated documentation
• [ ] Changes are SSR compatible
• [ ] Tested in Chrome
• [ ] Tested in Firefox
• [ ] Tested in Safari
• [ ] Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 85c68a63c4ecfc9317cf7653fbf2f489ed638696

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

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

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

[github-actions[bot]]

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	94.76 KB (+2.41% 🔺)
packages/react/dist/browser.umd.js	94.96 KB (+2.4% 🔺)

[siddharthkp]

Hi! There is another bug when aria-label is present, tried to address that in this PR (not merged): https://github.com/primer/react/pull/4459

Should I copy those changes over here as well?

[mperrotti]

@siddharthkp - yes, please copy those changes over as well

[siddharthkp]

Sorry, while copying over my changes, I ran into a couple questions

On this line: https://github.com/primer/react/pull/4485/files#diff-515525e4a59d9a55e13c6c60155ec6782c78a026aae4c540c324cd8e11c83a48R102-R105,

aria-labelledby is needed because the accessible name becomes unset when the button is in a loading state.

Looking at this comment, is this still true? It looks like we set aria-describedby not label aria-label or aria-labelledby 🤔

[mperrotti]

@siddharthkp I'm not sure I understand your question because we set all three depending on the situation.

We set aria-label for IconButton (see Storybook example)

We set aria-labelledby for either of these cases:

• loading prop is true
• aria-labelledby prop has been passed

We set aria-describedby for either of these cases:

• loading prop is true
• aria-describedby prop has been passed

[siddharthkp]

Sorry, bad choice of words from me. I'll try again

Looking at this block of code: https://github.com/primer/react/pull/4485/files#diff-515525e4a59d9a55e13c6c60155ec6782c78a026aae4c540c324cd8e11c83a48R102-R105:

// aria-labelledby is needed because the accessible name becomes unset when the button is in a loading state.
// We only set it when the button is in a loading state because it will supercede the aria-label when the screen
// reader announces the button name.
aria-labelledby={loading ? `${uuid}-label` : ariaLabelledBy}

and wondering why/when accessible name becomes unset when the button is in a loading state?

I removed it that line and saw no changes which really confused me. I'm probably missing something very obvious here, thanks for your patience with me!

[mperrotti]

Thanks @siddharthkp

I removed it that line and saw no changes which really confused me. I'm probably missing something very obvious here, thanks for your patience with me!

I had the same experience in the browser, but it caused the following test to fail:

 ● Button › should preserve the accessible button name when the button is in a loading state

    expect(element).toHaveAccessibleName()

    Expected element to have accessible name:
      content
    Received:

      248 |     const container = render(<Button loading>content</Button>)
      249 |
    > 250 |     expect(container.getByRole('button')).toHaveAccessibleName('content')
          |                                           ^
      251 |   })
      252 | })

[siddharthkp]

I had the same experience in the browser, but it caused the following test to fail:

Oh that's rough! Ideally, I'd like to optimise it for the browser and fix/workaround the jest error. UNLESS, we also see similar jest errors in dotcom integration tests?

cc @joshblack @broccolinisoup @pksjce (nerd sniping 🤭) have you seen something like this before?

[TylerJDev]

I removed it that line and saw no changes which really confused me.

It looks like when the button is in a loading state, the text is visually hidden via visibility: hidden. This means that it will be ignored in the accessibility tree and won't be used as the accessible name. Usage of aria-labelledby is valid on elements that use visibility: hidden which is why this works only when aria-labelledby is used.

I'm thinking that there are only a few options here, one being the current pattern using the aria-labelledby. We could also hide the text without using visibility: hidden, but I'm not sure if this is done anywhere in PRC currently.

[mperrotti]

I'm thinking that there are only a few options here, one being the current pattern using the aria-labelledby. We could also hide the text without using visibility: hidden, but I'm not sure if this is done anywhere in PRC currently.

We have a component for that: https://github.com/primer/react/blob/main/packages/react/src/internal/components/VisuallyHidden.tsx

[mperrotti]

@siddharthkp - do you think @TylerJDev 's suggestion will fix our problem?

[TylerJDev]

@siddharthkp - do you think @TylerJDev 's suggestion will fix our problem?

I believe the problem would still persist here if there's no ariaLabelledBy being passed. I'd say utilizing aria-labelledby as you're doing now is definitely a way to go about it, while also adding the ariaLabelledBy prop to the first condition.

We have a component for that: https://github.com/primer/react/blob/main/packages/react/src/internal/components/VisuallyHidden.tsx

This would actually be a good alternative! We wouldn't need to reference the inner span anymore if we used this, and it should be visually equivalent to visibility: hidden. I might prefer this route, as we wouldn't need to handle aria-labelledby directly anymore.

[lindseywild]

@mperrotti is there anything the a11y v-team can help with to get this PR along the finish line?

[mperrotti]

@lindseywild - PR reviews would be super helpful. The only thing that would be more helpful would be to take over this PR from me and get it merged 😂

[lindseywild]

@mperrotti Thanks! I see that Kate and Tyler both weighed in already (they're both on the v-team 😄 ) so let me know if you need anything else once their feedback is addressed!

[mperrotti]

@TylerJDev and @khiga8 - I made all the requested changes. Would you mind taking another look?

[siddharthkp]

👋 Wanted to test with dotcom but would have to wait until integration tests are clean: https://github.com/github/github/pull/329278

[siddharthkp]

Update: ~@mperrotti~ Ignore for now

---

Created a new integration branch in gh/gh and seeing CI failures: https://github.com/github/github/pull/330714

It's unlikely that they are all related to this PR 🤔

Update: They are not, Josh is working on them (https://github.com/primer/react/issues/4683 + https://github.com/github/github/pull/329788), will update integration PR tomorrow!

[siddharthkp]

@mperrotti New integration PR: https://github.com/github/github/pull/331055

I see a few failing tests. Can you please confirm if these are related to the PR?

[mperrotti]

These axe checks are failing because of a color change on disabled buttons: https://github.com/github/github/actions/runs/9713401583/job/26810192180?pr=331055

I'll push up a fix and open a new integration test PR.

[mperrotti]

These failures don't seem related to buttons at all:

• https://github.com/github/github/actions/runs/9713401583/job/26810188969?pr=331055#step:15:10043
• https://github.com/github/github/actions/runs/9713401583/job/26810188969?pr=331055#step:15:10056

[mperrotti]

I think the failed VRT was a fluke because I tested it in local and prod Storybook, and both match what we get in the "Actual" screenshot. Going to update snapshots.

[github-actions[bot]]

Uh oh! @mperrotti, the image you shared is missing helpful alt text. Check https://github.com/primer/react/pull/4485#issuecomment-2207152758.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

[mperrotti]

@siddharthkp @TylerJDev - I tried opening a new integration test PR with these changes, but I keep getting a mysterious failure in the commit-and-push job. Latest attempt: https://github.com/github/github/actions/runs/9783972307

[github-actions[bot]]

Uh oh! @mperrotti, the image you shared is missing helpful alt text. Check https://github.com/primer/react/pull/4485#issuecomment-2207152758.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

[siddharthkp]

@siddharthkp @TylerJDev - I tried opening a new integration test PR with these changes, but I keep getting a mysterious failure in the commit-and-push job. Latest attempt: https://github.com/github/github/actions/runs/9783972307

Looking at the logs, I think It failed only on the last step to ping you on slack, it should have created the PR before that though

(Will check if github actions supports "optional steps" so it doesn't look like a failed workflow)

[mperrotti]

@siddharthkp - that's what I thought too, but I don't see any PR created https://github.com/github/github/pulls?q=is%3Apr+is%3Aopen+Integration+tests+for+PRC

[siddharthkp]

ui/packages with failing tests in CI:

- @github-ui/filter (timeout, seems unrelated)
- @github-ui/pull-request-viewer (consistent, probably related)

Triaging them now

---

Update:

Error is on this line: https://github.com/github/github/blob/prc-integration-test-for-4485/ui/packages/pull-request-viewer/components/diffs/tests/DiffFileHeaderListView.test.tsx#L323

expect(fileLevelCommentsButton).toHaveTextContent('9')

Notice the text content within trailingVisual is missing in integration branch

on master:

<span data-component="buttonContent" class="Box-sc-g0xbh4-0 kkrdEu">
  <span data-component="leadingVisual" class="Box-sc-g0xbh4-0 trpoQ">
    <svg aria-hidden="true" focusable="false" role="img" class="octicon octicon-comment" viewBox="0 0 16 16" width="16"
      height="16" fill="currentColor">
      ...
    </svg>
  </span>
  <span data-component="trailingVisual" class="Box-sc-g0xbh4-0 trpoQ">
    <span aria-hidden="true" data-component="ButtonCounter" class="Box-sc-g0xbh4-0 fnWtqd">9</span>
    <span class="_VisuallyHidden__VisuallyHidden-sc-11jhm7a-0 rTZSs">&nbsp;(9)</span>
  </span>
</span>


on integration branch:

<span data-component="buttonContent" class="Box-sc-g0xbh4-0 kkrdEu">
  <span data-component="leadingVisual" class="Box-sc-g0xbh4-0 trpoQ">
    <svg aria-hidden="true" focusable="false" role="img" class="octicon octicon-comment" viewBox="0 0 16 16" width="16"
      height="16" fill="currentColor">
      ...
    </svg>
  </span>
</span>

---

The button in question is in the new pull request view on a file header: https://github.com/github/github/blob/prc-integration-test-for-4485/ui/packages/pull-request-viewer/components/diffs/diff-file-header-list-view/FileConversationsButton.tsx#L224-L231

<Button
  ref={conversationsListButtonRef}
  aria-label={fileConversationsButtonAriaLabel()}
  count={commentsCount}
  leadingVisual={CommentIcon}
  sx={{mr: 2, flexShrink: 0}}
  onClick={handleConversationListOpen}
/>

The text in the trailing visual comes from the count

Will try to reproduce it in primer/react next

---

Update:

Found it! See next comment for summary

[siddharthkp]

Oof! Took a journey (see previous comment) but found the bug!

Bug 1:

<Button leadingVisual={CommentIcon} count={3} />

on main	on branch

Related line: https://github.com/primer/react/pull/4485/files#diff-515525e4a59d9a55e13c6c60155ec6782c78a026aae4c540c324cd8e11c83a48R132 (there are no children, only count)

Turns out, we didn't have a story (and a visual regression test) for this use case. I've added a story to both main and this branch so that we can have visual regression tests for this case: https://github.com/primer/react/pull/4763

---

Bug 2:

Looking at the snapshots more closely, the space in the ActionMenu stories is missing:

https://github.com/primer/react/pull/4485/files?short_path=a21a44a#diff-e2a5677897b30a0ae510db2637803f d0e9bc24e67e8d3e427c48f328accb88bd

Before	After