Skip to content
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

Set role="application" #467

Merged
merged 1 commit into from
May 18, 2021
Merged

Set role="application" #467

merged 1 commit into from
May 18, 2021

Conversation

Malvoz
Copy link
Member

@Malvoz Malvoz commented May 18, 2021

Set role="application" on the map element.

Close #274, Close #423, Close #466.

Ideally role="region" on the Leaflet container would simply have been replaced by role="application", however doing so results in application not being announced by NVDA.

I've tested numerous combinations, e.g. moving both aria-label and role to the map element, or only setting role on the map element and aria-label and/or aria-roledescription on the Leaflet container, and vice-versa, etc. This approach seems to be the most consistent one.

@Malvoz Malvoz requested a review from prushforth May 18, 2021 03:19
@prushforth prushforth merged commit 45c980a into Maps4HTML:master May 18, 2021
@Malvoz Malvoz deleted the `role=application` branch May 18, 2021 23:02
@Malvoz
Copy link
Member Author

Malvoz commented May 19, 2021

@prushforth as you noticed popups are no longer announced, which is caused by this PR.

If we revert this and apply #466 instead the content is announced. I think it's strange to give featuregroups role="application" but the results are better than this.

Either way, we're just working around the root issue.

@Malvoz
Copy link
Member Author

Malvoz commented May 19, 2021

To be safe it's probably best to go back to square one by reverting this, and re-opening these:

Close #274, Close #423, Close #466.

role="application" on the map element(s) may have other implications we don't know about yet.

@prushforth
Copy link
Member

Huh I thought we were making progress, and we might be able to tweak something to get the popups read. All your changes seem to have worked, and as far as I recall, popups weren't being read even with limiting the role="application" to the featuregroup.

@Malvoz
Copy link
Member Author

Malvoz commented May 19, 2021

Yes we make progress in certain areas and introduce issues in others...

Setting role="application" has the following benefits:

But as noted popups are now not announced, I can confirm they were announced before applying role="application".

we might be able to tweak something to get the popups read

Yes... #274 (comment):

If we were to do this, we could perhaps set role="document" for things like author provided HTML (e.g. popup content) so that they don't inherit the application role's behavior

I've tested setting content.setAttribute("role", "document") in MapLayer.js locally, it works, the content of the popup is announced and the screen reader user can use keyboard shortcuts to navigate that content (tested in both Chrome and Firefox).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popups are misplaced when using a screen reader role="application"
2 participants