Skip to content
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

Banner accessibility fix #771

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
4 changes: 2 additions & 2 deletions packages/modules/src/hooks/useHasBeenSeen.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { useEffect, useState, useRef } from 'react';
import libDebounce from 'lodash.debounce';

export type HasBeenSeen = [boolean, (el: HTMLDivElement) => void];
export type HasBeenSeen = [boolean, (el: HTMLDivElement) => void, HTMLElement | null];

const useHasBeenSeen = (options: IntersectionObserverInit, debounce?: boolean): HasBeenSeen => {
const [hasBeenSeen, setHasBeenSeen] = useState<boolean>(false);
Expand Down Expand Up @@ -39,7 +39,7 @@ const useHasBeenSeen = (options: IntersectionObserverInit, debounce?: boolean):
}
}, [node, options, intersectionCallback]);

return [hasBeenSeen, setNode];
return [hasBeenSeen, setNode, node];
};

export { useHasBeenSeen };
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ const styles = {
color: inherit;
&:focus {
outline: none !important;
border-bottom-width: 3px;
}
`,
overlayContainer: css`
Expand Down
70 changes: 69 additions & 1 deletion packages/modules/src/modules/banners/common/BannerWrapper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,22 @@ export const getParagraphsOrMessageText = (
return bodyCopy;
};

const checkIfElementIsHidden = (el: HTMLElement | null): boolean => {
while (el && el.parentNode != null) {
if (el && el.style) {
if (el.style.display === 'none' || el.style.visibility === 'hidden') {
return true;
}
const computed = window.getComputedStyle(el);
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling getComputedStyle will cause style recalculation and potentially layout reflow. Determining visibility programmatically is tricky, but traversing up a DOM tree and calling this on every node will be terrible for performance, especially as this effectively has no exit condition until you reach document.body.

If we absolutely cannot avoid doing this, I think it would be better to use a TreeWalker (generally much more performant than a JS while loop) with the main container element of the banner as its root node, so it won't work its way up the entire DOM tree.

if (computed.display === 'none' || computed.visibility === 'hidden') {
return true;
}
}
el = el.parentNode instanceof HTMLElement ? el.parentNode : null;
}
return false;
};

const withBannerData = (
Banner: React.FC<BannerRenderProps>,
bannerId: BannerId,
Expand All @@ -82,7 +98,7 @@ const withBannerData = (
separateArticleCount,
} = bannerProps;

const [hasBeenSeen, setNode] = useHasBeenSeen(
const [hasBeenSeen, setNode, node] = useHasBeenSeen(
{
threshold: 0,
},
Expand All @@ -101,6 +117,58 @@ const withBannerData = (
}
}, [submitComponentEvent]);

useEffect(() => {
if (node != null) {
// Timeout required to allow component to settle before identifying focussable elements within it
setTimeout(() => {
Comment on lines +122 to +123
Copy link
Contributor

Choose a reason for hiding this comment

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

Arbitrary timeouts to let some other process finish are usually not a good idea. What does the component 'settling' mean, and are we confident it will always take less than 1000 milliseconds? Is there a better way to detect this?

const closeButtonElements: HTMLElement[] = [
...node.querySelectorAll<HTMLElement>('[aria-label="Close"]'),
].filter(
el =>
!el.hasAttribute('disabled') &&
!el.getAttribute('aria-hidden') &&
!checkIfElementIsHidden(el),
);
const focusableElements: HTMLElement[] = [
...node.querySelectorAll<HTMLElement>(
'button, a[href], input, [tabindex]:not([tabindex="-1"]',
),
].filter(
el =>
!el.hasAttribute('disabled') &&
!el.getAttribute('aria-hidden') &&
!checkIfElementIsHidden(el),
);
Comment on lines +124 to +141
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like it would be much more straightforward to use React refs to get instances of the elements we might want to focus, rather than having to do a DOM query for stuff we already control the creation and rendering of.

If we can't currently pass refs to focusable Source components this is something we need to bring up with CSTI and potentially make a PR to Source to implement.

const firstFocussableElement = focusableElements[0];
const lastFocussableElement = focusableElements[focusableElements.length - 1];

if (closeButtonElements[0] != null) {
closeButtonElements[0].focus();
} else {
focusableElements[0].focus();
}

node.addEventListener('keydown', e => {
const isTabPressed = e.key === 'Tab' || e.keyCode === 9;
if (!isTabPressed) {
return;
}
if (e.shiftKey) {
if (document.activeElement === firstFocussableElement) {
lastFocussableElement.focus();
e.preventDefault();
}
} else {
if (document.activeElement === lastFocussableElement) {
firstFocussableElement.focus();
e.preventDefault();
}
}
});
}, 1000);
}
}, [node]);

const cleanParagraphsOrMessageText = (
paras: string[] | undefined,
text: string | undefined,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ export const defaultStory = (): ReactElement => {
],
'|',
),
cta: {
baseUrl: 'https://theguardian.com',
text: 'Subscribe',
},
};

const mobileContent: BannerContent = {
Expand All @@ -59,6 +63,10 @@ export const defaultStory = (): ReactElement => {
],
'|',
),
cta: {
baseUrl: 'https://theguardian.com',
text: 'Subscribe',
},
secondaryCta: {
type: SecondaryCtaType.Custom,
cta: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ const DigitalSubscriptionsBanner: React.FC<BannerRenderProps> = ({
<div css={iconPanel}>
<ThemeProvider theme={buttonBrand}>
<Button
aria-label="Close"
size="small"
cssOverrides={closeButton}
priority="tertiary"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,14 @@ export const EnvironmentMomentBannerCloseButton: React.FC<EnvironmentMomentBanne
onClick,
}: EnvironmentMomentBannerCloseButtonProps) => (
<ThemeProvider theme={buttonReaderRevenueBrandAlt}>
<Button onClick={onClick} css={button} size="small" icon={<SvgCross />} hideLabel>
<Button
aria-label="Close"
onClick={onClick}
css={button}
size="small"
icon={<SvgCross />}
hideLabel
>
Dismiss the banner
Comment on lines +18 to 26
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a degradation of the experience for screen reader users- 'Dismiss the banner' is a much richer and more useful prompt than 'Close' (close what, specifically?). It's generally best to avoid using ARIA if regular ol' HTML will do the job, which a button with hideLabel does.

If there's no way around doing DOM queries to get focusable elements and you need an arbitrary selector for these buttons, it would be better to use a data attribute.

</Button>
</ThemeProvider>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export function GlobalNewYearBannerCloseButton({
<div css={styles.container}>
<Hide above="tablet">
<Button
aria-label="Close"
onClick={onCloseClick}
cssOverrides={styles.closeButton}
icon={<SvgCross />}
Expand All @@ -44,6 +45,7 @@ export function GlobalNewYearBannerCloseButton({
<Hide below="tablet">
<div>
<Button
aria-label="Close"
onClick={onCloseClick}
cssOverrides={styles.closeButton}
icon={<SvgCross />}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ type ButtonPropTypes = {

const CloseButton = (props: ButtonPropTypes): ReactElement => (
<Button
aria-label="Close"
css={closeButtonStyles}
data-link-name={closeComponentId}
onClick={props.onClick}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const withCloseable = (CloseableBanner: React.FC<CloseableBannerProps>): React.F
const onClose = (): void => {
setChannelClosedTimestamp(bannerProps.bannerChannel);
setIsOpen(false);
document.body.focus();
};

useEscapeShortcut(onClose, []);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ export function InvestigationsMomentBannerCloseButton({
<div css={styles.container}>
<Hide above="tablet">
<Button
aria-label="Close"
onClick={onCloseClick}
cssOverrides={styles.closeButton}
icon={<SvgCross />}
Expand All @@ -71,6 +72,7 @@ export function InvestigationsMomentBannerCloseButton({
<Hide below="tablet">
<div>
<Button
aria-label="Close"
onClick={onCloseClick}
cssOverrides={styles.closeButton}
icon={<SvgCross />}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ const styles = {
color: inherit;
&:focus {
outline: none !important;
border-bottom-width: 3px;
}
`,
overlayContainer: css`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export function MomentTemplateBannerCloseButton({
</div>

<Button
aria-label="Close"
onClick={onCloseClick}
cssOverrides={buttonStyles(settings)}
icon={<SvgCross />}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const articleCountButton = css`
color: inherit;
&:focus {
outline: none !important;
border-bottom-width: 3px;
}
`;

Expand Down