minor accessibility updates for Portal #26394
minor accessibility updates for Portal #26394cathysarisky wants to merge 2 commits intoTryGhost:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughConverted the close control from a 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/portal/src/components/common/close-button.js`:
- Around line 16-20: The CloseButton markup is causing accessibility and
correctness problems: add type="button" to the <button> element to prevent
accidental form submits, remove the duplicate onClick from the CloseIcon so the
handler (onClick || this.closePopup) is only attached to the button, and provide
an accessible name by adding an aria-label (e.g., aria-label="Close") or visible
text inside the button so screen readers can announce it; keep the CloseIcon as
a presentational SVG (no alt prop) and ensure the button uses the existing
onClick prop or this.closePopup.
🧹 Nitpick comments (1)
apps/portal/src/components/frame.styles.js (1)
516-525: Good button reset — consider addingcursor: pointeron the container too.The reset properties correctly neutralize default button styling. However,
cursor: pointeris only set on the child.gh-portal-closeicon(line 533), not on the button container itself. If there's any padding or hit-area around the icon, the cursor won't change to a pointer on the container edges. This is minor sincepadding: 0currently eliminates gap, but it's a defensive addition.Optional: add cursor: pointer
.gh-portal-closeicon-container { background: none; border: none; padding: 0; margin: 0; + cursor: pointer; position: fixed; top: 24px; right: 24px; z-index: 10000; }
| <button className='gh-portal-closeicon-container' data-test-button='close-popup' onClick = {onClick || this.closePopup}> | ||
| <CloseIcon | ||
| className='gh-portal-closeicon' alt='Close' onClick = {onClick || this.closePopup} data-testid='close-popup' | ||
| /> | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
Several accessibility and correctness issues with the new button element.
-
Missing
type="button": Without an explicit type, buttons default totype="submit". SinceCloseButtonis rendered inside pages that contain<form>elements (e.g., offer-page signup), clicking close could trigger form submission. -
Duplicate
onClickcauses double invocation: TheonClickhandler is attached to both the<button>(line 16) and the child<CloseIcon>(line 18). Clicking the SVG fires its handler, then the event bubbles to the button, firing the same handler again. Remove theonClickfromCloseIcon. -
Missing accessible name: The button has no text content and no
aria-label. Thealt='Close'on the SVG is not valid for SVG elements (only for<img>). Screen readers will announce this as an unlabeled button, which undermines the accessibility goal of this PR.
Proposed fix
- <button className='gh-portal-closeicon-container' data-test-button='close-popup' onClick = {onClick || this.closePopup}>
+ <button type='button' className='gh-portal-closeicon-container' data-test-button='close-popup' aria-label='Close' onClick={onClick || this.closePopup}>
<CloseIcon
- className='gh-portal-closeicon' alt='Close' onClick = {onClick || this.closePopup} data-testid='close-popup'
+ className='gh-portal-closeicon' aria-hidden='true' data-testid='close-popup'
/>
</button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button className='gh-portal-closeicon-container' data-test-button='close-popup' onClick = {onClick || this.closePopup}> | |
| <CloseIcon | |
| className='gh-portal-closeicon' alt='Close' onClick = {onClick || this.closePopup} data-testid='close-popup' | |
| /> | |
| </div> | |
| </button> | |
| <button type='button' className='gh-portal-closeicon-container' data-test-button='close-popup' aria-label='Close' onClick={onClick || this.closePopup}> | |
| <CloseIcon | |
| className='gh-portal-closeicon' aria-hidden='true' data-testid='close-popup' | |
| /> | |
| </button> |
🤖 Prompt for AI Agents
In `@apps/portal/src/components/common/close-button.js` around lines 16 - 20, The
CloseButton markup is causing accessibility and correctness problems: add
type="button" to the <button> element to prevent accidental form submits, remove
the duplicate onClick from the CloseIcon so the handler (onClick ||
this.closePopup) is only attached to the button, and provide an accessible name
by adding an aria-label (e.g., aria-label="Close") or visible text inside the
button so screen readers can announce it; keep the CloseIcon as a presentational
SVG (no alt prop) and ensure the button uses the existing onClick prop or
this.closePopup.
1 participant
This PR makes minor adjustments to portal to increase accessibility for screen readers. No visual changes, just better accessibility for users who need to tab navigate or use a screen reader.
The close button is now a button, not a div, which buys it automatic focus-ability. I added css to remove the default button styling, and the click handler on the button, so that screen users and tab-navigators can access it.
Also updated the benefits "checks" to not get read by the screen reader.