Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#22975 closed enhancement (fixed)

Remove deprecated jQuery methods from core to be safe for jQuery 1.9

Reported by: ocean90's profile ocean90 Owned by:
Milestone: 3.6 Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: has-patch
Focuses: Cc:

Description

Today jQuery 1.9 Beta was released.

jQuery 1.9 has removed many of the items we deprecated during the last few versions of jQuery.
To test, we recommend that you start with the jQuery Migrate plugin since it will warn you about any deprecated features the code may depend on. Just include these two script tags in your code, replacing your existing jQuery script include:

<script src="http://code.jquery.com/jquery-1.9.0b1.js"></script>

<script src="http://code.jquery.com/jquery-migrate-1.0.0b1.js"></script>

The attached patch adds both scripts to core so that you can test it. There are already some notices which we should try to reduce.

Attachments (21)

jquery-1.9-migrate.patch (374.0 KB) - added by ocean90 11 years ago.
22975-apply-fix.patch (813 bytes) - added by ocean90 11 years ago.
22975.custom-fields.patch (745 bytes) - added by SergeyBiryukov 11 years ago.
22975.custom-fields.diff (738 bytes) - added by cdog 11 years ago.
22975.custom-fields.2.diff (376 bytes) - added by cdog 11 years ago.
22975.custom-fields.3.diff (383 bytes) - added by SergeyBiryukov 11 years ago.
With proper formatting
22975.rc1.patch (42.5 KB) - added by ocean90 11 years ago.
22975.patch (7.5 KB) - added by ocean90 11 years ago.
jquery.1.9.1.diff (41.5 KB) - added by wonderboymusic 11 years ago.
22975.replace-live.patch (2.1 KB) - added by ocean90 11 years ago.
22975.thickbox.patch (992 bytes) - added by ocean90 11 years ago.
22975.browser.patch (1.9 KB) - added by ocean90 11 years ago.
22975.consolidated.patch (5.3 KB) - added by ocean90 11 years ago.
jquery.migrate.1.1.1.patch (5.0 KB) - added by ocean90 11 years ago.
22975.taxonomy-checklist.patch (723 bytes) - added by SergeyBiryukov 11 years ago.
22975.taxonomy-checklist.2.patch (743 bytes) - added by SergeyBiryukov 11 years ago.
22975.taxonomy-checklist.3.patch (707 bytes) - added by SergeyBiryukov 11 years ago.
22975.taxonomy-checklist.3b.patch (1.0 KB) - added by ocean90 11 years ago.
22975.migrate.1.2.1.patch (3.7 KB) - added by ocean90 11 years ago.
22975.diff (691 bytes) - added by aaroncampbell 11 years ago.
22975.suggest.patch (1.2 KB) - added by ocean90 11 years ago.

Download all attachments as: .zip

Change History (92)

#1 @scribu
11 years ago

  • Milestone changed from Future Release to 3.6

#2 @nacin
11 years ago

  • Keywords has-patch commit added

#3 @nacin
11 years ago

In 23180:

Update to jQuery 1.9 Beta 1 in trunk (3.6-alpha).

Temporarily includes jQuery in original, unminified form, for ease of testing and bug reporting.

jQuery 1.9 introduces a "migrate" script for deprecated and removed behavior. Warnings are issued for methods we should not be using, and should be fixed in core. These warnings are sent to the JavaScript console and are collected in jQuery.migrateWarnings.

See http://blog.jquery.com/2012/12/17/jquery-1-9-beta-1-released/ for more.

props ocean90, see #22975.

#4 @nacin
11 years ago

  • Keywords needs-patch added; has-patch commit removed

Setting to needs-patch for removal of warnings.

Note that I needed to make a change to jQuery Migrate before landing it. PR here: https://github.com/jquery/jquery-migrate/pull/5.

#5 @nacin
11 years ago

Seems like the dependency tree in [23180] would break with concatenation. Migrate needs to load after jQuery, not before.

We should just place the migration code directly into jQuery core for now. If we need to keep it for plugins (which is likely the case), we can figure out a better way.

#6 follow-ups: @nacin
11 years ago

Anyone else imagining this as a good idea?

$scripts->add( 'jquery', false, array( 'jquery-core', 'jquery-noconflict', 'jquery-migrate' ) );
$scripts->add( 'jquery-core', ... );
$scripts->add( 'jquery-noconflict', ... );
$scripts->add( 'jquery-migrate', ... );

