-
Notifications
You must be signed in to change notification settings - Fork 21
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
#3181 - dynamic labels for investigator and senior official [-Bob] #3577
base: main
Are you sure you want to change the base?
#3181 - dynamic labels for investigator and senior official [-Bob] #3577
Conversation
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.
Just needs some cleanup, but everything is here! I like how you are addressing this holistically, which is a very nice touch
This allows us to avoid overriding aria-label, which is used by select2 | ||
to send the current dropdown selection to ANDI | ||
*/ | ||
export function initAriaInjections() { |
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.
Could you rename this to something similar to initAriaInjectionsForSelect2
? Would be more descriptive for someone unfamiliar with this part of the codebase
setTimeout(function () { | ||
// Find all spans with "--aria-description" in their id | ||
const descriptionSpans = document.querySelectorAll('span[id*="--aria-description"]'); | ||
|
||
// Iterate through each span to add aria-describedby | ||
descriptionSpans.forEach(function(span) { | ||
// Extract the base ID from the span's id (remove "--aria-description" part) | ||
const fieldId = span.id.replace('--aria-description', ''); | ||
|
||
// Find the field element with the corresponding ID | ||
const field = document.getElementById(fieldId); | ||
|
||
// If the field exists, set the aria-describedby attribute | ||
if (field) { | ||
let select2ElementDetected = false | ||
if (field.classList.contains('admin-autocomplete')) { | ||
const select2Id="select2-"+fieldId+"-container" | ||
console.log("select2---> "+select2Id) | ||
// If it's a Select2 component, find the rendered span inside Select2 | ||
const select2SpanThatTriggersAria = document.querySelector("span[aria-labelledby='"+select2Id+"']"); | ||
const select2SpanThatHoldsSelection = document.getElementById(select2Id) | ||
if (select2SpanThatTriggersAria) { | ||
console.log("set select2 aria") | ||
select2SpanThatTriggersAria.setAttribute('aria-describedby', span.id); | ||
// select2SpanThatTriggersAria.setAttribute('aria-labelledby', select2Id); | ||
select2ElementDetected=true | ||
} | ||
} |
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.
This works but this is definitely a workaround for select2 weirdness! I think this on its own should be fine but I'd like to try another solution first
I haven't tested it but according to select2's docs you can check if its intialized as so: https://select2.org/programmatic-control/methods#checking-if-the-plugin-is-initialized. Then you might be able to use a mutationObserver. If this works, it would be this would be less flaky overall since the load time can vary per machine
One thing to note is that instead of the $
icon, django admin binds it to django.jQuery
// check if it's already initialized
if ($('#mySelect2').hasClass("select2-hidden-accessible")) {
console.log("Select2 already initialized when we checked");
// do some stuff
return;
}
const observer = new MutationObserver(function(mutations) {
if (django.jQuery(`#${select2Id}`).hasClass("select2-hidden-accessible")) {
console.log("initialized");
// Stop observing once initialized
observer.disconnect();
}
});
observer.observe(document.body, {
childList: true,
subtree: true
});
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'm so glad you brought this up. I hate relying on timeouts, so thanks for the suggestion. I'll give it a try
// Set timeout so this fires after select2.js finishes adding to the DOM | ||
setTimeout(function () { | ||
// Find all spans with "--aria-description" in their id | ||
const descriptionSpans = document.querySelectorAll('span[id*="--aria-description"]'); |
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 can't find this in the dom - which page are you finding this id on?
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.
This is nested in components on pages like admin --> change domain request
{% if "related_widget_wrapper" in field.field.field.widget.template_name %} | ||
<span id="{{ field.field.id_for_label }}--aria-description" class="visually-hidden admin-select--aria-description"> | ||
{{ field.field.label }}, combo-box, collapsed, edit, has autocomplete. To set the value, use the arrow keys or type the text. | ||
</span> | ||
{% endif %} |
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.
This check is magic to me, nice job 😄
{% endcomment %} | ||
{% if "related_widget_wrapper" in field.field.field.widget.template_name %} | ||
<span id="{{ field.field.id_for_label }}--aria-description" class="visually-hidden admin-select--aria-description"> | ||
{{ field.field.label }}, combo-box, collapsed, edit, has autocomplete. To set the value, use the arrow keys or type the text. |
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.
@SamiyahKey / @CocoByte I think the andi content should be different here. With narrator this reads: Combobox, collapsed. Investigator, combo-box, collapsed, edit, has autocomplete. To set the value, use the arrow keys or type the text.
I would propose that this should be:
Edit, has autocomplete. To set the value, use the arrow keys or type the text.
since screenreaders will append the combobox information for us
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.
Also - per the andi warning, this should have a aria-labelledby! It should be aria-labelledby=${id_of_title}
so the screenreader will automatically read out that this is associated with investigator, rather than us having to add it manually
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.
Yes @zandercymatics I agree! Also the form field for "Senior official" still needs to be corrected, right now there still needs to be an accessible name added to the senior official field to tie with that aria-describedby
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.
No can do with select2. That attr is already used
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.
@zandercymatics can you think of a possibility to handle this senior official field?
if (select2SpanThatTriggersAria) { | ||
console.log("set select2 aria") | ||
select2SpanThatTriggersAria.setAttribute('aria-describedby', span.id); | ||
// select2SpanThatTriggersAria.setAttribute('aria-labelledby', select2Id); |
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.
Can you clean up some of the old comments and logs here?
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.
nice catch - thanks!
Co-authored-by: zandercymatics <[email protected]>
🥳 Successfully deployed to developer sandbox nl. |
🥳 Successfully deployed to developer sandbox nl. |
Ticket 3181
Resolves #3181
Changes
Context for reviewers
The two dropdowns listed in the original ACs really just highlight a more systemic issue with our ARIA labels for select2 dropdowns. The updates for this ticket addresses all select2 components throughout Django Admin so they will all follow the same pattern.
Setup
Code Review Verification Steps
As the original developer, I have
Satisfied acceptance criteria and met development standards
Ensured code standards are met (Original Developer)
Validated user-facing changes (if applicable)
As a code reviewer, I have
Reviewed, tested, and left feedback about the changes
Validated user-facing changes as a developer
Note: Multiple code reviewers can share the checklists above, a second reviewer should not make a duplicate checklist. All checks should be checked before approving, even those labeled N/A.
As a designer reviewer, I have
Verified that the changes match the design intention
Validated user-facing changes as a designer
References
Screenshots