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

mperrotti · 2024-04-10T15:02:47Z

Brings back #3582

…)" (#4…" This reverts commit c01901f.

changeset-bot · 2024-04-10T15:02:51Z

🦋 Changeset detected

Latest commit: 4fce38a

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 · 2024-04-10T15:06:18Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	88.91 KB (+1.07% 🔺)
packages/react/dist/browser.umd.js	89.14 KB (+0.97% 🔺)

siddharthkp · 2024-04-10T16:00:22Z

Hi! There is another bug when aria-label is present, tried to address that in this PR (not merged): #4459

Should I copy those changes over here as well?

mperrotti · 2024-04-10T16:18:51Z

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

siddharthkp · 2024-04-12T14:56:03Z

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 · 2024-04-12T17:08:08Z

@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 · 2024-04-16T14:12:23Z

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 · 2024-04-19T20:28:54Z

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 | })

…t-3582

…o revert-4464-revert-3582

siddharthkp · 2024-04-22T13:38:09Z

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?

…t-3582

TylerJDev · 2024-04-24T19:42:20Z

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.

TylerJDev · 2024-04-24T20:59:01Z

packages/react/src/Button/ButtonBase.tsx

+          // 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}


I'm wondering if we could add ariaLabelledBy to the first condition too?

(i.e. ${uuid}-label ${ariaLabelledBy} : ariaLabelledBy)

This allows instances that rely on aria-labelledby to remain covered, even if the button is in a loading state.

Oh I didn't realize we could have 2 values in aria-labelledby. That could work! I'll give it a try...

mperrotti · 2024-04-26T20:19:35Z

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

…g state

…t-3582

mperrotti · 2024-04-26T20:50:40Z

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

TylerJDev · 2024-04-29T16:45:03Z

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

khiga8

Took a look because I am looking at an adjacent topic of buttons with dynamically updated labels! :)

khiga8 · 2024-05-01T13:10:33Z

packages/react/src/Button/Button.examples.stories.tsx