#7 follow-up: @trepmal
11 years ago

Getting a Javascript error using Enter New on custom fields.
http://content.screencast.com/users/trepmal/folders/Jing/media/2e3dc6ea-4680-442f-8e71-b414da1f184d/00000690.png

In screenshot, first 2 migrate notices are on page load, 3rd notice and error when clicking Enter New

#8 in reply to: ↑ 7 ; follow-up: @ocean90
11 years ago

Replying to trepmal:

Getting a Javascript error using Enter New on custom fields.

That's already fixed upstream, see https://github.com/jquery/jquery-migrate/issues/4

#9 @cklosows
11 years ago

  • Cc cklosowski@… added

Also Seeing this when trying to use .toggle() for a show/hide within a plugin page. Was able to duplicate @trepmal's case as well.

TypeError: Cannot call method 'apply' of undefined http://localhost/wordpress/wp-includes/js/jquery/jquery-migrate.js?ver=1.0.0b1:349

#10 in reply to: ↑ 8 @cklosows
11 years ago

Replying to ocean90:

Replying to trepmal:

Getting a Javascript error using Enter New on custom fields.

That's already fixed upstream, see https://github.com/jquery/jquery-migrate/issues/4

Confirmed that fixed it. Thanks @ocean90

#11 follow-up: @trepmal
11 years ago

22975-apply-fix.patch fixes the Enter New problem.

"Add Custom Field" (and Update for existing custom fields) button results in javascript error.
http://content.screencast.com/users/trepmal/folders/Jing/media/7d2aae71-6083-42ae-99c9-dc18c5534049/00000693.png
However, for new custom fields, the field is created and is then duplicated after updating/publishing/saving draft.

#12 @nacin
11 years ago

In 23182:

Update jQuery Migrate plugin to latest, fixing .toggle(boolean) usage. see #22975.

#13 @nacin
11 years ago

Here's the latest jQuery Migrate (live) — http://code.jquery.com/jquery-migrate-git.js. That's what [23182] applies. I didn't even see the chatter on this ticket committing, as one of the lead jQuery developers alerted me. He's very happy to see that we got some testers. :-)

#14 in reply to: ↑ 6 @TobiasBg
11 years ago

Replying to nacin:

Seems like the dependency tree in [23180] would break with concatenation. Migrate needs to load after jQuery, not before.

I can't reproduce problems with this. http://localhost/wp-admin/load-scripts.php?c=0&load[]=jquery-migrate,jquery,utils&ver=3.6-alpha works fine, jQuery is first (with .noConflict(); at the end), then jquery-migrate.js.

Replying to nacin:

$scripts->add( 'jquery-noconflict', ... );

This should also get a dependency on jquery-core. This would allow plugins to enqueue jQuery without jquery-migrate, if it is sure that it doesn't need the backwards compatibility part.

#15 follow-up: @nacin
11 years ago

In 23183:

Have the 'jquery' script handle be a parent of both jQuery core and jQuery Migrate. see #22975.

#16 in reply to: ↑ 15 @markmcwilliams
11 years ago

Replying to nacin:

In 23183:

Have the 'jquery' script handle be a parent of both jQuery core and jQuery Migrate. see #22975.

Should the version not be updated as per [23182] or will it not matter at this early stage?

Last edited 11 years ago by markmcwilliams (previous) (diff)

#17 in reply to: ↑ 11 ; follow-ups: @SergeyBiryukov
11 years ago

Replying to trepmal:

"Add Custom Field" (and Update for existing custom fields) button results in javascript error.

Confirmed. The error happens at the very beginning of wpList.add():
http://core.trac.wordpress.org/browser/tags/3.5/wp-includes/js/wp-lists.js#L310

Seems that jQuery 1.9 has a problem if the string starts with \n. Also reproduced with this fiddle: http://jsfiddle.net/VnCrU/.

22975.custom-fields.patch fixes the error.

#18 in reply to: ↑ 17 @cdog
11 years ago

Replying to SergeyBiryukov:

22975.custom-fields.patch fixes the error.

It fixes the error but throws the following warning:

JQMIGRATE: $(html) HTML strings must start with '<' character

Removing both leading characters \n\t as shown in attachment:22975.custom-fields.diff seems to get rid of the warning too.

Edit: Is there any reason for using those special characters at the beginig of each concatenated string? Removing them would produce the same results making the code more readable.

