#22746 closed Bug (fixed)

CSS glitch in the admin interface.

Reported by: kkoroviev@… Owned by: kkoroviev@…
Component: contrib.admin Version: 1.6
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: yes

Change History (9)

comment:1 Changed 11 months ago by kkoroviev@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

You may find this test kinda interesting. I used that test to generate both screenshots automatically.

You might not want to merge that test into the main test suite. I enjoyed writing it, anyway.

comment:2 Changed 11 months ago by gchp

  • Patch needs improvement set

This seems like a real edge case to me. From reading the patch, this only occurs when you replace the quotes in the success message with <code> tags. I'll defer to some of the core devs on whether it's worth fixing this or not.

Just in terms of your patch. Have a read through the commit guidelines: https://docs.djangoproject.com/en/dev/internals/contributing/committing-code/#committing-guidelines. Patches should consist of single commits (multiple commits can be squashed before submitting), and commit messages should be in the format described in the above link.

Also, I don't think that it's a good idea to generate screenshots in the test suite. Doing so means that whenever anyone runs the full suite a new screenshot will be generated. I think the screenshots may be useful for debugging a test, but shouldn't be included in the final version.

I'm marking this as needing improvement, but I won't change the review status as I'm not sure if an edge case like this warrants a fix. I'll leave that to others.

comment:3 Changed 11 months ago by kkoroviev@…

this only occurs when you replace the quotes in the success message with <code> tags

Not only. You can create an admin action that will use django.contrib.admin.ModelAdmin.message_user() to output something like mark_safe("All <code>&lt;script&gt;</code> tags in your messages have been removed successfully.") and the user will get: "All <script> tags in your messages have been removed successfully.". Yet I decided to override message_user() to get around Django's inability to provide an easy way to redefine default admin messages. I in no way suppose that message_user() overriding is a proper way to handle the task of redefining the default admin messages. Nevertheless, there is no any other option available (except for modifying the admin source code, of course).

Also, I don't think that it's a good idea to generate screenshots in the test suite.

I totally agree with you. That test is nothing more than a proof of concept (or should I say: me fiddling with Selenium). If there were a need for such automated visual tests, I would probably create an additional option for runtests.py, something like --save-screenshots-to <screenshot-dir>, and decorate all screenshot-taking tests appropriately to not run when they are not needed.

I'm not sure if an edge case like this warrants a fix.

I do not consider this bug an edge case. I saw this glitch in at least two other people's projects.

comment:4 Changed 11 months ago by timo

I don't think a test is really needed for a change like this, but I agree it's interesting.

That CSS line has been in Django since 2008, however, so I'm not confident we can simply remove it without breaking its intended purpose in the first place. Did you have any thoughts on that?

comment:5 Changed 11 months ago by timo

  • Easy pickings unset
  • Triage Stage changed from Unreviewed to Accepted

comment:6 Changed 11 months ago by kkoroviev

That CSS line has been in Django since 2008

We need to go even deeper: https://github.com/django/django/commit/dd5320d1d56ca7603747dd68871e72eee99d9e67#diff-0c5cb7e69e6e0dcf5644a2809cbccb53R65. It has been in Django since 16 July 2005. According to Wikipedia, Django was released for the first time on 21 July 2005. I suppose I won't be wrong if I say that this line has been in Django since Django's inception.

comment:7 Changed 11 months ago by claudep

Adding it in Django master now gives us time to notice a possible unplanned effect until release...

comment:8 Changed 11 months ago by kkoroviev

Another option is to change that line to background-color: inherit;, which I deem to be its intended purpose. Yet I see no difference between inherit and transparent in terms of background-color. The default value for background-color is transparent.

comment:9 Changed 11 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In e560558ecb3a727c8f3f23e3a036413f8d8679eb:

Fixed #22746 -- Removed background property from pre/code in admin CSS.

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