AG-12606 Screenreader fixes #2479
Conversation
olegat
force-pushed
the
AG-12606/screenreader_fixes
branch
from
d8d150c
to
6ac71bd
Compare
| // menu item without an accessible text alternative. | ||
| type LabelAndIcon = { label: string; icon?: AgIconName; altText?: undefined }; | ||
| type IconOnly = { label?: undefined; icon: AgIconName; altText: string }; | ||
| export type LabelIcon = LabelAndIcon | IconOnly; |
There was a problem hiding this comment.
Note: This LabelIcon allows 3 syntax modes:
{ label } // text only
{ label, icon } // text & icon
{ icon, altText } // icon only (with altText for screenreaders)Specifying an icon without altText, or specifying both a label and altText will raise a compiler error.
| if (key === 'ariaControls') return 'aria-controls'; | ||
| return key; | ||
| // Convert camelCaseString to kebab-case-string | ||
| return key.replace(/([a-z])([A-Z])/g, '$1-$2').toLowerCase(); |
There was a problem hiding this comment.
This won't work for some property-attribute pairs like ariaSetSize / aria-setsize or ariaPosInSet / aria-posinset. We might want to have a more strict interface for the Attrs type.
Thoughts?
There was a problem hiding this comment.
Agreed, the current Attrs type also allows function properties like setEventListener which is a bit leaky.
If we only care about targetting aria attributes, could we just replace the aria with aria- and not kebab the whole thing? Or are there other attrs we need to consider?
The reason I called it "missing" aria attrs is the typescript types for elements includes many attrs like ariaLabel but not ariaControls. Not sure if that is just our ts version being out of date or if it implies something else?
There was a problem hiding this comment.
AG-Grid uses wrappers to set aria attributes:
https://github.com/ag-grid/ag-grid/blob/419770ca302ce87bc3c9a64b51a464c86132ab7c/community-modules/core/src/utils/aria.ts
We have something similar but it's not so widely used unfortunately:
https://github.com/ag-grid/ag-charts/blob/c2f6bb944c7dd8bce14803cdac6717a485e2537f/packages/ag-charts-community/src/util/attributeUtil.ts
It might be a good idea to start using attributeUtil.ts in more places sure as here.
| @@ -67,6 +67,8 @@ export const AG_CHARTS_LOCALE_EN_US: Record<string, string> = { | |||
| iconAltTextAlignCenter: 'Center', | |||
| // Alt-text for the 'position-right' icon | |||
| iconAltTextAlignRight: 'Right', | |||
| // Alt-text for the 'close' icon | |||
| iconAltTextClose: 'Close', | |||
There was a problem hiding this comment.
Worth highlighting this with @JamesSwinton - #2478
There was a problem hiding this comment.
We'll hold off merging #2478 until these new keys are merged.
olegat
force-pushed
the
AG-12606/screenreader_fixes
branch
from
6ac71bd
to
1f30005
Compare
olegat
force-pushed
the
AG-12606/screenreader_fixes
branch
from
1f30005
to
1516482
Compare
The :focus-visible CSS pseudo-class will take care of showing/hidden the indicator box.
The camelCase to kebab-case conversion elements.ts will not correctly work for property/attribute pairs like ariaSetSize/aria-setsize. Also, the current type mechanism exposes "attributes" like addEventListener which is a method not an attribute. Piggy-back on the existing `setAttribute` function to enforce type safety.
olegat
force-pushed
the
AG-12606/screenreader_fixes
branch
from
1516482
to
1eaf789
Compare
https://ag-grid.atlassian.net/browse/AG-12606