Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Fix off-center popover flash when clicking CopyToClipboard #3443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/copy-to-clipboard/internal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ export default function InternalCopyToClipboard({
triggerType="custom"
dismissButton={false}
renderWithPortal={popoverRenderWithPortal}
content={<InternalStatusIndicator type={status}>{statusText}</InternalStatusIndicator>}
content={
status === 'pending' ? null : <InternalStatusIndicator type={status}>{statusText}</InternalStatusIndicator>
}
Comment on lines +73 to +75
Copy link
Member

@cansuaa cansuaa Apr 29, 2025 •
edited
Loading

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.

Copy link
Author

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.

Copy link
Author

@TrevorBurnham TrevorBurnham Apr 30, 2025 •
edited
Loading

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?

__onOpen={onClick}
>
<InternalButton
Expand Down
39 changes: 39 additions & 0 deletions src/popover/__tests__/popover.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,45 @@ describe('Dismiss button', () => {
});
expect(wrapper.findBody({ renderWithPortal })).toBeTruthy();
});

it('does not render the popover body if content is null when a click is fired on the trigger', () => {
const wrapper = renderPopover({ children: 'Trigger', content: null, renderWithPortal });
wrapper.findTrigger().click();
act(() => {
wrapper
.findTrigger()
.getElement()
.dispatchEvent(new MouseEvent('mousedown', { bubbles: true }));
});
expect(wrapper.findBody({ renderWithPortal })).toBeNull();
});

it('renders the popover body if content becomes non-null after a click is fired on the trigger', () => {
const { container, rerender } = render(
<Popover content={null} renderWithPortal={renderWithPortal}>
Trigger
</Popover>
);
const wrapper = new PopoverInternalWrapper(container);

wrapper.findTrigger().click();
act(() => {
wrapper
.findTrigger()
.getElement()
.dispatchEvent(new MouseEvent('mousedown', { bubbles: true }));
});
expect(wrapper.findBody({ renderWithPortal })).toBeNull();

// Update the props with non-null content
rerender(
<Popover content="Popover Content" renderWithPortal={renderWithPortal}>
Trigger
</Popover>
);

expect(wrapper.findBody({ renderWithPortal })).toBeTruthy();
});
});
});

Expand Down
63 changes: 32 additions & 31 deletions src/popover/internal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,37 +147,38 @@ function InternalPopover(

const referrerId = useUniqueId();

const popoverContent = (
<div
aria-live={dismissButton ? undefined : 'polite'}
aria-atomic={dismissButton ? undefined : true}
className={clsx(popoverClasses, !renderWithPortal && styles['popover-inline-content'])}
data-awsui-referrer-id={referrerId}
>
<PopoverContainer
size={size}
fixedWidth={fixedWidth}
position={position}
trackRef={triggerRef}
arrow={position => <Arrow position={position} />}
renderWithPortal={renderWithPortal}
zIndex={renderWithPortal ? 7000 : undefined}
const popoverContent =
content === null ? null : (
<div
aria-live={dismissButton ? undefined : 'polite'}
aria-atomic={dismissButton ? undefined : true}
className={clsx(popoverClasses, !renderWithPortal && styles['popover-inline-content'])}
data-awsui-referrer-id={referrerId}
>
<LinkDefaultVariantContext.Provider value={{ defaultVariant: 'primary' }}>
<PopoverBody
dismissButton={dismissButton}
dismissAriaLabel={dismissAriaLabel}
header={header}
onDismiss={onDismiss}
overflowVisible="both"
closeAnalyticsAction={__closeAnalyticsAction}
>
{content}
</PopoverBody>
</LinkDefaultVariantContext.Provider>
</PopoverContainer>
</div>
);
<PopoverContainer
size={size}
fixedWidth={fixedWidth}
position={position}
trackRef={triggerRef}
arrow={position => <Arrow position={position} />}
renderWithPortal={renderWithPortal}
zIndex={renderWithPortal ? 7000 : undefined}
>
<LinkDefaultVariantContext.Provider value={{ defaultVariant: 'primary' }}>
<PopoverBody
dismissButton={dismissButton}
dismissAriaLabel={dismissAriaLabel}
header={header}
onDismiss={onDismiss}
overflowVisible="both"
closeAnalyticsAction={__closeAnalyticsAction}
>
{content}
</PopoverBody>
</LinkDefaultVariantContext.Provider>
</PopoverContainer>
</div>
);

const mergedRef = useMergeRefs(popoverRef, __internalRootRef);

Expand Down Expand Up @@ -215,7 +216,7 @@ function InternalPopover(
{children}
</span>
)}
{visible && (
{visible && popoverContent !== null && (
<ResetContextsForModal>
{renderWithPortal ? <Portal>{popoverContent}</Portal> : popoverContent}
</ResetContextsForModal>
Expand Down
Loading