Opened 6 months ago

Closed 3 months ago

#35404 closed Bug (fixed)

Admin fieldset multiple flexbox regressions

Reported by: minusf Owned by:
Component: contrib.admin Version: 4.2
Severity: Normal Keywords: css admin fieldset accessibility
Cc: minusf Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description (last modified by minusf)

Hello. I have noticed a couple regressions in the admin fieldset css. I think they are all related to switching <div class="form-row"> to flexbox.

  1. input type=text boxes on the right side grow in height if the label doesn't fit into 160px.


  1. long labels have uncovered another issue: incorrect line height and a forced height in general. After applying the following patch:
M django/contrib/admin/static/admin/css/forms.css
@@ -84,14 +84,13 @@ form ul.inline li {
     min-width: 160px;
     width: 160px;
     word-wrap: break-word;
-    line-height: 1;
+    line-height: 1.5;
 }
 
 .aligned label:not(.vCheckboxLabel):after {
     content: '';
     display: inline-block;
     vertical-align: middle;
-    height: 1.625rem;
 }
 
 .aligned label + p, .aligned .checkbox-row + div.help, .aligned label + div.readonly {

long labels looks more readable:


  1. checkboxes are not aligned with their labels. I don't have a patch for this, only 2 screenshots:

with flexbox:


without flexbox:


Attachments (8)

flexbox-input.png (81.3 KB ) - added by minusf 6 months ago.
long-labels-with-patch.png (82.2 KB ) - added by minusf 6 months ago.
unaligned-checkbox.png (11.3 KB ) - added by minusf 6 months ago.
checkbox-wo-flexbox.png (11.0 KB ) - added by minusf 6 months ago.
image-20240425-090901.png (27.5 KB ) - added by Sarah Boyce 6 months ago.
image-20240425-092610.png (15.2 KB ) - added by Sarah Boyce 6 months ago.
image-20240429-085114.png (41.7 KB ) - added by Sarah Boyce 6 months ago.
admin-help-text.png (21.8 KB ) - added by minusf 6 months ago.

Download all attachments as: .zip

Change History (26)

by minusf, 6 months ago

Attachment: flexbox-input.png added

by minusf, 6 months ago

Attachment: long-labels-with-patch.png added

by minusf, 6 months ago

Attachment: unaligned-checkbox.png added

by minusf, 6 months ago

Attachment: checkbox-wo-flexbox.png added

comment:1 by minusf, 6 months ago

Description: modified (diff)

by Sarah Boyce, 6 months ago

Attachment: image-20240425-090901.png added

by Sarah Boyce, 6 months ago

Attachment: image-20240425-092610.png added

comment:2 by Sarah Boyce, 6 months ago

Triage Stage: Unreviewed โ†’ Accepted
Version: 5.0 โ†’ 4.2

Thank you for the report and screenshots! Replicated ๐Ÿ‘

For reference, this is how these examples looked on 4.1 (ignore the red dot)

1 and 3 are regressions from 96a598356a9ea8c2c05b22cadc12e256a3b295fd, 2 is a long standing issue.

Unsure what is the best option for 1, because the grow behaviour looks better when there is help text.
Happy to receive suggestions or to default to the previous behaviour.

comment:3 by minusf, 6 months ago

Thank your for looking into this. Interesting that for so long nobody reported this.

I am sorry but I think the help_text is not helping much, personally my eyes are still bleeding ;-)

If you are happy with the patch for the height issue (no 2.), I can start a pull request and work from there.

The first couple of "fixes" I tried were not effective and it seems like I have to dive into flex myself much deeper as my understanding of it is flawed.

by Sarah Boyce, 6 months ago

Attachment: image-20240429-085114.png added

comment:4 by Sarah Boyce, 6 months ago

If you are happy with the patch for the height issue (no 2.), ...

I think you can also remove the line-height

M django/contrib/admin/static/admin/css/forms.css
@@ -84,14 +84,13 @@ form ul.inline li {
     min-width: 160px;
     width: 160px;
     word-wrap: break-word;
-    line-height: 1;
 }
 
 .aligned label:not(.vCheckboxLabel):after {
     content: '';
     display: inline-block;
     vertical-align: middle;
-    height: 1.625rem;
 }

Looks ok to me

..., I can start a pull request and work from there.

