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 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions django/contrib/admin/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ def __init__(self, form, field, is_first):
self.is_first = is_first # Whether this field is first on the line
self.is_checkbox = isinstance(self.field.field.widget, forms.CheckboxInput)
self.is_readonly = False
self.is_fieldset = self.field.field.widget.use_fieldset

def label_tag(self):
classes = []
Expand All @@ -185,12 +186,14 @@ def label_tag(self):
if not self.is_first:
classes.append("inline")
attrs = {"class": " ".join(classes)} if classes else {}
tag = "legend" if self.is_fieldset else None
# checkboxes should not have a label suffix as the checkbox appears
# to the left of the label.
return self.field.label_tag(
contents=mark_safe(contents),
attrs=attrs,
label_suffix="" if self.is_checkbox else None,
tag=tag,
)

def errors(self):
Expand Down
21 changes: 20 additions & 1 deletion django/contrib/admin/static/admin/css/forms.css
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,13 @@ form .form-row p {

/* FORM LABELS */

label {
legend, label {
font-weight: normal;
color: var(--body-quiet-color);
font-size: 0.8125rem;
}

.required legend, legend.required,
.required label, label.required {
font-weight: bold;
}
Expand Down Expand Up @@ -91,6 +92,20 @@ fieldset .inline-heading,

/* ALIGNED FIELDSETS */

.aligned fieldset {
width: 100%;
border-top: none;
}

.aligned fieldset > div {
width: 100%;
}

.aligned legend {
float: left;
}

.aligned legend,
.aligned label {
display: block;
padding: 4px 10px 0 0;
Expand Down Expand Up @@ -138,6 +153,10 @@ form .aligned div.radiolist {
padding: 0;
}

form .aligned fieldset div.help {
margin-left: 0;
}

form .aligned p.help,
form .aligned div.help {
margin-top: 0;
Expand Down
2 changes: 2 additions & 0 deletions django/contrib/admin/static/admin/css/responsive.css
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ input[type="submit"], button {

/* Forms */

legend,
label {
font-size: 1rem;
}
Expand Down Expand Up @@ -481,6 +482,7 @@ input[type="submit"], button {
padding-top: 15px;
}

.aligned legend,
.aligned label {
width: 100%;
min-width: auto;
Expand Down
4 changes: 4 additions & 0 deletions django/contrib/admin/static/admin/css/widgets.css
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,10 @@ p.datetime {
font-weight: bold;
}

p.datetime label {
display: inline;
}

.datetime span {
white-space: nowrap;
font-weight: normal;
Expand Down
30 changes: 24 additions & 6 deletions django/contrib/admin/static/admin/js/SelectFilter2.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ Requires core.js and SelectBox.js.
const from_box = document.getElementById(field_id);
from_box.id += '_from'; // change its ID
from_box.className = 'filtered';
from_box.setAttribute('aria-labelledby', field_id + '_from_title');
from_box.setAttribute('aria-labelledby', field_id + '_from_label');
from_box.setAttribute('aria-describedby', `${field_id}_helptext ${field_id}_choose_helptext`);

for (const p of from_box.parentNode.getElementsByTagName('p')) {
if (p.classList.contains("info")) {
Expand All @@ -42,12 +43,20 @@ Requires core.js and SelectBox.js.
const selector_available_title = quickElement('div', selector_available);
selector_available_title.id = field_id + '_from_title';
selector_available_title.className = 'selector-available-title';
quickElement('label', selector_available_title, interpolate(gettext('Available %s') + ' ', [field_name]), 'for', field_id + '_from');
quickElement(
'label',
selector_available_title,
interpolate(gettext('Available %s') + ' ', [field_name]),
'id',
field_id + '_from_label',
'for',
field_id + '_from'
);
quickElement(
'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.

);

const filter_p = quickElement('p', selector_available, '', 'id', field_id + '_filter');
Expand Down Expand Up @@ -102,12 +111,20 @@ Requires core.js and SelectBox.js.
const selector_chosen_title = quickElement('div', selector_chosen);
selector_chosen_title.className = 'selector-chosen-title';
selector_chosen_title.id = field_id + '_to_title';
quickElement('label', selector_chosen_title, interpolate(gettext('Chosen %s') + ' ', [field_name]), 'for', field_id + '_to');
quickElement(
'label',
selector_chosen_title,
interpolate(gettext('Chosen %s') + ' ', [field_name]),
'id',
field_id + '_to_label',
'for',
field_id + '_to'
);
quickElement(
'p',
selector_chosen_title,
interpolate(gettext('Remove %s by selecting them and then select the "Remove" arrow button.'), [field_name]),
'class', 'helptext'
'id', `${field_id}_remove_helptext`, 'class', 'helptext'
);

const filter_selected_p = quickElement('p', selector_chosen, '', 'id', field_id + '_filter_selected');
Expand All @@ -134,7 +151,8 @@ Requires core.js and SelectBox.js.
'multiple', '',
'size', from_box.size,
'name', from_box.name,
'aria-labelledby', field_id + '_to_title',
'aria-labelledby', field_id + '_to_label',
'aria-describedby', `${field_id}_helptext ${field_id}_remove_helptext`,
'class', 'filtered'
);
const warning_footer = quickElement('div', selector_chosen, '', 'class', 'list-footer-display');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,10 @@
message = interpolate(message, [timezoneOffset]);

const warning = document.createElement('div');
const id = inp.id;
const field_id = inp.closest('p.datetime') ? id.slice(0, id.lastIndexOf("_")) : id;
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.

warning.textContent = message;
inp.parentNode.appendChild(warning);
},
Expand Down
6 changes: 4 additions & 2 deletions django/contrib/admin/templates/admin/includes/fieldset.html
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,14 @@
<div class="form-row{% if line.fields|length == 1 and line.errors %} errors{% endif %}{% if not line.has_visible_field %} hidden{% endif %}{% for field in line %}{% if field.field.name %} field-{{ field.field.name }}{% endif %}{% endfor %}">
{% if line.fields|length == 1 %}{{ line.errors }}{% else %}<div class="flex-container form-multiline">{% endif %}
{% for field in line %}
{% if field.is_fieldset %}<fieldset class="flex-container"{% if field.field.help_text %} aria-describedby="{{ field.field.id_for_label }}_helptext"{% endif %}>{{ field.label_tag }}{% endif %}
<div>
{% if not line.fields|length == 1 and not field.is_readonly %}{{ field.errors }}{% endif %}
<div class="flex-container{% if not line.fields|length == 1 %} fieldBox{% if field.field.name %} field-{{ field.field.name }}{% endif %}{% if not field.is_readonly and field.errors %} errors{% endif %}{% if field.field.is_hidden %} hidden{% endif %}{% endif %}{% if field.is_checkbox %} checkbox-row{% endif %}">
{% if field.is_checkbox %}
{{ field.field }}{{ field.label_tag }}
{{ field.field }}{% if not field.is_fieldset %}{{ field.label_tag }}{% endif %}
{% else %}
{{ field.label_tag }}
{% if not field.is_fieldset %}{{ field.label_tag }}{% endif %}
{% if field.is_readonly %}
<div class="readonly">{{ field.contents }}</div>
{% else %}
Expand All @@ -31,6 +32,7 @@
</div>
{% endif %}
</div>
{% if field.is_fieldset %}</fieldset>{% endif %}
{% endfor %}
{% if not line.fields|length == 1 %}</div>{% endif %}
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<p class="datetime">
{{ date_label }} {% with widget=widget.subwidgets.0 %}{% include widget.template_name %}{% endwith %}<br>
{{ time_label }} {% with widget=widget.subwidgets.1 %}{% include widget.template_name %}{% endwith %}
<label {% if widget.attrs.id %}for="{{ widget.subwidgets.0.attrs.id }}"{% endif %}>{{ date_label }}</label> {% with widget=widget.subwidgets.0 %}{% include widget.template_name %}{% endwith %}<br>
<label {% if widget.attrs.id %}for="{{ widget.subwidgets.1.attrs.id }}"{% endif %}>{{ time_label }}</label> {% with widget=widget.subwidgets.1 %}{% include widget.template_name %}{% endwith %}
</p>
19 changes: 17 additions & 2 deletions django/contrib/admin/widgets.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,16 @@ def get_context(self, name, value, attrs):
return context


class BaseAdminDateWidget(forms.DateInput):
class DateTimeWidgetContextMixin:
def get_context(self, name, value, attrs):
context = super().get_context(name, value, attrs)
context["widget"]["attrs"][
"aria-describedby"
] = f"id_{name}_timezone_warning_helptext"
return context


class BaseAdminDateWidget(DateTimeWidgetContextMixin, forms.DateInput):
class Media:
js = [
"admin/js/calendar.js",
Expand All @@ -65,7 +74,7 @@ class AdminDateWidget(BaseAdminDateWidget):
template_name = "admin/widgets/date.html"


class BaseAdminTimeWidget(forms.TimeInput):
class BaseAdminTimeWidget(DateTimeWidgetContextMixin, forms.TimeInput):
class Media:
js = [
"admin/js/calendar.js",
Expand Down Expand Up @@ -98,8 +107,13 @@ def get_context(self, name, value, attrs):
context = super().get_context(name, value, attrs)
context["date_label"] = _("Date:")
context["time_label"] = _("Time:")
for widget in context["widget"]["subwidgets"]:
widget["attrs"]["aria-describedby"] = f"id_{name}_timezone_warning_helptext"
return context

def id_for_label(self, id_):
return id_


class AdminRadioSelect(forms.RadioSelect):
template_name = "admin/widgets/radio.html"
Expand Down Expand Up @@ -282,6 +296,7 @@ def __init__(
self.can_view_related = not multiple and can_view_related
# To check if the related object is registered with this AdminSite.
self.admin_site = admin_site
self.use_fieldset = True

def __deepcopy__(self, memo):
obj = copy.copy(self)
Expand Down
1 change: 1 addition & 0 deletions django/forms/widgets.py
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,7 @@ class ClearableFileInput(FileInput):
input_text = _("Change")
template_name = "django/forms/widgets/clearable_file_input.html"
checked = False
use_fieldset = True

def clear_checkbox_name(self, name):
"""
Expand Down
12 changes: 11 additions & 1 deletion js_tests/admin/DateTimeShortcuts.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,20 @@ QUnit.test('custom time shortcuts', function(assert) {
QUnit.test('time zone offset warning', function(assert) {
const $ = django.jQuery;
const savedOffset = $('body').attr('data-admin-utc-offset');
const timeField = $('<input type="text" name="time_test" class="vTimeField">');
// Single date or time field.
const timeField = $('<input id="id_updated_at" type="text" name="updated_at" class="vTimeField">');
$('#qunit-fixture').append(timeField);
$('body').attr('data-admin-utc-offset', new Date().getTimezoneOffset() * -60 + 3600);
DateTimeShortcuts.init();
$('body').attr('data-admin-utc-offset', savedOffset);
assert.equal($('.timezonewarning').text(), 'Note: You are 1 hour behind server time.');
assert.equal($('.timezonewarning').attr("id"), "id_updated_at_timezone_warning_helptext");
$('#qunit-fixture').empty();
// DateTimeField with fieldset containing date and time inputs.
$('#qunit-fixture').append(
'<p class="datetime"><input id="id_updated_at_0" type="text" name="updated_at_0" class="vTimeField"></p>'
);
$('body').attr('data-admin-utc-offset', new Date().getTimezoneOffset() * -60 + 3600);
DateTimeShortcuts.init();
assert.equal($('.timezonewarning').attr("id"), "id_updated_at_timezone_warning_helptext");
});
8 changes: 6 additions & 2 deletions js_tests/admin/SelectFilter2.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ QUnit.test('init', function(assert) {
assert.equal($('#test').children().first().prop("tagName"), "DIV");
assert.equal($('#test').children().first().attr("class"), "selector");
assert.equal($('.selector-available label').text().trim(), "Available things");
assert.equal($('.selector-available label').attr("id"), "id_from_label");
assert.equal($('.selector-chosen label').text().trim(), "Chosen things");
assert.equal($('.selector-chosen label').attr("id"), "id_to_label");
assert.equal($('.selector-chosen select')[0].getAttribute('multiple'), '');
assert.equal($('.selector-chooseall').text(), "Choose all things");
assert.equal($('.selector-chooseall').prop("tagName"), "BUTTON");
Expand All @@ -23,10 +25,12 @@ QUnit.test('init', function(assert) {
assert.equal($('.selector-remove').prop("tagName"), "BUTTON");
assert.equal($('.selector-clearall').text(), "Remove all things");
assert.equal($('.selector-clearall').prop("tagName"), "BUTTON");
assert.equal($('.selector-available .filtered').attr("aria-labelledby"), "id_from_title");
assert.equal($('.selector-available .filtered').attr("aria-labelledby"), "id_from_label");
assert.equal($('.selector-available .filtered').attr("aria-describedby"), "id_helptext id_choose_helptext");
assert.equal($('.selector-available .selector-available-title label').text(), "Available things ");
assert.equal($('.selector-available .selector-available-title .helptext').text(), 'Choose things by selecting them and then select the "Choose" arrow button.');
assert.equal($('.selector-chosen .filtered').attr("aria-labelledby"), "id_to_title");
assert.equal($('.selector-chosen .filtered').attr("aria-labelledby"), "id_to_label");
assert.equal($('.selector-chosen .filtered').attr("aria-describedby"), "id_helptext id_remove_helptext");
assert.equal($('.selector-chosen .selector-chosen-title label').text(), "Chosen things ");
assert.equal($('.selector-chosen .selector-chosen-title .helptext').text(), 'Remove things by selecting them and then select the "Remove" arrow button.');
assert.equal($('.selector-filter label .help-tooltip')[0].getAttribute("aria-label"), "Type into this box to filter down the list of available things.");
Expand Down
2 changes: 2 additions & 0 deletions tests/admin_views/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
Color2,
ComplexSortedPerson,
Country,
Course,
CoverLetter,
CustomArticle,
CyclicOne,
Expand Down Expand Up @@ -1278,6 +1279,7 @@ class CamelCaseAdmin(admin.ModelAdmin):
site.register(Pizza, PizzaAdmin)
site.register(ReadOnlyPizza, ReadOnlyPizzaAdmin)
site.register(ReadablePizza)
site.register(Course)
site.register(Topping, ToppingAdmin)
site.register(Album, AlbumAdmin)
site.register(Song)
Expand Down
16 changes: 16 additions & 0 deletions tests/admin_views/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,22 @@ def __str__(self):
return self.name


class Course(models.Model):
DIFFICULTY_CHOICES = [
("beginner", "Beginner Class"),
("intermediate", "Intermediate Class"),
("advanced", "Advanced Class"),
]

title = models.CharField(max_length=100)
materials = models.FileField(upload_to="test_upload")
difficulty = models.CharField(
max_length=20, choices=DIFFICULTY_CHOICES, null=True, blank=True
)
categories = models.ManyToManyField(Category, blank=True)
start_datetime = models.DateTimeField(null=True, blank=True)


class Topping(models.Model):
name = models.CharField(max_length=20)

Expand Down
25 changes: 25 additions & 0 deletions tests/admin_views/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
Color,
ComplexSortedPerson,
Country,
Course,
CoverLetter,
CustomArticle,
CyclicOne,
Expand Down Expand Up @@ -6908,6 +6909,30 @@ def test_redirect_on_add_view_continue_button(self):
name_input_value = name_input.get_attribute("value")
self.assertEqual(name_input_value, "Test section 1")

def test_use_fieldset_fields_render(self):
from selenium.webdriver.common.by import By

self.admin_login(
username="super", password="secret", login_url=reverse("admin:index")
)
course = Course.objects.create(
title="Django Class", materials="django_documents"
)
expected_legend_tags_text = [
"Materials:",
"Difficulty:",
"Categories:",
"Start datetime:",
]
url = reverse("admin:admin_views_course_change", args=(course.pk,))
self.selenium.get(self.live_server_url + url)
fieldsets = self.selenium.find_elements(
By.CSS_SELECTOR, "fieldset.aligned fieldset"
)
for fieldset in fieldsets:
legend = fieldset.find_element(By.TAG_NAME, "legend")
self.assertIn(legend.text, expected_legend_tags_text)

@screenshot_cases(["desktop_size", "mobile_size", "rtl", "dark", "high_contrast"])
@override_settings(MESSAGE_LEVEL=10)
def test_messages(self):
Expand Down
Loading
Loading