-
Notifications
You must be signed in to change notification settings - Fork 171
fix: Fix off-center popover flash when clicking CopyToClipboard #3443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: Fix off-center popover flash when clicking CopyToClipboard #3443
Conversation
Because the clipboard API is async, the popover briefly renders in the "pending" state before rendering in the "success" or "error" state, which is perceived as a visual glitch. The fix here is to wait to open the popover until we're out of the "pending" state. To achieve that, I've modified the `InternalPopover` API so that it doesn't render a popover if the given `content` is `null`, regardless of its internal `visible` state.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3443 +/- ##
==========================================
+ Coverage 96.49% 96.55% +0.05%
==========================================
Files 807 807
Lines 23063 23740 +677
Branches 7974 7940 -34
==========================================
+ Hits 22255 22921 +666
- Misses 754 812 +58
+ Partials 54 7 -47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
content={ | ||
status === 'pending' ? null : <InternalStatusIndicator type={status}>{statusText}</InternalStatusIndicator> | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the content is passed as null, the screen reader stops working, at least VoiceOver doesn't announce the popover text anymore. You can try here.
My hypothesis is that it doesn't announce because screen reader has issues with conditional rendering of live regions like below (popover/internal). And when I remove that conditional rendering, it still doesn't announce and that is because the content changes from null to something else very fast. I will reach out to someone from a11y in our team to clarify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's very interesting! I asked Claude about this, and it said:
Screen readers need time to register the live region before it can announce changes. A small delay (50-100ms) between adding the element and updating its content helps ensure reliable announcements.
So the old copy behavior was (probably accidentally?) allowing the live region to be registered. This seems like a more general issue with popovers that don't have a dismiss button (since those that do have a dismiss button don't use aria-live
).
I found that using LiveRegion
instead of aria-live
makes the announcement work, likely because LiveRegion
uses timers to work around this issue. I've pushed a proof of concept here: TrevorBurnham@664ac49 Let me know if you'd like me to open up a separate PR for that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, this issue is addressed in the new Popover API: Anything with the popover
attribute is announced to screen readers as soon as it's displayed.
Using that attribute would require a deeper rethinking of the popover component, and browser support isn't quite where it needs to be for Cloudscape to adopt it yet (it requires Safari 17), but I thought I'd note it in case using the Popover API is on the roadmap.
At any rate, I think using LiveRegion
is the best workaround for now, so let me know how you'd like me to proceed: Should I open a separate PR to use LiveRegion
in popovers, then revisit this PR once that's merged?
This fixes the a11y issue discussed at cloudscape-design#3443 (comment), where popover content isn't announced to screen readers.
Re: cloudscape-design#3443 (comment) Using `LiveRegion` instead of `aria-live="polite"` fixes the issue where the popup was not announced by VoiceOver.
Description
Because the clipboard API is async, the
CopyToClipboard
popover briefly renders in the "pending" state before rendering in the "success" or "error" state, which is perceived as a visual glitch.The fix here is to wait to open the popover until we're out of the "pending" state. To achieve that, I've modified the
InternalPopover
API so that it doesn't render a popover if the givencontent
isnull
, regardless of its internalvisible
state.How has this been tested?
You can replicate the bug and verify this fix in the local demo: Run
npm run start
, open the local URL, then go to any of theCopyToClipboard
examples and click around.Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.