Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

D8CORE-4728: removed offending aria-label #865

Merged
merged 1 commit into from
Mar 31, 2022
Merged

Conversation

cjwest
Copy link
Member

@cjwest cjwest commented Mar 31, 2022

READY FOR REVIEW

Summary

Currently the search is broken for voice navigation because the visible label and accessible name do not match.
See https://my2.siteimprove.com/Accessibility/1061997/NextGen/Issue?ruleId=14&pageSegments=&issueKind=1&exceptTags=1,2&conformance=&siteGoal=0,1,3,4

Needed By (Date)

Soon

Urgency

Steps to Test

  1. Pull this into your site
  2. Verify in the search form that the aria-label="Search" has been removed.

Affected Projects or Products

  • Does this PR impact any particular projects, products, or modules?
  • D9Core
  • Anything with V6

Associated Issues and/or People

  • JIRA ticket: https://stanfordits.atlassian.net/browse/D8CORE-4728
  • Other PRs
  • Any other contextual information that might be helpful (e.g., description of a bug that this PR fixes, new functionality that it adds, etc.)
  • Anyone who should be notified? (@mention them here)

See Also

@cjwest cjwest requested a review from jenbreese March 31, 2022 20:28
@yvonnetangsu
Copy link
Member

yvonnetangsu commented Mar 31, 2022

@cjwest Is there a reason for this change? 😃 Do you have a quick link to the ticket? I'm guess it's because the button already have a text label and so we don't actually need the aria-label - is that correct?

@yvonnetangsu yvonnetangsu self-requested a review March 31, 2022 20:37
@cjwest cjwest requested a review from sherakama March 31, 2022 20:39
@cjwest
Copy link
Member Author

cjwest commented Mar 31, 2022

@cjwest Is there a reason for this change? 😃 Do you have a quick link to the ticket? I'm guess it's because the button already have a text label and so we don't actually need the aria-label - is that correct?

Actually, it's worse than that, @yvonnetangsu. The way it's worded, it doesn't allow for voice navigation. 🤦‍♀️
See Kevin's comment at:
https://stanfordits.atlassian.net/browse/D8CORE-4728?focusedCommentId=122125

@yvonnetangsu
Copy link
Member

@cjwest Is there a reason for this change? 😃 Do you have a quick link to the ticket? I'm guess it's because the button already have a text label and so we don't actually need the aria-label - is that correct?

Actually, it's worse than that, @yvonnetangsu. The way it's worded, it doesn't allow for voice navigation. 🤦‍♀️ See Kevin's comment at: https://stanfordits.atlassian.net/browse/D8CORE-4728?focusedCommentId=122125

Ahh that makes sense - the voice navigation! Removing the aria-label sounds good to me and I don't think it would break any existing projects. I can approve this for you now 😄 Thank you for this PR!

Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

GTG - thanks for being the a11y ambassador @cjwest ! 😄 👍🏼

@jenbreese
Copy link
Contributor

@yvonnetangsu I tried it out on Voiceover. The request is to merge to master. Do we want to do that or to v6? I am unsure. thanks! Jen

@yvonnetangsu
Copy link
Member

@yvonnetangsu I tried it out on Voiceover. The request is to merge to master. Do we want to do that or to v6? I am unsure. thanks! Jen

Awesome - thanks for testing, @jenbreese! I just double checked and we're merging any v6 changes into master so we're GTG 🥳

@yvonnetangsu
Copy link
Member

Please feel free to hit the squash and merge whenever you're ready, @cjwest or @jenbreese

@jenbreese jenbreese merged commit 822f50c into master Mar 31, 2022
@jenbreese jenbreese deleted the D8CORE-4728-search branch March 31, 2022 21:39
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants