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.

Sign up for GitHub

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

Jump to bottom

Hide X char in popup close button [accessibility] #11040

Merged
merged 3 commits into from
Sep 22, 2021
Merged

Hide X char in popup close button [accessibility] #11040

merged 3 commits into from
Sep 22, 2021

Conversation

avpeery
Copy link
Contributor

@avpeery avpeery commented Sep 21, 2021 •
edited
Loading

Adding the attribute 'aria-hidden' hides the X character used for the close popup button from being voiced over when enabling voice over accessibility. As mentioned in the issue #11035 , the X character is announced as 'multiplication' or 'times'. Tested debug page with accessibility voice over turned on.

Launch Checklist

  • briefly describe the changes in this PR
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'

@avpeery avpeery added the skip changelog Used for PRs that do not need a changelog entry label Sep 21, 2021
@avpeery avpeery linked an issue Sep 21, 2021 that may be closed by this pull request
[accessibility] The × character in the close button of popups should be explicitly hidden from ATs #11035
Closed
mourner
mourner reviewed Sep 21, 2021
View reviewed changes
src/ui/popup.js Outdated
@@ -534,6 +534,7 @@ export default class Popup extends Evented {
this._closeButton = DOM.create('button', 'mapboxgl-popup-close-button', this._content);
this._closeButton.type = 'button';
this._closeButton.setAttribute('aria-label', 'Close popup');
this._closeButton.setAttribute('aria-hidden', '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to set explicitly to true as the ticket suggests?

Suggested change
this._closeButton.setAttribute('aria-hidden', '');
this._closeButton.setAttribute('aria-hidden', 'true');

Copy link
Contributor Author

@avpeery avpeery Sep 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mourner This is my bad - I should've updated the PR to specify this change. I originally had 'true' but it failed CircleCI flow tests with this error: "Cannot call this._closeButton.setAttribute with true bound to value because boolean [1] is incompatible with
string [2].". I looked up the setAttribute docs and it recommends using an empty string in the example: https://developer.mozilla.org/en-US/docs/Web/API/Element/setAttribute#example

Copy link
Contributor

@rreusser rreusser Sep 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe set to 'aria-hidden' or otherwise add a comment? // The empty string is a recommended truthy value for setAttribute. Seems a bit surprising/confusing, though there's relatively low maintenance burden here.

rreusser
rreusser approved these changes Sep 21, 2021
View reviewed changes
Copy link
Contributor

@rreusser rreusser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing! 👏 🙇 No change required by me, though the empty string being a recommended truthy value is definitely a TIL for me!

src/ui/popup.js Outdated
@@ -534,6 +534,7 @@ export default class Popup extends Evented {
this._closeButton = DOM.create('button', 'mapboxgl-popup-close-button', this._content);
this._closeButton.type = 'button';
this._closeButton.setAttribute('aria-label', 'Close popup');
this._closeButton.setAttribute('aria-hidden', '');
Copy link
Contributor

@rreusser rreusser Sep 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe set to 'aria-hidden' or otherwise add a comment? // The empty string is a recommended truthy value for setAttribute. Seems a bit surprising/confusing, though there's relatively low maintenance burden here.

Malvoz
Malvoz suggested changes Sep 21, 2021
View reviewed changes
Copy link

@Malvoz Malvoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aria-hidden is not a boolean, it should be set to true.

@avpeery
Copy link
Contributor Author

avpeery commented Sep 21, 2021

aria-hidden is not a boolean, it should be set to true.

Hi @Malvoz thanks for checking on this. Unfortunately, I came across an error when trying to set it to true on our automated tests. I can look into why this is, but to double check you mean the attribute value must be set to boolean true and not string "true"? Thanks!

mourner
mourner requested changes Sep 21, 2021
View reviewed changes
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@avpeery I think the empty string thing in MDN only applies to specific attributes like disabled and checked, but not to aria-hidden — it needs to explicitly be set to the "true" string, and Flow shouldn't complain.

SnailBones
SnailBones approved these changes Sep 22, 2021
View reviewed changes
Copy link
Contributor

@SnailBones SnailBones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@avpeery avpeery merged commit 881bd85 into main Sep 22, 2021
@avpeery avpeery deleted the avpeery/hide-x-char-accessibility branch September 22, 2021 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Malvoz Malvoz Malvoz approved these changes

@SnailBones SnailBones SnailBones approved these changes

@rreusser rreusser rreusser approved these changes

@mourner mourner mourner approved these changes

Assignees
No one assigned
Labels
skip changelog Used for PRs that do not need a changelog entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[accessibility] The × character in the close button of popups should be explicitly hidden from ATs
5 participants
@avpeery @mourner @rreusser @SnailBones @Malvoz