Fixed #35189 -- Render admin collapsible fieldsets with <details>.

[MHLut]

TL;DR: Improve the accessibility of admin fieldsets and collapse them with a native HTML implementation.

• Ticket: #35189
• Proof of concept: Example 2: Details inside (no JS)
• Initial discussion on Discord, accessibility channel

Related tickets and PRs

Related items:

• Ticket #35382; this ticket removed unused CSS temporarily unearthed by this PR.
• Ticket #35380; documentation screenshots aren't being automatically generated.
• Added a high contrast mode to screenshot cases. #18080; add more forced-colors screenshots.

Important facts

• Works with the main model fieldset and stacked inline fieldsets.
• This change removes all JavaScript for collapsible fieldsets.
• I have recycled as much CSS as possible.
• JavaScript used to check if a fieldset could be collapsible. With JS gone, I moved that check to the Fieldset helper class in Python.
• The translations for "Show" and "Hide" are now redundant.

Known issues

• No existing CSS bugs were fixed.
  • Example: The font size of the fieldset is below 16px/1rem.
  • Example: The fieldset heading is hard to distinguish in forced-colors mode.
  • Context: Style for .collapse. h3 in forms.css was copied from .module h2, .module caption, .inline-group h2 in base.css.

Note: Several CSS fixes that we could make would cause scope creep beyond the collapsible fieldsets, so I recommend fixing them in separate PRs.

[carltongibson]

buildbot, test on selenium. I don’t think that works anymore 😅

[sarahboyce]

Hi @MHLut I think this looks really promising thank you ⭐
I've found something I'd like you to look into. I had some screenshots generated from this PR. You can see these here when you download the Artifacts.
These screenshots are taken under a few scenarios we try to support include dark mode and right to left languages (side note we should think about adding forced-colors mode to these).

I can see on your PR that in RTL we have this (notice that 'Collapsible' is on the left)

Previously 'Collapsible' was on the right:

I am also not sure if the collapse icon (> triangle thingy) should be on the left, so before Fields in the right to left case (maybe @sakhawy might be able to tell us 🙏 )

You might need to look into django/contrib/admin/static/admin/css/rtl.css 👍

[sarahboyce]

(on further thought, I think the triangle is in the correct place for RTL languages but Moe is welcome to confirm)

[sakhawy]

@sarahboyce I guess it "feels off" because we're used to seeing dropdowns with the triangle after the text and not before it :D
But I think the triangle here is part of the <details> tag's default style, so nothing to worry about!

One reason for checking the django/contrib/admin/static/admin/css/rtl.css file would be this line:

django/django/contrib/admin/static/admin/css/forms.css

Line 225 in 88274f6

text-align: left;

I think it's the reason 'Collapsible' is on the left when dir="rtl".

[MHLut]

@sarahboyce @sakhawy Thanks for the heads-up. I can't remember why I put the text-align: left in forms.py, but it's gone now. The heading aligns to the right side again in RTL mode.

The triangle is—as Moe mentioned—part of <details> native styling; I'd rather not mess with that. Since Moe seems OK with it, I'll keep that as-is.

FYI: I'm currently doing some free-form CSS experiments for forced-colors mode.

[knyghty]

I didn't look much at the code, or look at it on a screen.

From an accessibility standpoint I think we're good with this approach. The outline seems a little odd still (that there are multiple h3s per link, rather than one h3 and then going to h4 or something similar) but I think this is better than what we currently have and shouldn't block this merge - to be honest I'm not even sure if it's a problem at all, I could be totally wrong.

If I find time before someone else I will also review the code.

[MHLut]

@nessita The tests finally seem to pass, so that you can return to testing and reviewing!

I don't have much time for follow-ups, but I would like to see this merged before the feature freeze, so feel free to update the branch as you see fit.

@sakhawy might also be able to assist with issues that don't require knowing the entire PR history.

[nessita]

@nessita The tests finally seem to pass, so that you can return to testing and reviewing!

I don't have much time for follow-ups, but I would like to see this merged before the feature freeze, so feel free to update the branch as you see fit.

@sakhawy might also be able to assist with issues that don't require knowing the entire PR history.

Thank you so much @MHLut, I'll work in this PR tomorrow or Thu at the latest.

[nessita]

@MHLut Hey there! I have pushed some changes removing the definition and usage of heading_id because it felt inconsistent with the existing code, and also because it's usually a bad practice to rely on random numbers to generate unique IDs (random module does not guarantee uniqueness, even in large ranges). We could use UUID instead (which does guarantee uniqueness) but that feels a bit overkill for the branch's need, so I built the fieldset heading ID by using information available in the template (or its parent template), similar to what formsets already do.

@sarahboyce Would you fancy to re-review that commit (it's isolated in a second commit to ease review)? (Since I changed some of the already approved code.)

FYI I still need to review in detail tests and docs, but overall I think the branch is in very good shape.

[sarahboyce]

I didn't do a full review since the change of approach. 👍
This generally looks good, we're missing tests checking the logic that exists in the templates

[MHLut]

@nessita @sarahboyce Thank you for the heading ID update! I cannot review the ID changes in detail right now, but the only thing important to me is that they are predictable and unique since aria-labelledby relies on them for accessibility reasons.

A nice to have for IDs would be for them to be short and readable so that they are understandable when read from a browser URL.

[sarahboyce]

Looks good!
If we want to split commits I would have:

• setting the header levels and adding the aria-labelledby as "Associated fieldsets with their headers."
• updating collapse to use details/summary

[nessita]

Looks good! If we want to split commits I would have:

* setting the header levels and adding the `aria-labelledby` as "Associated fieldsets with their headers."

* updating collapse to use details/summary

This is a good proposal, thank you! I will try to work this out after our weekly sync.

[nessita]

I split the first commit into two logical commits.