Last edited 11 years ago by cdog (previous) (diff)

#19 @cdog
11 years ago

  • Cc catalin.dogaru@… added

#20 in reply to: ↑ 17 ; follow-up: @SergeyBiryukov
11 years ago

Replying to SergeyBiryukov:

Seems that jQuery 1.9 has a problem if the string starts with \n. Also reproduced with this fiddle: http://jsfiddle.net/VnCrU/.

Appears to be a result of the fix in http://bugs.jquery.com/ticket/11290.

#21 @cdog
11 years ago

Please take a look at: attachment:22975.custom-fields.2.diff. I think a better approach would be to trim leading spaces as noted in http://bugs.jquery.com/ticket/11290#comment:4. Should also solve similar issues if they exist.

@SergeyBiryukov
11 years ago

With proper formatting

#22 @nacin
11 years ago

A developer using wp_localize_script( 'jquery', ... ) (to inexplicably pass values to, well, I don't know, they're doing it wrong) will find that their code breaks now.

@ocean90
11 years ago

#23 @ocean90
11 years ago

Attached a patch for jQuery 1.9 RC1. Includes also the updated version of the Migrate script. I used the git Version since it includes already a new fix.

#24 @ocean90
11 years ago

#23204 was marked as a duplicate.

#26 @nacin
11 years ago

In 23301:

jQuery 1.9 final. jQuery Migrate 1.0. Uncompressed for now, while we iron out kinks.

props jorbin. see #22975.

#27 @Lwangaman
11 years ago

  • Cc donjohn.fmmi@… added

jQuery 1.9 has specifically deprecated the methods ".live()" and ".browser()" in favour of ".on()" and ".support()" respectively.
This breaks "wp-includes/js/admin-bar.js" (or "wp-includes/js/admin-bar.min.js") which makes use of the "$.browser()" method to target Mozilla. Hovering over admin bar items when using jQuery 1.9 results in these errors in the console:
Uncaught TypeError: Cannot read property 'mozilla' of undefined ---- admin-bar.min.js:1
It will be necessary to target the specific functionality of mozilla that is different from other browsers in order to use "$.support()".

#28 @ocean90
11 years ago

  • Keywords has-patch added; needs-patch removed

@ocean90
11 years ago

#29 @wonderboymusic
11 years ago

#23388 was marked as a duplicate.

#30 @SergeyBiryukov
11 years ago

jquery.1.9.1.diff fixes the error when adding or updating custom fields (comment:17).

#31 @nacin
11 years ago

In 23421:

jQuery 1.9.1 and jQuery Migrate 1.1.0.

Remains uncompressed for now, until we work out all 1.9.x issues.
Fixes custom fields.

props ocean90, wonderboymusic. see #22975.

#32 @ocean90
11 years ago

  • Keywords needs-patch added; has-patch removed

I will start to replace the deprecated methods now.

#33 @ocean90
11 years ago

I have started with the .live() event.

$ ack ".live\(" -l --ignore-dir=wp-content
wp-admin/js/categories.js
wp-admin/js/inline-edit-post.js
wp-admin/js/inline-edit-tax.js
wp-admin/js/post.js
wp-admin/js/tags.js
wp-includes/js/jquery/jquery-migrate.js
wp-includes/js/swfupload/handlers.js
wp-includes/js/thickbox/thickbox.js

22975.replace-live.patch adresses

  • wp-admin/js/inline-edit-post.js
  • wp-admin/js/inline-edit-tax.js
  • wp-admin/js/tags.js

I ignored external libs like Thickbox. wp-admin/js/categories.js isn't used anymore, see #23486.

wp-admin/js/post.js: I have no idea what these lines should do, it seems that it does nothing.

Version 0, edited 11 years ago by ocean90 (next)

#34 @ocean90
11 years ago

Next: $.browser

ack "(\\$|jQuery)\.browser" --ignore-dir=wp-content -l
wp-admin/js/color-picker.js
wp-includes/js/admin-bar.js
wp-includes/js/autosave.js
wp-includes/js/imgareaselect/jquery.imgareaselect.js
wp-includes/js/jquery/jquery-migrate.js
wp-includes/js/jquery/jquery.hotkeys.js
wp-includes/js/jquery/suggest.js
wp-includes/js/thickbox/thickbox.js

wp-admin/js/color-picker.js is handled in #23484.

wp-includes/js/admin-bar.js uses $.browser to detect WebKit to fix the focus bug. See [22249].

