-
Notifications
You must be signed in to change notification settings - Fork 653
AO3-7131 Text used by screen readers to announce help links is unclear #5526
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
base: master
Are you sure you want to change the base?
Conversation
|
According to the ticket, the links should only have the The new method
Now that the "locale preferences" help modal is prepared for localization and using the new method, it might be a good idea to do it with that one instead, with a new link name like "Locale preferences help" Also, you should fix style issues that Rubocop finds - use double quotes and add the space where it's missing after a colon in this case. There's a bit of info on Rubocop in the wiki (Reviewdog and RuboCop, Previewing Reviewdog comments) and you can look up info on specific guidelines using their names if you need examples - e.g. Style/StringLiterals |
|
Thanks for the review! I'll get on it |
035f358 to
273d319
Compare
|
I think I did everything as you requested. Did the force push because I rebased recent changes from master. I have some concerns:
|
|
Please hold on on further review, the think the rebase went wrong, I can't even start it right now.
I think it may be related to #5513 ? I can't even start a fresh |
|
Seems like your development environment isn't up to date. Are you using Docker? There's a guide for updating it in the wiki |
|
Oh yep! I'm trying to rebuild the images now and it looks promising |
Both need to be changed - most help modals still use Also I don't think renaming the _title to _label is needed, technically it's still a title, it's just not in the |
|
I apologize for the persistent misunderstandings, does this look good? This ended up much smaller patch than I expected |
|
By checking my other PR, I found out that the tests failings isnt normal, I'll fix it tomorrow |
|
Ah, the tests are failing because the title attribute got removed, sorry for not noticing that earlier... I've looked more into accessibility for buttons like these to see if there are better solutions and it seems it's usually recommended to use invisible text rather than An simpler option would be just modifying the tests to work for buttons that are only @sarken what do you think? Regardless of these test failures, the |
|
I think I'd be inclined to define a new step for the help links in particular instead of updating our configuration to use |
|
Alright, how about now? Only concern of my right now is that
may redundantly check for both title and aria-label but I wanted to keep backwards compatibility with title, as I'm sure there are other parts of the site that still use title |
slavalamp
left a comment
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.
I wanted to keep backwards compatibility with title, as I'm sure there are other parts of the site that still use title
There are a couple other modals that do, yeah, so that part is good
| When /^(?:|I )follow help tag "([^"]*)"(?: within "([^"]*)")?$/ do |link, selector| # rubocop:disable Cucumber/RegexStepName | ||
| with_scope(selector) do | ||
| find(:xpath, "//*[@aria-label='#{link}']").click | ||
| end | ||
| end | ||
|
|
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.
I think it'd be nicer to call this step "I open help modal", it probably wouldn't ever need the "within selector" part, and as sarken said, you can use the capybara option in the step definition, so it could look like this
When /^(?:|I )open help modal "([^"]*)"$/ do |link|
Capybara.enable_aria_label = true
step %{I follow "#{link}"}
endand it doesn't really belong in web_steps.rb, you can make a new file help_steps.rb for this
Pull Request Checklist
as the first thing in your pull request title (e.g.
AO3-1234 Fix thing)until they are reviewed and merged before creating new pull requests.
Issue
https://otwarchive.atlassian.net/browse/AO3-7131
Purpose
This PR addresses AO3-7131, bad accessibility of help modals.
I have some concerns:
Testing Instructions
Described in AO3-7131, I tested using KDE built in screen reader and Firefox.
Credit
Richard Hajek, he/him, assigned in Jira