Code

Opened 4 years ago

Last modified 4 weeks ago

#13978 assigned New feature

Allow inline js/css in forms.Media

Reported by: nathforge Owned by: dmpayton
Component: Forms Version: master
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)

inlinemedia.diff (3.5 KB) - added by nathforge 4 years ago.
Patch
0001-Allow-inline-JS-CSS-in-forms.Media-13978.patch (7.7 KB) - added by dmpayton 17 months ago.
Updated patch for Django 1.5
13978-internal-media.patch (16.1 KB) - added by dmpayton 17 months ago.

Download all attachments as: .zip

Change History (24)

Changed 4 years ago by nathforge

Patch

comment:1 Changed 3 years ago by rafen

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to rafen
  • Patch needs improvement unset

comment:2 Changed 3 years ago by rafen

  • Owner changed from rafen to nobody

comment:3 Changed 3 years ago by mariarchi

  • Needs documentation set
  • Needs tests set
  • Triage Stage changed from Unreviewed to Design decision needed

Needs tests and docs [and maybe design decision]

comment:4 Changed 3 years ago by julien

  • Keywords sprintdec2010 added
  • milestone set to 1.3
  • Triage Stage changed from Design decision needed to 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:5 Changed 3 years ago by julien

  • milestone 1.3 deleted

Too late for 1.3.

comment:6 Changed 3 years ago by mk

This is sort-of a duplicate of #11865. Not closing as a duplicate because this ticket covers inline CSS and JS, while #11865 only covers inline JS.

comment:7 Changed 3 years ago by graham_king

  • Severity set to Normal
  • Type set to New feature

comment:8 Changed 3 years ago by julien

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.

Last edited 3 years ago by julien (previous) (diff)

comment:9 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:11 Changed 17 months ago by dmpayton

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?

Changed 17 months ago by dmpayton

Updated patch for Django 1.5

comment:12 Changed 17 months ago by dmpayton

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 Changed 17 months ago by julien

  • 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!

Changed 17 months ago by dmpayton

comment:14 Changed 17 months ago by dmpayton

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 Changed 17 months ago by julien

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 Changed 15 months ago by dmpayton

How do "EmbeddedCSS" and "EmbeddedJS" sound? Updated patch:

https://github.com/dmpayton/django/commit/a676b609004863dd726332df865b9ead7487767e

[Edit: updated URL]

Last edited 13 months ago by dmpayton (previous) (diff)

comment:17 Changed 15 months ago by dmpayton

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to dmpayton
  • Status changed from new to assigned

comment:18 Changed 13 months ago by julien

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 Changed 13 months ago by julien

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 Changed 11 months ago by dmpayton

comment:21 Changed 4 weeks ago by aaugustin

Related: #21987.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from dmpayton to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.