chore: fix various a11y violations in examples (2) #7604
Conversation
|
Preview: https://patternfly-react-pr-7604.surge.sh A11y report: https://patternfly-react-pr-7604-a11y.surge.sh |
nicolethoen
changed the title
nicolethoen
force-pushed
the
fix_site_a11y_violations_3
branch
from
ce4bbba to
2600e21
Compare
|
What I'm seeing that I want to point out to see if these are issues that should be addressed:
|
|
Thanks @andyyvo
I'm not sure that I can do anything about these violations - right now, the violation is raising a 'needs review' warning (in my console). This is because the buttons are claiming to be labelled by tooltips which don't exist on the page (yet - until they are triggered to open). So I sort of decided that 'upon review' this was not really a problem. @thatblindgeye do you think that makes sense?
I did notice that as well, and I made a note to ignore this violation whenever the report is run (bullet point number 5 in this issue). |
That makes sense to me. It might be worth looking into in the future to avoid the error, but I'm also getting the "needs review" and "impact: unknown" on this issue in aXe. One quick work around could be to wrap the Tooltip in a div and give the div the id of "tooltip-ref1", but that would still be worth discussing if we wanted to update our recommended approach to that just for this aXe issue. |
nicolethoen
requested review from
jenny-s51 and
kmcfaul
and removed request for
jenny-s51
| aria-hidden={!navOpen} | ||
| {...props} | ||
| > | ||
| <div className={styles.pageSidebarBody}> |
There was a problem hiding this comment.
@nicolethoen were the styles not getting applied correctly? @mattnolting or @mcoker can you verify everything is as expected .
There was a problem hiding this comment.
The class still looks like it's getting applied properly.
| @@ -0,0 +1,3 @@ | |||
| .pf-c-page__sidebar-body { | |||
There was a problem hiding this comment.
It's to fix this "Navigation" text in the sidebar.
We don't set the color of regular text in the sidebar since we don't have a use case for text there, so by default the "Navigation" text is our default/black text color and not visible. This local style makes it visible. @nicolethoen's fix is the same one I recommended to fix this issue in core - patternfly/patternfly#4920 (comment)
Though without a parent selector to limit this style to the page examples, will it apply to other .pf-c-page__sidebar-body elements in other examples after visiting the page component examples and loading this style? I don't know if that would cause an immediate issue, but it might lead to unexpected styles in other examples.
There was a problem hiding this comment.
+1 to what @mcoker said, I was just trying to fix the color contrast issue without adding extra components to the page examples - trying to keep them pure.
| isPlain = false, | ||
| isLiveRegion = false, | ||
| variantLabel = `${capitalize(variant)} alert:`, | ||
| 'aria-label': ariaLabel = `${capitalize(variant)} Alert`, |
There was a problem hiding this comment.
Not necessarily a blocker, but one pro for keeping this aria-label in is that, at least for VoiceOver, it announces "Info alert, group" and "end of info alert, group" when entering/exiting it. So possibly better context of what is actually considered part of the alert or not.
I would agree that I don't think we need both this aria-label and the variantLabel that prefaces the alert title, though, and if we would want to avoid that aXe error about aria-labels on divs then I'm on board with this change. Another option would be adding a role/changing the div, but I'm not sure what would be the best approach for this other than maybe a status role (though that will create a live region on alerts).
There was a problem hiding this comment.
Yeah this was one where I was not sure which of the possible fixes were the best.
I opted for this because it's a single alert, not an alert group, so I thought that VO call out was not quite accurate.
jessiehuff
left a comment
jessiehuff
left a comment
There was a problem hiding this comment.
On the alert discussion, I admit that I'm conflicted on it myself. I do think the aria-label can add extra context that it's an alert/block of info, but I'm not quite sure what role to give it. We could give it role="alert" but that typically turns it into a live region, and I don't know if that makes sense in all contexts/if it could create a double live region when used with AlertGroup. When users are including dynamic alerts, we currently recommend that they use an AlertGroup to contain them and that becomes the live region. So ideally this shouldn't be too big of an issue to remove the aria-label because the only time it should affect them is in static single alerts.
nicolethoen
force-pushed
the
fix_site_a11y_violations_3
branch
from
2bb36fc to
e33f668
Compare
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
9 participants
What: Closes #7596
Notes:
Beta featurealert on beta components was throwing an a11y error about heading level and aria-labels on divs. I don't believe that Alerts need the generated aria-label on divs, since that aria label programmatically matches thepf-u-screen-readertext in the Alert title. I will address the heading level violation in the beta tag in org after this PR merges.