Skip to content

Fixed #35892 -- Supported Widget.use_fieldset in admin forms and linked help texts aria-describedby for accessibility. #19678

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Antoliny0919
Copy link
Contributor

@Antoliny0919 Antoliny0919 commented Jul 27, 2025

Trac ticket number

ticket-35892

Branch description

continues PR..

Thank you to @sai-ganesh-03 for implementing the work, to @sarahboyce for offering guidance on the direction,
and to @knyghty for bringing up the issue 🥰

I updated the admin change form to use <fieldset> and <legend> tags for fields when use_fieldset is set to True.
I also connected additional help texts within widgets using aria-describedby.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

@sarahboyce sarahboyce added selenium Apply to have Selenium tests run on a PR screenshots 🖼️ labels Jul 29, 2025
Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

Thank you for the PR ⭐

The styling of the legend is not right on mobile:

Screenshot from 2025-07-29 09-37-18

Also the AdminSplitDateTime widget (see django/contrib/admin/templates/admin/widgets/split_datetime.html) should have "Date" and "Time" in label tags associated with the appropriate inputs 👍

@@ -47,7 +47,7 @@ Requires core.js and SelectBox.js.
'p',
selector_available_title,
interpolate(gettext('Choose %s by selecting them and then select the "Choose" arrow button.'), [field_name]),
'class', 'helptext'
'id', `${field_id}_choose_helptext`, 'class', 'helptext'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the help text for the from box rather than the help text for the fieldset, so the from box should have the aria-describedby attribute (not the fieldset). Similar for the to box

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing out such an important detail.
Currently, when the select element is focused, it’s connected to the title via aria-labelledby, so both the label and a description are provided.
However, it might be better to provide the label via aria-labelledby, and then connect the help text and the instructions for selecting multiple options using aria-describedby.

warning.classList.add('help', warningClass);
warning.id = `${field_id}_timezone_warning_helptext`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this helptext should also be specific to the input rather than the fieldset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think the same. It seems better to associate it with each input element.

@knyghty knyghty requested a review from a team July 30, 2025 08:41
@Antoliny0919 Antoliny0919 force-pushed the ticket_35892 branch 2 times, most recently from 55815b2 to 5b17a6e Compare July 30, 2025 09:58
@Antoliny0919
Copy link
Contributor Author

New Accessibility Improvement Project 🎉🎉🎉

@Antoliny0919
Copy link
Contributor Author

Changed ..

  1. Apply use_fieldset to fields in the Admin ChangeForm.
  2. If a field has sub_help_text, provide a description when the associated node is accessed via aria-describedby.

When accessing each input field for DateTime, Date, and Time, a timezone explanation is provided.

Screenshot 2025-07-30 at 8 21 36 PM

For fields with filter_horizontal applied, the sub_help_text explaining how to select multiple items is provided in the selection boxes.

Screenshot 2025-07-30 at 8 23 32 PM

Previously, only the help text for the box type(from, to header) fields was provided.

@Antoliny0919 Antoliny0919 force-pushed the ticket_35892 branch 2 times, most recently from 9403679 to 93a290f Compare July 30, 2025 12:09
@Antoliny0919
Copy link
Contributor Author

After adding the label tag, the color of the Date and Time text became slightly lighter.
Would it be better to revert to the previous style?
It looks fine to me.

Before

Screenshot 2025-07-30 at 8 57 43 PM

After

Screenshot 2025-07-30 at 9 10 46 PM

@sarahboyce
Copy link
Contributor

Would it be better to revert to the previous style?
It looks fine to me.

I also think it looks fine. More consistent with other labels 🤷

Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

I noticed that the legends are styled black (rather than the grey styling of the labels), which needs an update

I did some testing with a screen reader and this is definitely an improvement to what we had 😁
I think this is getting close but I will let the accessibility team do more in depth testing

@Antoliny0919 Antoliny0919 force-pushed the ticket_35892 branch 2 times, most recently from 79d1a33 to a41ff76 Compare August 5, 2025 02:29
@Antoliny0919
Copy link
Contributor Author

Antoliny0919 commented Aug 5, 2025

I noticed that the legends are styled black (rather than the grey styling of the labels), which needs an update

I did some testing with a screen reader and this is definitely an improvement to what we had 😁 I think this is getting close but I will let the accessibility team do more in depth testing

Yes, I also definitely noticed the improvements when testing with a screen reader.
Thanks to the help of so many people recently, accessibility has come a long way. There's still more to do, but I'm really glad to see how much progress has been made 🙌
I’ll also take a look to see if there are any other widgets where using a fieldset might make sense.

@sarahboyce I was wondering if I could ask you for a small favor.
Would you be able to add the PRs I shared in the Discord chat to the accessibility improvement project?
It might be a bit of a hassle, but I think having them in the project could be helpful for contributors who are interested in accessibility 🥰

@sarahboyce
Copy link
Contributor

After adding the label tag, the color of the Date and Time text became slightly lighter. Would it be better to revert to the previous style? It looks fine to me.

Before

Screenshot 2025-07-30 at 8 57 43 PM ## After Screenshot 2025-07-30 at 9 10 46 PM

Thank you for the updates
For the new style, we would need to update the following docs screenshots:

  • docs/intro/_images/admin05t.png
  • docs/intro/_images/admin07.png
  • docs/intro/_images/admin08t.png

Labels are bold when they are a required field. I am not sure if labels of subwidgets of a required field should be bold 🤔
What do you think?

@Antoliny0919
Copy link
Contributor Author

Labels are bold when they are a required field. I am not sure if labels of subwidgets of a required field should be bold 🤔 What do you think?

Previously, the text was bold regardless of whether the field was required, but I actually think the lighter color looks better.
It feels more consistent 👍
So I prefer the current style.

@sarahboyce
Copy link
Contributor

Ok then can you update the screenshots?

@Antoliny0919
Copy link
Contributor Author

Ok then can you update the screenshots?

Of course! It’s late now, so I’ll make sure to add it tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
screenshots 🖼️ selenium Apply to have Selenium tests run on a PR
Projects
Development

Successfully merging this pull request may close these issues.

2 participants