-
Notifications
You must be signed in to change notification settings - Fork 2
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
base: main
Are you sure you want to change the base?
Changes from all commits
cba5ad6
e2b9ccd
a5c75dd
5cbbba4
60a55a2
f99c951
d10cd39
98fc919
e11137d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
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, | ||
|
@@ -82,7 +98,7 @@ const withBannerData = ( | |
separateArticleCount, | ||
} = bannerProps; | ||
|
||
const [hasBeenSeen, setNode] = useHasBeenSeen( | ||
const [hasBeenSeen, setNode, node] = useHasBeenSeen( | ||
{ | ||
threshold: 0, | ||
}, | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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> | ||
|
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.
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 reachdocument.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.