Opened 14 years ago
Closed 8 years ago
#13978 closed New feature (wontfix)
Allow inline js/css in forms.Media
Reported by: | Nathan Reynolds | Owned by: | Derek Payton |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Normal | Keywords: | sprintdec2010 |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
It's not possible to use the Media class for inline Javascript or CSS content, though this is often useful when bootstrapping external scripts.
Widgets can include inline content in their render method, but ModelAdmin's appear to be out of luck.
I've attached a patch to allow inline content, e.g:
@property def media(self): return forms.Media( js=(forms.Media.Inline('alert("Test")'),), css={ 'all': (forms.Media.Inline('body { background: black }'),), } )
Attachments (3)
Change History (25)
by , 14 years ago
Attachment: | inlinemedia.diff added |
---|
comment:1 by , 14 years ago
Owner: | changed from | to
---|
comment:2 by , 14 years ago
Owner: | changed from | to
---|
comment:3 by , 14 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Triage Stage: | Unreviewed → Design decision needed |
Needs tests and docs [and maybe design decision]
comment:4 by , 14 years ago
Keywords: | sprintdec2010 added |
---|---|
milestone: | → 1.3 |
Triage Stage: | Design decision needed → Accepted |
This sounds like a really useful feature. You'd need to add some tests and documentation if you want it to have a chance of shipping with 1.3?
comment:6 by , 14 years ago
comment:7 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:8 by , 14 years ago
I went on and closed #11865 as a duplicate. I believe it's best to tackle both css and js at once to maintain some consistency.
comment:11 by , 12 years ago
I've updated this patch for the latest Django. Assuming I were to write docs and a test, could this feasibly make it into 1.5?
by , 12 years ago
Attachment: | 0001-Allow-inline-JS-CSS-in-forms.Media-13978.patch added |
---|
Updated patch for Django 1.5
comment:12 by , 12 years ago
I've updated with a patch that contains a first pass at docs and tests.
[15:44:41] derek @ pangolin in ~/dev/django/tests (git:feature/13978-inline-media) ± python runtests.py --settings=test_sqlite forms.FormsMediaTestCase Creating test database for alias 'default'... Creating test database for alias 'other'... ............... ---------------------------------------------------------------------- Ran 15 tests in 0.011s OK Destroying test database for alias 'default'... Destroying test database for alias 'other'...
comment:13 by , 12 years ago
Patch needs improvement: | set |
---|
Thank you for the patch, it looks great! I just have a couple of comments.
Instead of having a single internal class Media.Inline
, it'd be better to have two standalone (and global at the forms.widgets level) classes, one for JS and one for CSS. This way we could potentially later add extra features specific to either JS or CSS (e.g. to control the "media" attribute for the CSS code).
Also, I'm wondering if we could find a better name than "inline". My concerns are that it could be confused for the admin inlines, and that it could be confused for another type of inline code, e.g. <div style="color:#fff"/>
. Maybe something like BlockJS
, BlockCSS
. Any suggestion for better names are welcome.
Thanks!
by , 12 years ago
Attachment: | 13978-internal-media.patch added |
---|
comment:14 by , 12 years ago
Hi Julien,
Thanks for the feedback! I've added a new patch that switches naming from "inline" to "internal" and adds separate top-level classes for CSS and JS.
https://github.com/dmpayton/django/compare/dmpayton:master...dmpayton:feature/13978-internal-media
And, of course, the tests still pass:
[19:17:54] derek @ pangolin in ~/dev/django/tests (git:feature/13978-internal-media) ± python runtests.py --settings=test_sqlite forms.FormsMediaTestCase Creating test database for alias 'default'... Creating test database for alias 'other'... ............... ---------------------------------------------------------------------- Ran 15 tests in 0.011s OK Destroying test database for alias 'default'... Destroying test database for alias 'other'...
Please let me know if there's anything else I can do.
Thanks!
comment:15 by , 12 years ago
Thanks for the updated patch, Derek. We're past the beta so unfortunately this can't go in 1.5, but I'm definitely keen to push this forward ASAP to make sure it gets considered for 1.6. I'll try to review the patch in more detail soon.
Just one quick note in passing: I don't really like the term "Internal" as it's a little vague. I'll do more thinking about it and this can easily be modified in the final commit. In the meantime, any other suggestions are welcome.
Thanks again!
comment:16 by , 12 years ago
How do "EmbeddedCSS" and "EmbeddedJS" sound? Updated patch:
https://github.com/dmpayton/django/commit/a676b609004863dd726332df865b9ead7487767e
[Edit: updated URL]
comment:17 by , 12 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Owner: | changed from | to
Status: | new → assigned |
comment:18 by , 12 years ago
Thanks for your work, Derek. The patch looks pretty good! I like the term "embedded" a lot more. Since it's a pretty important API addition I'll just seek more opinions on IRC, and then I'm hoping we can move forward with this soon.
comment:19 by , 12 years ago
This ticket was brought on in IRC. A good point was made by @apollo13 that a new W3C policy is being worked on to discourage developers from using inline JS, in particular to prevent the risk of XSS: the Content Security Policy http://www.w3.org/TR/CSP/
While I do recognize the convenience of inline JS/CSS, this is an important question that needs more discussing, especially around whether Django core should allow this feature which seems to now go against recommended practices. At this point I'd recommend bringing this to the django-devs mailing list to gather more feedback.
comment:20 by , 11 years ago
Thanks Julien, I've posted this to the mailing list.
https://groups.google.com/forum/?fromgroups=#!topic/django-developers/G5_W3_MdmIg
comment:22 by , 8 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
Closing as wontfix given the lack of activity/interest and because it isn't compatible with CSP.
Patch