AO3-6566 ARIA controls must contain the exact ID #4590
Conversation
neuroalien
force-pushed
the
AO3-6566-aria-controls-modal-id
branch
from
145e9ad to
04dd573
Compare
app/helpers/application_helper.rb
Outdated
| @@ -150,7 +150,7 @@ def link_to_modal(content = "", options = {}) | |||
| options[:for] ||= "" | |||
| options[:title] ||= options[:for] | |||
|
|
|||
| html_options = { "class" => options[:class] + " modal", "title" => options[:title], "aria-controls" => "#modal" } | |||
| html_options = { "class" => "#{options[:class]} modal", "title" => options[:title], "aria-controls" => "modal" } | |||
There was a problem hiding this comment.
Does this still need the old hash rocket syntax or does html_options = { class: "#{options[:class]} modal", title: options[:title], aria: { controls: "modal" } } work?
And it's okay if you don't know the answer to this, but any idea why we set the aria-controls attribute here and in the JavaScript? It seems like the JavaScript is overwriting this: if I just change it here and I check the modals in, e.g., the symbols block on blurbs or anywhere on the search pages, the modal links still have whatever aria-controls is set in the JavaScript.
There was a problem hiding this comment.
I'm pretty sure there's no need for the old syntax for the first two keys, but I didn't know you could nest aria keys like that. I also wasn't sure if we wanted to do piecemeal syntax updates, or if that would open a whole can of worms. 😅
Is it meant to work without JavaScript? No clue. 🤷
There was a problem hiding this comment.
Without JavaScript, the modal definitely won't work -- it should just fall back to being a plain link, but I haven't tested it to see if that's wishful linking -- so including the ARIA attribute is pretty much unnecessary.
The ID of the modal controlled by the link to modal is `modal`. The hash sign must not be included in the reference to the element. As per Sarken's comment, I've removed the redundant setting of the aria attribute in HTML options, since it's always overwritten by the JavaScript.
CristinaRO
force-pushed
the
AO3-6566-aria-controls-modal-id
branch
from
04dd573 to
8095b45
Compare
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-6566
Purpose
What does this PR do?
Correctly declare the ID of the element controlled by the link to modal.
The ID of the modal controlled by the link to modal is
modal. The hash sign must not be included in the reference to the element.Testing Instructions
How can the Archive's QA team verify that this is working as you intended?
If you have a Jira account with access, please update or comment on the issue
with any new or missing testing instructions instead.
References
Are there other relevant issues/pull requests/mailing list discussions?
Credit
What name and pronouns should we use to credit you in the Archive of Our Own's Release Notes?
If you have a Jira account, please include the same name in the "Full name"
field on your Jira profile, so we can assign you the issues you're working on.
Please note that if you do not fill in this section, we will use your GitHub account name and
they/them pronouns.