Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#11967 closed (fixed)

[patch] Firefox 3.5/TinyMCE collision breaks DateTimeShortcuts.js in admin

Reported by: Daniel Roseman Owned by: Jacob
Component: contrib.admin Version: 1.1
Severity: Keywords:
Cc: smulloni@…, olau@…, justin@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The combination of Firefox 3.5 with TinyMCE on an admin add/edit screen which contains a date or time field causes the calendar/clock icons not to appear, and spurious 404/500 errors to appear in logs.

I have reproduced this on Mac and Linux (Ubuntu), and more reports are here:
http://groups.google.com/group/django-users/browse_thread/thread/14beb2d0cab45257
http://groups.google.com/group/django-users/browse_thread/thread/be50deac49247f80

The cause appears to be the way DateTimeShortcuts.js works out the value of admin_media_prefix, by searching for its own name in the list of scripts returned by document.getElementsByTagName('script'). TinyMCE dynamically injects various scripts into the page, and some issue with Firefox 3.5 causes the it to no longer return the full list of scripts in the DOM. Therefore, DateTimeShortcuts.js is not found, and the script incorrectly uses a blank value as the media prefix, leading to it requesting the icons via a relative path from the current page - eg /admin/app/model/1234/img/admin/icon_calendar.gif - which causes a ValueError: invalid literal for int() with base 10 in Django.

Although this is clearly a bug in Firefox and/or TinyMCE, I think it's worth fixing in Django anyway as the way DateTimeShortcuts works out admin_media_prefix is quite crufty, and using TinyMCE in the admin is fairly popular. The proposed patch simply defines a global admin_media_prefix variable in the change_form template, and DateTimeShortcuts.js uses that if defined (it still falls back to the old method otherwise). Having admin_media_prefix available to all scripts would seem like a good idea in any case.

Attachments (2)

admin_media.diff (2.3 KB) - added by Daniel Roseman 7 years ago.
admin_media.2.diff (2.3 KB) - added by Ole Laursen 7 years ago.
Bugfixed patch

Download all attachments as: .zip

Change History (11)

Changed 7 years ago by Daniel Roseman

Attachment: admin_media.diff added

comment:1 Changed 7 years ago by anonymous

Cc: smulloni@… added
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 7 years ago by lakin@…

This is driving me nuts too. It wouldn't be so bad if this was a 404, but combined with #11191 and #3785 it generates a ton of 500 errors. :/

Any chance this will be in 1.0.5 or 1.1.2?

comment:3 Changed 7 years ago by Jacob

milestone: 1.2
Owner: changed from nobody to Jacob
Status: newassigned
Triage Stage: UnreviewedAccepted

comment:4 Changed 7 years ago by Ole Laursen

There was a bug in that patch. admin_media_prefix is used as a variable with {{ admin_media_prefix }} instead of as a tag {% admin_media_prefix %}. Here's a fixed patch.

Changed 7 years ago by Ole Laursen

Attachment: admin_media.2.diff added

Bugfixed patch

comment:5 Changed 7 years ago by Ole Laursen

Cc: olau@… added

comment:6 Changed 7 years ago by Justin Lilly

Cc: justin@… added

comment:7 Changed 6 years ago by Luke Plant

I think it is much better to not fall back to the current method - a single mechanism that works completely or not at all is far preferable to a situation where you can forget part of the solution and it still works most of the time.

However, the single mechanism needs to be robust, and it isn't yet - every time you use DateTimeShortcuts.js you have to remember to add that var definition to the template. How about just putting that var definition in 'admin/base.html'?

comment:8 Changed 6 years ago by Jacob

Resolution: fixed
Status: assignedclosed

(In [13002]) Fixed #11967: use a different technique to get ADMIN_MEDIA_PREFIX in admin JS.

We now store ADMIN_MEDIA_PREFIX on the window object and let JS files read it
from there. The old technique -- looking a <script> tags and trying to deduce
the prefix -- broke with libraries like TineMCE that dynamically add <script>
tags to the <head>.

This is potentially backwards-incompatible for folks who've overridden
admin/base.html; they'll need to add the proper <script> line to
their new admin/base.html. For that reason, this changeset shouldn't
be backported to 1.1.X.

comment:9 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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