Closed (fixed)
Project:
Drupal core
Version:
6.x-dev
Component:
javascript
Priority:
Critical
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
11 Sep 2007 at 13:08 UTC
Updated:
2 Oct 2007 at 06:14 UTC
Jump to comment: Most recent file
Comments
Comment #1
KentBye commentedHere's some relevant info from the previous thread to upgrade from jQuery 1.1.3.1 to 1.1.4.
SUMMARY: jQuery reports some deprecated functions. Most of the deprecated functions should still work even if they're not changed. jQuery 1.2 is required in order to fully replace the deprecated functions, and so the previous attempts to remove them failed with various errors. We can try to remove them again to see if it works this time to prevent having to do it in the future, or we can leave them the same and see if jQuery 1.2 breaks anything else... which it shouldn't.
More details below...
Dmitrig01 did an audit of Drupal's core javascript code looking for instances of the deprecated selectors, and found that four files use the deprecated XPath Attribute Selector of $("a[@href]") -- book.js, comment.js, node.js & user.js.
In this follow-up, dvessel rolled a patch that removed the @ XPath selectors from all of the Drupal *.js files.
But when 1.1.4 had came out, John Resig said:
So removing the deprecated @ XPath selector failed with jQuery 1.1.4-- as evidenced by the following error reported by Momendo:
At the time, the conclusion was to wait for jQuery 1.2 in order to replace the @ selectors:
CONCLUSION it's probably worth trying to take the @ selectors out again to see if it works this time around
Also dvessel pointed out that the $Date$ and or $Rev$ tags could interfere when committed, and so the $'s were removed from the jquery.js file.
I'll also add a pointer to this thread on the previous jQuery upgrade thread.
Comment #2
jrabeemer commentedjQuery 1.1.4 is in D6 head. Regarding the @ issue, according to the jQuery wiki, it's deprecated but it still works for 1.2. For 1.2, we may need the compat plugin, but I'd rather do without it if possible.
I reported a related JS bug with IE7 which maybe related to the upgrade to jQuery 1.1.4 http://drupal.org/node/172782
Comment #3
KentBye commentedJust wanted to point out that jQuery is now offering two different compressed versions of jQuery 1.2.
There's a 14kb "Minified and Gzipped" version and a 26kb "Packed" version.
The minified & gzipped version looks like the version to go with according to both size and speed, but yet it sounds like you have to have certain hosting capabilities like HTTP compression available in order to use Gzip with JavaScript
The jQuery site points to this blog post with a bit more information:
So the minified and gzipped sounds smaller and faster, but will everyone be able to use it?
Comment #4
dvessel commentedAnd a patch? Who's roll'n? I'm just subscribing for now.
And remember, beta is due soon. Once that hits, this is less likely to get in.
Comment #5
jrabeemer commentedA gzip version is out of the question for now. GZIP has to be enabled on the server. It maybe possible in the future to include the gzip file and maybe switching the two versions with some UI.
Comment #6
chx commentedComment #7
eaton commentedKeeping jQuery at the latest version available before the release of Drupal has long been a valid task. Upgrading to 1.2 is a legitimate 6.0 issue, though it's certainly not a beta-breaker.
Comment #8
Crell commentedSince has jjeff has pointed out, you can't upgrade jquery for a specific site without editing core files I would also favor just moving to 1.2 now. That's better than needing to modify core and apply a BC plugin and make sure it didn't break any edge cases on any site that has to do really fancy JS.
Also note that the gzipped version won't play nice with our own JS compressor, obviously, so you'd end up with 2 JS files at least. Let's just use the minified version and let our compressor do the rest.
Comment #9
jrabeemer commentedThat's interesting. I'd like to see which does better, 1.2 minified or packed - in terms of size and speed of decoding with the D6 JS compressor.
Comment #10
KentBye commentedI just pointed out this thread on IRC channel was a brief discussion with a couple of votes to delay to 7.x, and about 5-6 votes to upgrade.
Here were some of the comments:
* of the two words "API freeze" which one needs explanation?
* I agree with 7.xing that, actually. We're in a code freeze, and they've deprecated functions.
* Actually, the deprecated functions will still work. We've already done an audit on them
* that only covers core. there are also contrib modules that have started upgrading because they believe that Drupal's underlying API won't change because, hey, we're in a code freeze.
* There are jQuery backwards compatibility plug-ins as well. Would that be an option?
* no. jQuery update module
* Considering how much jquery stuff is showing up, I sure hope a way can be found to get hte latest and greatest into core
* I think updating to jquery 1.2 would be worth the API hassle.
* I think it would *really* suck to release a minor version behind.
* I think it would be better to go for 1.2
* 1.1.4 was the "break stuff" version, really
* consideing the a bit unorganizd way that jquery distributes plugins and stuff, it might be hard to get 1.1 stuff shortly.
* It already sucked for D5, which was released within 24 hours of jQuery 1.1.
* Well, they're using project.module now; but like us, they're almost certainly not going to bother continuing to work on 1.1 plugins.
* There *are* backwards compatibility plugins, and I have successfully appended them to jquery.js before
* cat jquery-1.2.js jquery.compat-1.1.js.txt jquery.xpath.js.txt > jquery.js
Comment #11
jrabeemer commentedNo worries. Let's just work through the patch and see what breaks. I have no patch yet. But the results are promising.
I tested the following in FF2 with 1.2 and found no problems or JS bugs.
- expanding grab boxes
- color picker in garland theme
- expanding/collapsing arrows
- expanding user.js radio
- summary splitter in content (not sure what the exact behavior is, it splits, somebody with more experience with this should comment)
- user text field lookup checker
Broken:
- checkall boxes
Haven't tested, don't know where to test... ????
- autoselecting radios
- comment info store by cookie
Testing IE7 next.
Comment #12
eaton commentedThis is definitely a valid issue. Have we consider how our code freeze policy interacts with third-party libraries that we're dependent on?
Comment #13
killes@www.drop.org commented@eaton: No, we haven't. we didn't have this "problem" until we integrated jQuery.
API freeze or not, I'd like to see jQuery 1.2 in Drupal 6.
My single reason is that it was extremly difficult to obtain jQuery 1.0 plugins after the release of jQuery 1.1. I am afraid that this might happen again.
Comment #14
jrabeemer commentedTested in IE7, Safari 3.03, Opera 9.23. Same results as my previous post.
Checkall box is broken amongst all of them. I suspect tableselect.js is the culprit. Somebody with domain knowledge of this JS file should take a look... ::looks at m3avrck, Steven, and kkaefer:: >:-p I have posted a patch just to get the ball rolling with jQuery 1.2. Also, can anyone check the previous areas...
Comment #15
sepeck commentedTo add to killes comments. Long term Drupal 6 support in forums should be simplified and reduced if we are able to release with the current 1.2 released version of JQuery.
However, there is no patch so someone please roll a patch. A list of things to be tested would be beneficial and could be pulled from previous tests and patches.
Comment #16
sepeck commentedwell, there is a patch now. :)
Comment #17
Crell commentedPatches should say patch. :-)
Comment #18
Frando commentedAttached patch fixes the table checkboxes, which is so far the only bug found. Tested in FF2.
Comment #19
webchickI was initially extremely opposed to this, since it's an API change *well* past our code freeze. However, in talking with digitalspaghetti, one of the jQuery core team members, he highly recommended not shipping D6 without this. jQuery is structured a lot like Drupal, in that their release cycles happen quickly, and once a new version comes out, their "contrib" plug-in authors tend to all migrate to that new version. It's almost impossible to find actively-maintained 1.0.x (the version of jQuery D5 shipped with) plug-ins nowadays, same as it's almost impossible to find actively maintained D4.6 plug-ins. Remember that D6 will be "in the wild" for 2+ years; we need to make a future-proof decision regarding what libraries we provide contributed module authors.
Therefore, upgrading this to a beta-breaker. :( If we don't do this before beta, it'll be too late. :(
Comment #20
jrabeemer commentedTested Frando's patch. Tested in FF, IE7, Safari 3.03, Opera 9.23. Check all works now!
Comment #21
profix898 commentedSubscribing. I'm all for shipping Drupal 6 with the latest jQuery release.
Comment #22
kkaefer commentedRemove @ selectors.
Comment #23
kkaefer commentedNow minus Drupal.mousePosition, this is in jQuery (e.pageX/Y) for a long time.
Comment #24
KentBye commentedThanks webchick for the beta-breaker upgrade and others for rolling patches.
I just wanted to clarify something that Crell said here: "Also note that the gzipped version won't play nice with our own JS compressor... Let's just use the minified version and let our compressor do the rest."
The 14kb minified/gzipped version is actually the same thing, and so I think you meant the 26 kb "Packed" jquery version, which is what is included in the latest patches and is the compression type in D5
Just wanted to clarify that for anyone following along.
Comment #25
kkaefer commentedAnd some other features jQuery already includes are being removed from drupal.js.
Comment #26
webchickBtw, for those testing this patch, there is a list of things to test under the "Javascript" section of http://groups.drupal.org/node/5974 (thanks, frando!)
Please report back with:
- Browser(s) and versions
- Operating system(s) and versions
- Which of the tests you were able to complete
- What the status was of your testing (pass/fail on each test)
We can probably RTBC this once we've been assured that each of these major functionality points continues to work under 1.2 in all of the major browsers on all of the major operating systems. And a couple of the odd-ball ones too.
Comment #27
kkaefer commentedLast cruft-removing patch. This is ready for testing.
Comment #28
kkaefer commentedFurther explanation of my changes: jQuery nowadays ships with a lot of the stuff that was custom-built in
drupal.js. This patch removes all this cruft (duplicate functionality) in favor of the jQuery functions.Drupal.extend: this function has been removed in favor ofjQuery.extend()Drupal.absolutePosition(): this functionality is now in jQuery 1.2 in$(elem).offset()Drupal.dimensions(): these values can be gathered by calling$(elem).width()or$(elem).height()Drupal.mousePosition: John Resig told me thate.pageYresp.e.pageXcontain the value (eis the first parameter of an event callback)Drupal.parseJson: You basically only need this function when doing an XmlHttpRequest. jQuery supports all kinds of functionality around JSON.Additionally, I checked all Drupal JS files with JSLint and removed all warnings (missing semicolons or invalid regexps).
Comment #29
bdragon commentedGeneral note: If need be, the removed functions could be put back in as wrappers around jQuery functionality, if the removal of these ends up blocking this getting in!
Comment #30
kkaefer commentedOr just take the patch from #22.
Comment #31
KentBye commentedMy vote is to focus on the conservative update patch in #22 which updates jQuery from 1.1.4 to 1.2 and removes the deprecated @ selectors from the core *.js files.
I think the removal of the Drupal core javascript cruft should be postponed to 7.x, especially since it breaks the API for contributed modules that may be using these functions -- like Drupal.dimensions & Drupal.absolutePosition. And according to kkaefer on IRC don't have a compelling performance improvement for taking them out this late in the game.
So my vote is to keep the patch simple in order to get this in for the beta, which Gabor says is slated to be out by the end of this week.
Comment #32
kkaefer commentedI grepped HEAD for the use of these functions:
Drupal.absolutePosition: jsnippets, jstools, jstools, jstools, mysite, slideshowDrupal.dimensions: nodestackDrupal.mousePosition: contemplate, jstools, nodestackDrupal.parseJson: activeselect, communitytags, fasttoggle, findpost, jstools, jstools, jstools,jstools, jstools, karma, nodequeue, projectissue
Comment #33
tanepiper commentedI've had a look through the patch code, and haven't noticed anything wrong in there. I've also tested a fresh install with patch 3 and all modules installed with FF2 in Windows & Ubuntu, Opera 8 in Ubuntu & 9.5 Windows and Safari in Windows, I can't detect any errors with patch 3 so I give it my seal of approval.
Comment #34
kkaefer commentedThe
DRUPAL-6--1branch does not contain any uses of these functions.Comment #35
hass commentedEvery maintainer needs to update a module from D5 to D6, while there are many changes. So it shouldn't be a problem to replace outdated Drupal functions with the newer ones... D5 modules are "broken" in general in D6... so - no harm if we remove outdated code and reduce JS file size :-).
Comment #36
kkaefer commentedI manually checked each module in the
HEADlist of modules using the functions to be removed: No upgraded module uses this function. We’re safe.Comment #37
kkaefer commentedBTW, once this patch is in, I'll write update instructions for the removed functions.
Comment #38
eaton commentedI agree wholeheartedly. Let's stay as compatible as we possibly can without anchoring ourselves to an already-obsolete library for 1-2 years.
Comment #39
jrabeemer commentedIf we don't remove these functions, what's the incentive for module writers to make the switch and Do The Right Thing (tm)? More importantly, since the move to 1.2, almost all modules that make use of Drupal.js and jquery will most certainly need to make JS changes anyway. Some will probably request we add jquery 1.1 compatibility library. This transition will take months for module writers. Meanwhile D6 core needs to be up to date and clean of cruft. I don't think this is a big deal. As long as the core JS testing checks, we're good. Module writers will need to upgrade. Such is progress.
Comment #40
KentBye commentedThe list of affected modules that kkaefer listed does seem fairly small & manageable, and he did verify that no one has already made the upgrade to Drupal 6 yet -- so no worries of causing any one to duplicate their work.
And I think that encouraging module writers to "make the switch and Do The Right Thing (tm)" is also relevant -- a lot of people look at Drupal's core code as a standard to mimic, and so it'll be easier to eliminate these functions now than later. And the D5 -> D6 update documentation will certainly help make it less painful.
So I'm more on the fence about which one to go with, but like Eaton, I just want to make sure that the upgrade to 1.2 gets into the beta.
Comment #41
kkaefer commentedThis patch fixes file uploads in several browsers. They were broken for a very long time (ahah.js patch) without anyone noticing it...
Comment #42
chx commentedDo it or do not, there is no try. My vote is on the latest patch.
Comment #43
kkaefer commentedBTW, the removal of upload.js in the latest patch is on purpose: it's no longer used.
Comment #44
John Resig commentedI'm the creator of jQuery, I was asked to weigh in with my thoughts.
Synchronization of jQuery 1.2 and Drupal 6 would be a huge win for both groups. I think, based upon the results of the recent Drupal 5/jQuery 1.0 mismatch we can safely say that the amount of overhead required to maintain an off-version relationship is very high.
Here's some points to consider:
- Generally speaking, bug fix releases are only done for the current branch. We can sometimes make an exception for old branches, but it's much more difficult for us.
- Plugin authors optimize and port to the most recent version of jQuery. Very rarely do authors try to support multiple branches.
- The new jQuery UI (the successor to Interface) will only work on jQuery 1.2.
So yeah, we'd definitely like it if it were possible to bring Drupal up to jQuery 1.2. If there's any technical reasons at play, please let me know so that we can help, wherever possible.
Comment #45
Crell commentedTesting #41 on fresh HEAD using Konqueror 3.5.6 and Firefox 2.0.0.6 on Kubuntu Feisty (I figure that's the only browser that's not gotten attention yet...):
- Install: OK
- Password checker during install: OK
- Collapsable fieldsets: If the fieldset toggling does not force the page to scroll, it's faster and smoother than it was before. If it does it's a bit jerky, but then it always was. Overall it seems smoother in Konqueror than in Firefox.
- Fixed-position table headers: Work, although on complex pages (modules page) is a bit jerky. Is fine in Firefox.
- Autocomplete of username: No autocomplete is event attempted. In Firefox, it attempts but fails. Firebug reports: "Drupal.parseJson is not a function".
- Teaser splitter: The new textfield is added, but the text is NOT transferred from the body to the teaser. It is removed from the body, however, so joining again means data loss. Bad. :-) Firefox is fine.
- Book outline selectors: OK
- File attachments: Works in Firefox. In Konqueror, it shows the uploading animation and never completes. I believe that has been the case for as long as I remember. :-) Probably not a show-stopper for this issue.
- Checkbox tables: OK
- color.module: As cool as ever. :-)
So overall, outstanding issues:
- Teaser splitter breaks badly in Konqueror.
- Textfield autocomplete doesn't work at all in Konqueror, and breaks with JS error in Firefox.
Setting to CNW.
Comment #46
chx commentedDespite I am a Kubuntu user (konqueror is available) I would say do not bother much with fixing those bugs. I am not consider Konqueror bugs to be beta breakers -- if we work with Konqueror, that's good, if we don't, well, who cares? Textfield autocomplete breaks with JS error in Firefox <= that's a problem.
Comment #47
chx commentedComment #48
fgmChecked autocomplete and node edit splitter.
Both work on FF2.0.0.6 on Linux and Win32.
Comment #49
kevin francis commentedTesting with fresh Drupal HEAD checkout it works fine. No warnings or errors detected.
Comment #50
kevin francis commentedReverting patch to RTBC (posted over fgm's RTBC)
Comment #51
fgmFollowing chx's suggestion, I have to add testing the same (autocomplete + splitter) under Opera 9.23 Win32 works too.
Comment #52
kkaefer commentedWe do not have any reports from IE. The attached patch fixes the autocomplete bug more cleanly.
Comment #53
Frando commentedTesting the latest patch in IE6 (and at the same time, working with the javascript testing list I created yesterday at http://groups.drupal.org/node/5974):
PASS: Check if collapsing/expanding fieldsets work.
PASS: node/add/story Check if the autocomplete form fields (e.g. "Authored by") work.
PASS: node/add/story Check if the teaser splitter works.
PASS: node/add/story With book.module enabled: Check if the parent selector works ("Book outline").
PASS: node/add/story With upload.module enabled: Test attaching a new file.
PASS: Check if logging in with OpenID works.
PASS: admin/content/node Check if a row gets highlighted when selected. Check if you can select multiple rows with shift+click.Check if you can select all rows by clicking at the checkbox in the table header.
FAIL: admin/content/node Create a few nodes so that the table is longer than your browser viewport. Then, srcoll down. The table header should keep floating at the top of the viewport.
PASS: admin/build/themes/settings/garland Check if the color picker works.
admin/user/user/create Check if the password strength indicator and password match indicator works.
So, everything works with the patch in IE6 apart from the floating table headers. These are broken without the patch, too, though it is an unrelated bug that has nothing to do with the jQuery update and should therefore be fixed (if possible) in another issue.
This one here is RTBC.
Comment #54
webchickIE 7?
Comment #55
gábor hojtsyA very recent IE6 JS fix broken the tableheader.js hunk:
patching file includes/common.inc
patching file misc/ahah.js
patching file misc/autocomplete.js
patching file misc/collapse.js
patching file misc/drupal.js
patching file misc/jquery.js
patching file misc/progress.js
patching file misc/tableheader.js
Hunk #2 FAILED at 91.
1 out of 2 hunks FAILED -- saving rejects to file misc/tableheader.js.rej
patching file misc/tableselect.js
patching file misc/textarea.js
patching file misc/upload.js
patching file modules/book/book.js
patching file modules/color/color.js
patching file modules/comment/comment.js
patching file modules/node/node.js
patching file modules/system/system.js
patching file modules/upload/upload.module
patching file modules/user/user.js
Make sure to fix that.
Comment #56
catchMarking CNW for that hunk.
Comment #57
kkaefer commentedSorry for posting yet another patch, but sticky table headers are broken in IE 7 with the new patch (see http://dev.jquery.com/ticket/1599). I modified the script to check for negative widths and pass 0 instead. This fixes the bug in IE7.
Additionally, the radio button selector on node filter pages did no longer work. I converted it to a behavior, generalized it (it now works for the user filter as well) and moved it to
misc/form.js.I tested this patch with IE 6 and 7 and couldn’t find any non-working scripts. We disabled the sticky tableheaders for IE6 on purpose since noone is willing to fix the script for it.
Comment #58
tanepiper commentedOk, I've just tested patch 6 on FF2 under Ubuntu 7.04, works fine.
Comment #59
dvessel commentedIn Safari 2.0.4:
PASS: Check if collapsing/expanding fieldsets work.
PASS: node/add/story Check if the autocomplete form fields (e.g. "Authored by") work.
PASS: node/add/story Check if the teaser splitter works.
PASS: node/add/story With book.module enabled: Check if the parent selector works ("Book outline").
PASS: node/add/story With upload.module enabled: Test attaching a new file.
- This was actually broken before. Click attach and the animated bar would go on forever.
PASS: Check if logging in with OpenID works.
PASS: admin/content/node Check if a row gets highlighted when selected. Check if you can select multiple rows with shift+click.Check if you can select all rows by clicking at the checkbox in the table header.
PASS: admin/content/node Create a few nodes so that the table is longer than your browser viewport. Then, srcoll down. The table header should keep floating at the top of the viewport.
PASS: admin/build/themes/settings/garland Check if the color picker works.
admin/user/user/create Check if the password strength indicator and password match indicator works.
PASS: admin/settings/clean-urls See if it detects clean urls.
---
Not related to this patch but I just wanted to bring this up. The checkbox to select all table rows from the table header doesn't work when it's floating.
But this is looking really good.
Comment #60
gábor hojtsySounds good. Thanks for the quick reaction and the awesome team job in this issue in the past 30 hours. Committed the latest patch. I hope we have a stable jQuery version now in core :)
Comment #61
kkaefer commentedAdded upgrade information.
Comment #62
forngren commentedhttp://jquery.com/blog/2007/09/16/jquery-121-quick-fixes-for-12/
Comment #63
sepeck commentedsearch please.
Comment #64
sepeck commentedsearch please.
http://drupal.org/node/176222
Comment #65
(not verified) commented