wp-includes/js/autosave.js uses $.browser to detect Safari. See #18341.

#35 @nacin
11 years ago

In 23444:

Stop using jQuery.live(). props ocean90. see #22975.

#36 follow-up: @nacin
11 years ago

Let's also patch Thickbox. It is dead upstream.

#37 in reply to: ↑ 36 @ocean90
11 years ago

  • Keywords has-patch added; needs-patch removed

Replying to nacin:

Let's also patch Thickbox. It is dead upstream.

.live() is done in 22975.thickbox.patch.

In 22975.browser.patch I have replaced the jQuery.browser calls with navigator.userAgent. Except in Thickbox - It would work without the two lines, but I leave it in, since it seems not to hurt.

#38 @ocean90
11 years ago

There is another jQuery.browser call in admin-bar.js. It's in the hoverintent lib. Added in [18488]. Looking at the original version there seems not to be such a call. Will talk to azaozz.

#39 @ocean90
11 years ago

22975.consolidated.patch combines 22975.thickbox.patch and 22975.browser.patch​. And updates hoverIntent to r6, see [19605].

#41 follow-up: @SergeyBiryukov
11 years ago

In [23444], the selector in post.js appears to be redundant: 22975.taxonomy-checklist.patch.

#42 in reply to: ↑ 41 ; follow-ups: @ocean90
11 years ago

Replying to SergeyBiryukov:

In [23444], the selector in post.js appears to be redundant: 22975.taxonomy-checklist.patch.

Yeah, just saw that I have added the wrong patch which has included the console.log() call.

One small disadvantage is, that the event is now fired for each checkbox, not only the popular ones. We could add some parent element checks, but IMO the current way is not such harmful.

#43 follow-up: @azaozz
11 years ago

...but IMO the current way is not such harmful.

As far as I remember that bit of JS was causing slow running problems in (older) IE when there are a lot of categories. Probably better to keep it on the "popular" checkboxes only.

#44 in reply to: ↑ 43 ; follow-up: @ocean90
11 years ago

Replying to azaozz:

As far as I remember that bit of JS was causing slow running problems in (older) IE when there are a lot of categories. Probably better to keep it on the "popular" checkboxes only.

The performance one was r21737, see comment:ticket:21106:16. It had used previously :checkbox, which was replaced with [type="checkbox"]. So the change should be okay - Sergey, can you confirm?

#45 in reply to: ↑ 44 @azaozz
11 years ago

Replying to ocean90:

You're right. It also doesn't need the two selectors as we are looking at all checkboxes in the two <ul>:

