Opened 10 years ago
Closed 10 years ago
#22746 closed Bug (fixed)
CSS glitch in the admin interface.
Reported by: | Owned by: | ||
---|---|---|---|
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 |
Description
Change History (9)
comment:1 by , 10 years ago
comment:2 by , 10 years ago
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 by , 10 years ago
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><script></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 by , 10 years ago
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 by , 10 years ago
Easy pickings: | unset |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:6 by , 10 years ago
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 by , 10 years ago
Adding it in Django master now gives us time to notice a possible unplanned effect until release...
comment:8 by , 10 years ago
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 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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.