Please do! I also think (2) should be a separate commit to (1) and (3) (maybe a commit message something like... Refs #35404 -- Fixed line height of admin fieldset labels.)

comment:5 by minusf, 6 months ago

I think you can also remove the line-height

I might be wrong but afaik multiline paragraphs strongly benefit in readability with line-height at least 1.2 to 1.5 depending on font type... This is an actual issue in the admin how it shows help_text as well and I meant to send a patch for that for years...


Last edited 6 months ago by minusf (previous) (diff)

by minusf, 6 months ago

Attachment: admin-help-text.png added

comment:6 by Sarah Boyce, 6 months ago

strongly benefit in readability with line-height at least 1.2 to 1.5 depending on font type

I agree 1 is too small ๐Ÿ‘
In case I was misunderstood, for the css we were looking at line-height: 1; was overriding an existing larger line-height. Generally, I would like our css have good defaults and not have too much being overriden for particular classes/elements.

in reply to:  6 comment:7 by minusf, 6 months ago

Replying to Sarah Boyce:

In case I was misunderstood, for the css we were looking at line-height: 1; was overriding an existing larger line-height. Generally, I would like our css have good defaults and not have too much being overriden for particular classes/elements.

Yes, I see what you mean. Removing it sets line-height: normal (by user agent), which according to mdn is "a default value of roughly 1.2, depending on the element's font-family"...

โ€‹https://github.com/django/django/pull/18151

comment:8 by minusf, 6 months ago

Has patch: set

comment:9 by minusf, 6 months ago

Regarding help_text, I think that with this small font line-height: normal is not enough:

M django/contrib/admin/static/admin/css/base.css
@@ -268,6 +268,7 @@ hr {
 .help, p.help, form p.help, div.help, form div.help, div.help li {
     font-size: 0.6875rem;
     color: var(--body-quiet-color);
+    line-height: 1.5;
 }
ยท
 div.help ul {

in reply to:  9 comment:10 by Sarah Boyce, 6 months ago

Keywords: accessibility added

Replying to minusf:

Regarding help_text, I think that with this small font line-height: normal is not enough:

M django/contrib/admin/static/admin/css/base.css
@@ -268,6 +268,7 @@ hr {
 .help, p.help, form p.help, div.help, form div.help, div.help li {
     font-size: 0.6875rem;
     color: var(--body-quiet-color);
+    line-height: 1.5;
 }
ยท
 div.help ul {

I don't think, taking into account the way the admin is already, this css change clearly fixes an issue/bug.

I think a wider discussion on whether the fonts and spacing in the admin is too small (with the accessibility team involved) is a good idea, but out of scope of this ticket.
There are many small tweaks we can make to css and there is a lot of subjectivity around what looks "better", so let's fix the issue as reported and create further discussions as necessary. Then the changes we make are scoped out and made to all required areas at once, rather than a series of many small tweaks requiring many reviews. I hope that makes sense ๐Ÿ‘

comment:11 by Sarah Boyce <42296566+sarahboyce@โ€ฆ>, 6 months ago

In dd46cab:

Refs #35404 -- Fixed the line height of admin fieldset labels.

comment:12 by Sarah Boyce, 6 months ago

Has patch: unset

comment:13 by Vaarun Sinha, 5 months ago

Owner: changed from nobody to Vaarun Sinha
Status: new โ†’ assigned

comment:14 by Vaarun Sinha, 5 months ago

โ€‹https://github.com/VaarunSinha/contribution-notes/blob/main/django/ticket_35404/notes.md

The checkbox issue has been fixed, and these are my contribution notes.

I will be committing and creating a PR soon.

comment:15 by Sarah Boyce <42296566+sarahboyce@โ€ฆ>, 5 months ago

In 9691a00d:

Refs #35404 -- Fixed padding of admin fieldset checkbox label.

comment:16 by Sarah Boyce <42296566+sarahboyce@โ€ฆ>, 5 months ago

In ba81b3f0:

[5.1.x] Refs #35404 -- Fixed padding of admin fieldset checkbox label.

Backport of 9691a00d5839e6137a2716526277013af9ee97ff from main.

comment:17 by Sarah Boyce, 3 months ago

Owner: Vaarun Sinha removed
Status: assigned โ†’ new

comment:18 by Sarah Boyce, 3 months ago

Resolution: โ†’ fixed
Status: new โ†’ closed

Note that only issue 1 is remaining and that this issue would be resolved by #34643 (which has a mature patch)
As such I will close this ticket and the remaining work can be done in #34643

Note: See TracTickets for help on using tickets.
Back to Top