Ticket #12583 (closed Bug: wontfix)

Opened 2 years ago

Last modified 6 months ago

plugin.js from KSS breaks jQuery in IE

Reported by: marco.mariani Owned by: ree
Priority: minor Milestone: 4.x
Component: KSS (Ajax) Version: 4.2
Keywords: Cc:

Description

What happens - including kukit in a page breaks subsequent calls to jQuery if they rely on window.$.

See  http://plone.293351.n2.nabble.com/kukit-parts-load-order-vs-jquery-and-IE8-tt6501790.html

Inside plugin.js, there is a snippet of code that should never be executed unless Scriptaculous effects are in use:

if (kukit.HASEFFECTS && typeof(Effect.Transitions) != "undefined") {
   ...
}

Unfortunately, the if block contains the following:

    // This is terrible. We needed to copy this part
    // from prototype. Notice that I put this.$ =
    // in the beginning. Without that the function
    // declarations in IE won't overwrite each others,
    // and one of them (first or last occurence) comes
    // in. Now, we have a contradicting $ declaration
    // in Mochikit, causing the problem.

    this.$ = function $() {
      var results = [], element;
      for (var i = 0; i < arguments.length; i++) {
        element = arguments[i];
        if (typeof element == 'string')
          element = document.getElementById(element);
        results.push(Element.extend(element));
      }
      return results.length < 2 ? results[0] : results;
    };

that, if executed, will happily overwrite the jQuery binding to the global $, which is sometimes relied upon despite best practices.

This is fine on several browser, because the "if" condition is false, but IE treats named function expressions as if they were statements (see  http://kangax.github.com/nfe/). Because of that, the window.$ is overwritten not because of the left part of the assigment (this.$ =) but because of the right part (the function is not anonymous).

The fix is trivial and should not affect anything:

--- a/plugins/effects/browser/plugin.js	Thu Feb 23 10:30:24 2012 +0100
+++ b/plugins/effects/browser/plugin.js	Thu Feb 23 10:45:20 2012 +0100
@@ -34,7 +34,7 @@
     // in. Now, we have a contradicting $ declaration
     // in Mochikit, causing the problem.
 
-    this.$ = function $() {
+    this.$ = function () {
       var results = [], element;
       for (var i = 0; i < arguments.length; i++) {
         element = arguments[i];

Change History

comment:1 follow-up: ↓ 2 Changed 2 years ago by eleddy

  • severity changed from Untriaged to Normal
  • Milestone set to 4.2

any chance you have commit privs and can push this fix?

comment:2 in reply to: ↑ 1 ; follow-up: ↓ 3 Changed 2 years ago by marco.mariani

Replying to eleddy:

any chance you have commit privs and can push this fix?

No, I cannot.

comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 2 years ago by eleddy

Replying to marco.mariani:

Replying to eleddy:

any chance you have commit privs and can push this fix?

No, I cannot.

Do you want them?

comment:4 in reply to: ↑ 3 Changed 2 years ago by marco.mariani

No, I cannot.

Do you want them?

sure, thanks :)

comment:5 Changed 2 years ago by eleddy

sweet! You have to sign this agreement then I can get things hooked up.  http://plone.org/foundation/contributors-agreement/agreement.pdf/view You can scan and send that to elizabeth.leddy@… and I'll make sure to get you perms. I need to wait until some Americans wake up I think... :)

comment:6 Changed 2 years ago by marco.mariani

Actually, I see that kss.core is in the collective SVN repo, where I have write access. Committed for 1.6.4 in rev. 248599.

Thanks again.

comment:7 Changed 2 years ago by eleddy

  • Keywords unsupported added
  • Status changed from new to closed
  • Resolution set to wontfix

comment:8 Changed 2 years ago by smcmahon

  • Priority changed from minor to major
  • Keywords unsupported removed
  • Version changed from 4.1 to 4.2
  • Resolution wontfix deleted
  • Status changed from closed to reopened

This is actually quite nasty. It has the potential to break anything that loads after kukit.js on IE8, which we still support. That in particular includes jquery ui, which is loaded at the bottom of the js registry.

To make things a little worse: this is fixed in kss.core 1.6.4, but 1.6.4 includes several changes which don't work on 4.2. There is a branch, 1.6.3FTA, which is 1.6.3 plus this patch.

comment:9 Changed 2 years ago by kleist

  • Status changed from reopened to confirmed
  • Milestone changed from 4.2 to 4.x

comment:10 Changed 23 months ago by smcmahon

  • Priority changed from major to minor

The problem does not affect jquery ui, so it's minor rather than major.

comment:11 Changed 22 months ago by kleist

see also #13154

comment:12 Changed 6 months ago by davisagli

  • Status changed from confirmed to closed
  • Resolution set to wontfix

KSS is no longer included in Plone 4.3.

Note: See TracTickets for help on using tickets.