-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Fix some failed results from Web map tools WCAG 2.1 evaluation #9991
Conversation
That's a really cool doc that we can reference! This is exciting work. @tristen would you be willing to take a look at #9774 as part of this work? It's an attempt to make markers more accessible and focusable. I'm not sure it's quite ready, but it would be helpful to get a second set of eyes on it. Also cc @malwoodsantoro who has been working on an accessibility plugin |
{key: 'owner', value: this.styleOwner}, | ||
{key: 'id', value: this.styleId}, | ||
{key: 'access_token', value: this._map._requestManager._customAccessToken || config.ACCESS_TOKEN} |
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.
Not a necessary change but I just came across this string inconsistency.
Cool looks like this would close #3959 |
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.
Overall this looks really great. I just have a few minor suggestions.
this._innerContainer = DOM.create('div', 'mapboxgl-ctrl-attrib-inner', this._container); | ||
this._innerContainer.setAttribute('role', 'list'); |
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.
The comment about this feature is Control to display attribution and feedback links is missing name and role.
. Should we add a name
attribute to this feature as well?
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.
Yeah, name
and role
for the feedback links themselves. I have a PR in a branch to address that in the TileJSON. The attribution control just receives these as strings and appends them in so it felt right to do it upstream.
|
||
.mapboxgl-ctrl-attrib.mapboxgl-compact::after { | ||
content: ''; | ||
.mapboxgl-ctrl-attrib-button { |
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.
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.
Increased padding in 67282f4 to address this!
Addresses some Level A and Level AA failures as outlined in https://github.com/Malvoz/web-maps-wcag-evaluation/blob/ac797a3e5fe984db27662c27ae05077e7b22026b/README.md
Level A
In https://github.com/Malvoz/web-maps-wcag-evaluation#131-info-and-relationships-level-a
aria-hidden
attribute when min/max controls trigger disabled statesIn https://github.com/Malvoz/web-maps-wcag-evaluation#211-keyboard-level-a
In https://github.com/Malvoz/web-maps-wcag-evaluation#412-name-role-value-level-a
Added
role:region
to the canvas container.Added
role:list
to the.mapboxgl-ctrl-attrib-inner
container. This also requires an upstream change to the TileJSON doc to addrole:listitem
Level AA
In https://github.com/Malvoz/web-maps-wcag-evaluation#247-focus-visible-level-aa
This is addressed by turning the compact attribution link into a button toggle with focus.
Launch Checklist
document any changes to public APIspost benchmark scorestagged@mapbox/map-design-team
@mapbox/static-apis
if this PR includes style spec API or visual changestagged@mapbox/gl-native
if this PR includes shader changes or needs a native portmapbox-gl-js
changelog:<changelog>Addresses some Level A and Level AA accesibility recommendations</changelog>