CSS glitch in the admin interface.

[kkoroviev@…]

​https://github.com/kkoroviev/django/tree/admin-css-fix

Before:

After:

[kkoroviev@…]

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.

[Greg Chapple]

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.

[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.

[Tim Graham]

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?

[Konstantin Koroviev]

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.

[Claude Paroz]

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

[Konstantin Koroviev]

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.

[Tim Graham <timograham@…>]

In e560558ecb3a727c8f3f23e3a036413f8d8679eb:

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