+  return (
+    <>
+      <VisuallyHidden aria-live="polite">{!loading && success ? 'Export completed' : null}</VisuallyHidden>
+      <Button loading={loading} leadingVisual={DownloadIcon} onClick={onClick('error')}>


Suggested change

<Button loading={loading} leadingVisual={DownloadIcon} onClick={onClick('error')}>

<Button loading={loading} leadingVisual={DownloadIcon} onClick={onClick('success')}>

Should this be success? I noticed that the live region doesn't get updated with the Export completed announcement in the storybook success example.

It seems to be getting updated for the error example though!

khiga8 · 2024-05-01T13:13:39Z

packages/react/src/Button/Button.examples.stories.tsx

+  )
+}
+
+export const LoadingStatusAnnouncementError = () => {


In this example, the failure state is communicated to assistive technologies via announcement, but there's no visual indication that anything failed.

Do we have guidance around communicating errors visually? If so, should this example include that?

khiga8 · 2024-05-01T13:26:22Z

packages/react/src/Button/Button.examples.stories.tsx

+
+  return (
+    <>
+      <VisuallyHidden aria-live="polite">{!loading && error ? 'Export failed' : null}</VisuallyHidden>


Should the announcement utility components be used in these examples instead, or are they only recommended for use internally? (cc: @joshblack)

Suggested change

<VisuallyHidden aria-live="polite">{!loading && error ? 'Export failed' : null}</VisuallyHidden>

<VisuallyHidden><Status>{!loading && error ? 'Export failed' : null}</Status></VisuallyHidden>

khiga8 · 2024-05-01T13:33:26Z

packages/react/src/Button/Button.examples.stories.tsx

+
+export default meta
+
+export const LoadingStatusAnnouncementSuccessful = () => {


In these examples, when the loading state is activated, the focus gets lost and removed from the button. I believe this is a 2.4.3 issue, and may result in some screen reader users ending up at the top of the page. Could we avoid focus loss?

lindseywild · 2024-05-16T14:01:06Z

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

Revert "Revert "Add loading prop for Button and IconButton (#3582…

60b3253

…)" (#4…" This reverts commit c01901f.

mperrotti requested review from a team as code owners April 10, 2024 15:02

mperrotti requested review from maximedegreve and siddharthkp April 10, 2024 15:02

mperrotti changed the title ~~Revert "Revert "Add loading prop for Button and IconButton (#3582)""~~ Un-revert "Add loading prop for Button and IconButton (#3582)" Apr 10, 2024

mperrotti temporarily deployed to github-pages April 10, 2024 15:06 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4485 April 10, 2024 15:06 Inactive

only overrides btn label when loading

0c29ea7

mperrotti temporarily deployed to github-pages April 10, 2024 15:44 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4485 April 10, 2024 15:44 Inactive

mperrotti mentioned this pull request Apr 10, 2024

Only sets aria-labelledby to button label node only if loading is true #4450

Closed

11 tasks

siddharthkp added the no-breaking-changes: need to test This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Apr 10, 2024

updates snapshots after merging from main

27a7c58

mperrotti temporarily deployed to github-pages April 10, 2024 17:45 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4485 April 10, 2024 17:46 Inactive

Merge branch 'main' into revert-4464-revert-3582

14c0278

mperrotti temporarily deployed to github-pages April 11, 2024 14:16 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4485 April 11, 2024 14:16 Inactive

Merge branch 'main' into revert-4464-revert-3582

ac0801b

mperrotti temporarily deployed to github-pages April 12, 2024 17:11 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4485 April 12, 2024 17:12 Inactive

mperrotti added 3 commits April 19, 2024 18:00

Merge branch 'main' of github.com:primer/react into revert-4464-rever…

708f452

…t-3582

update tooltip tests to accomodate loading message aria-describedby

e765c05

Merge branch 'revert-4464-revert-3582' of github.com:primer/react int…

22ea550

…o revert-4464-revert-3582

mperrotti temporarily deployed to github-pages April 19, 2024 22:14 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4485 April 19, 2024 22:14 Inactive

mperrotti added 2 commits April 22, 2024 14:47

import missing modules in IconButton stories

b881fd7

Merge branch 'main' of github.com:primer/react into revert-4464-rever…

ccde61a

…t-3582

mperrotti temporarily deployed to github-pages April 22, 2024 18:51 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4485 April 22, 2024 18:51 Inactive

TylerJDev reviewed Apr 24, 2024

View reviewed changes

mperrotti added 2 commits April 26, 2024 16:48

makes aria-labelledby prop remain set when when button is in a loadin…

3399501

…g state

Merge branch 'main' of github.com:primer/react into revert-4464-rever…

4fce38a

…t-3582

mperrotti deployed to github-pages April 26, 2024 20:53 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-4485 April 26, 2024 20:54 View deployment

khiga8 reviewed May 1, 2024

View reviewed changes

broccolinisoup mentioned this pull request May 24, 2024

Apply 'disabled' styling when aria-disabled is set on a Button or IconButton #3991

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mperrotti commented Apr 10, 2024

changeset-bot bot commented Apr 10, 2024 •

edited

github-actions bot commented Apr 10, 2024 •

edited

siddharthkp commented Apr 10, 2024 •

edited

mperrotti commented Apr 10, 2024

siddharthkp commented Apr 12, 2024

mperrotti commented Apr 12, 2024

siddharthkp commented Apr 16, 2024 •

edited

mperrotti commented Apr 19, 2024

siddharthkp commented Apr 22, 2024

TylerJDev commented Apr 24, 2024

TylerJDev Apr 24, 2024

mperrotti Apr 26, 2024

mperrotti commented Apr 26, 2024 •

edited

mperrotti commented Apr 26, 2024

TylerJDev commented Apr 29, 2024

khiga8 left a comment

khiga8 May 1, 2024

khiga8 May 1, 2024

khiga8 May 1, 2024

khiga8 May 1, 2024

lindseywild commented May 16, 2024

	<Button loading={loading} leadingVisual={DownloadIcon} onClick={onClick('error')}>
	<Button loading={loading} leadingVisual={DownloadIcon} onClick={onClick('success')}>

	<VisuallyHidden aria-live="polite">{!loading && error ? 'Export failed' : null}</VisuallyHidden>
	<VisuallyHidden><Status>{!loading && error ? 'Export failed' : null}</Status></VisuallyHidden>


		export default meta

		export const LoadingStatusAnnouncementSuccessful = () => {

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

Are you sure you want to change the base?

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

Conversation

mperrotti commented Apr 10, 2024

changeset-bot bot commented Apr 10, 2024 • edited

🦋 Changeset detected

github-actions bot commented Apr 10, 2024 • edited

size-limit report 📦

siddharthkp commented Apr 10, 2024 • edited

mperrotti commented Apr 10, 2024

siddharthkp commented Apr 12, 2024

mperrotti commented Apr 12, 2024

siddharthkp commented Apr 16, 2024 • edited

mperrotti commented Apr 19, 2024

siddharthkp commented Apr 22, 2024

TylerJDev commented Apr 24, 2024

TylerJDev Apr 24, 2024

Choose a reason for hiding this comment

mperrotti Apr 26, 2024

Choose a reason for hiding this comment

mperrotti commented Apr 26, 2024 • edited

mperrotti commented Apr 26, 2024

TylerJDev commented Apr 29, 2024

khiga8 left a comment

Choose a reason for hiding this comment

khiga8 May 1, 2024

Choose a reason for hiding this comment

khiga8 May 1, 2024

Choose a reason for hiding this comment

khiga8 May 1, 2024

Choose a reason for hiding this comment

khiga8 May 1, 2024

Choose a reason for hiding this comment

lindseywild commented May 16, 2024

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

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

changeset-bot bot commented Apr 10, 2024 •

edited

github-actions bot commented Apr 10, 2024 •

edited

siddharthkp commented Apr 10, 2024 •

edited

siddharthkp commented Apr 16, 2024 •

edited

mperrotti commented Apr 26, 2024 •

edited