Opened 8 years ago

Closed 8 years ago

#27386 closed Cleanup/optimization (fixed)

Readonly callable field is unconditionally wrapped inside <p>...</p>, which might create invalid HTML

Reported by: Jacob Rief Owned by: nobody
Component: contrib.admin Version: dev
Severity: Normal Keywords: callable field is_readonly <p>
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

(Pseudo)-Fields in classes inheriting from django.contrib.admin.ModelsAdmin which are callables, must be listed in readonly_fields. This implies that in admin/includes/fieldset.html (line 17) and admin/edit_inline/tabular.html (line 55) the content of this field is wrapped inside a paragraph <p>{{ field.contents }}</p>.

However, a <p>...</p> is not suitable to accept every kind of HTML element. Therefore when using a "callable" field, which renders it's content in HTML, one might get a surprising result.

Since the author of a callable field may wrap it's content into whatever (s)he likes, there should be a way to avoid these wrapping paragraphs.

My proposal is to check if field.contents is safe text, and if so then leave it as-is, or otherwise wrap it into <p>..</p> as we do it right now.

Change History (9)

comment:1 by Jacob Rief, 8 years ago

Keywords: is_readonly <p> added

comment:2 by Jacob Rief, 8 years ago

Keywords: callable added

comment:3 by Tim Graham, 8 years ago

Description: modified (diff)
Summary: Callable field is wrapped inside <p>...</p>Readonly callable field is unconditionally wrapped inside <p>...</p>, which might create invalid HTML
Triage Stage: UnreviewedAccepted

I'm not sure if the proposal is completely backwards-compatible, but the problem seems real.

comment:4 by Jacob Rief, 8 years ago

Yes, changing this could add a minor backwards compatibility problem; something probably not really noticeable and if, easy to fix.

I looked at the code. We presumably should add another tag to django.contrib.admin.helpers.AdminReadonlyField.init named is_callable which is True for callable fields. Then in the templates, we'd have to add another {% if field.is_callable %} and only wrap the content inside <p>..</p> it that's false.

If this proposal is accepted, I will implement it right now.

Version 0, edited 8 years ago by Jacob Rief (next)

comment:5 by Tim Graham, 8 years ago

I'm not sure. It doesn't seem obviously correct to assume that all callables might have HTML.

comment:6 by Jacob Rief, 8 years ago

Has patch: set
Needs tests: set
Resolution: fixed
Status: newclosed

PR https://github.com/django/django/pull/7477 to fix this ticket is pending.

comment:7 by Jacob Rief, 8 years ago

Needs tests: unset
Resolution: fixed
Status: closednew

comment:8 by Tim Graham, 8 years ago

Patch needs improvement: set

Some comments for improvement are on the PR.

comment:9 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: newclosed

In b3162ca:

Fixed #27386 -- Wrapped admin's readonly fields in <div> rather than <p>.

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