Code

Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#11967 closed (fixed)

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

Reported by: danielr 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 danielr 5 years ago.
admin_media.2.diff (2.3 KB) - added by olau 5 years ago.
Bugfixed patch

Download all attachments as: .zip

Change History (11)

Changed 5 years ago by danielr

comment:1 Changed 5 years ago by anonymous

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

comment:2 Changed 5 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 5 years ago by jacob

  • milestone set to 1.2
  • Owner changed from nobody to jacob
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 5 years ago by olau

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 5 years ago by olau

Bugfixed patch

comment:5 Changed 5 years ago by olau

  • Cc olau@… added

comment:6 Changed 5 years ago by justinlilly

  • Cc justin@… added

comment:7 Changed 4 years ago by lukeplant

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 4 years ago by jacob

  • Resolution set to fixed
  • Status changed from assigned to closed

(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 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.