.on( 'click', 'input[type="checkbox"]', function() {...

would be better (that JS and postbox really needs redoing).

#46 in reply to: ↑ 42 @SergeyBiryukov
11 years ago

Replying to ocean90:

One small disadvantage is, that the event is now fired for each checkbox, not only the popular ones.

Would 22975.taxonomy-checklist.2.patch make more sense then?

The performance one was r21737, see comment:ticket:21106:16. It had used previously :checkbox, which was replaced with [type="checkbox"]. So the change should be okay - Sergey, can you confirm?

Correct.

#47 @ocean90
11 years ago

  • Keywords commit added

22975.taxonomy-checklist.patch won't work, because li.popular-category can be added after the DOM is ready.

Commit candidates:

#48 @nacin
11 years ago

In 23516:

jQuery Migrate 1.1.1. props ocean90. see #22975.

#49 @nacin
11 years ago

In 23517:

Imporove a selector targeting taxonomy meta boxes. props SergeyBiryukoc. see #22975.

#50 @nacin
11 years ago

In 23518:

Stop using deprecated jQuery API (.browser and .live). Do manual UA sniffing where still necessary. Improve selector performance by using delegated events. props ocean90. see #22975.

#51 @nacin
11 years ago

  • Keywords has-patch commit removed

#52 @nacin
11 years ago

In 23536:

Imporove a selector targeting taxonomy meta boxes. props SergeyBiryukov. see #22975.

Uses correct patch, reverting [23517].

#53 follow-up: @SergeyBiryukov
11 years ago

Not sure why [23517] got reverted. Per comment:47, 22975.taxonomy-checklist.2.patch is the correct patch.

#54 in reply to: ↑ 53 @nacin
11 years ago

Replying to SergeyBiryukov:

Not sure why [23517] got reverted. Per comment:47, 22975.taxonomy-checklist.2.patch is the correct patch.

ocean90 had sent me a note — he mixed up the two patches in that comment. .2 wouldn't work. The first one does. Hence the swap.

#55 in reply to: ↑ 42 ; follow-up: @SergeyBiryukov
11 years ago

Replying to ocean90:

One small disadvantage is, that the event is now fired for each checkbox, not only the popular ones.

Seems like 22975.taxonomy-checklist.3.patch would be the correct optimization here. We only want the checkbox synchronization between "All Categories" and "Most Used" tabs to fire when clicking on a popular category itself, not on its child categories (like before [23444]) or any category (like now).

#56 in reply to: ↑ 20 @SergeyBiryukov
11 years ago

Replying to SergeyBiryukov:

Seems that jQuery 1.9 has a problem if the string starts with \n. Also reproduced with this fiddle: http://jsfiddle.net/VnCrU/.

Appears to be a result of the fix in http://bugs.jquery.com/ticket/11290.

Custom fields (comment:11) and menus (#23681) are affected.

In current trunk it's a console message rather than a JS error thanks to jQuery Migrate 1.1.0 ([23421]).

#57 @ocean90
11 years ago

  • Keywords needs-patch added

#58 @aniketpant
11 years ago

  • Cc me@… added

#59 in reply to: ↑ 55 @ocean90
11 years ago

Replying to SergeyBiryukov:

Seems like 22975.taxonomy-checklist.3.patch would be the correct optimization here. We only want the checkbox synchronization between "All Categories" and "Most Used" tabs to fire when clicking on a popular category itself, not on its child categories (like before [23444]) or any category (like now).

Committed in [24116].

#60 in reply to: ↑ 6 ; follow-up: @anastis
11 years ago

Replying to nacin:

Anyone else imagining this as a good idea?

$scripts->add( 'jquery', false, array( 'jquery-core', 'jquery-noconflict', 'jquery-migrate' ) );
$scripts->add( 'jquery-core', ... );
$scripts->add( 'jquery-noconflict', ... );
$scripts->add( 'jquery-migrate', ... );

This seems to have make it into beta 2 in the form:

$scripts->add( 'jquery', false, array( 'jquery-core', 'jquery-migrate' ) );
$scripts->add( 'jquery-core', '/wp-includes/js/jquery/jquery.js', array(), '1.9.1' );
$scripts->add( 'jquery-migrate', '/wp-includes/js/jquery/jquery-migrate.js', array(), '1.1.1' );

Is the jquery-migrate plugin going to be removed in 3.6 or in some future version?

Also, I've seen some plugins (notably WooCommerce) checking for the jQuery version like this:

if ( isset( $wp_scripts->registered['jquery']->ver ) && $wp_scripts->registered['jquery']->ver < '1.7' )

Perhaps it would be a good idea to add a version number to 'jquery' so that this kind of code remains functional?

#61 @ocean90
11 years ago

In 24252:

jQuery Migrate 1.2.1. see #22975.

@aaroncampbell
11 years ago

#62 in reply to: ↑ 60 @aaroncampbell
11 years ago

22975.diff adds the jquery-core version to jquery. This will fix the issue brought up in comment 60

Last edited 11 years ago by aaroncampbell (previous) (diff)

#63 @ryan
11 years ago

Looks good.

#64 @aaroncampbell
11 years ago

  • Keywords commit has-patch added; needs-patch removed

#65 @markjaquith
11 years ago

In 24314:

Add the jQuery version to the 'jquery' script alias

  • Allows people to check it and get the right result

see #22975. props aaroncampbell.

#66 @ocean90
11 years ago

  • Keywords needs-patch added; commit has-patch removed

#67 @ocean90
11 years ago

  • Keywords has-patch added; needs-patch removed

22975.suggest.patch removes the IE browser check since the method .bgiframe() on $result has no function because the method doesn't exist.

And it removes the check for Mozilla. keydown works for me in all browsers. Tested also via Browserstack down to Firefox 3.6.

#68 @markjaquith
11 years ago

Looks good to me. Go for it.

#69 @ocean90
11 years ago

In 24371:

Remove deprecated jQuery API .browser in suggest.js. No need for a replacement since it works without too. see #22975.

#70 @ocean90
11 years ago

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

#71 @wonderboymusic
10 years ago

Can someone tell Akismet to stop using .live()?

Note: See TracTickets for help on using tickets.