WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 20 months ago

Last modified 20 months ago

#21106 closed defect (bug) (fixed)

Unable to add Taxonomies to custom post type taxonomies in 3.4.1

Reported by: lightmensendme Owned by: markjaquith
Milestone: 3.4.2 Priority: normal
Severity: major Version: 3.4
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

This is reproducible in at least IE8 and FF 13.0.1, WordPress 3.4.1, and at least two separate domains.

Problem: We have a least two sites which have custom post types and taxonomies originally added by the Custom Post Type UI plugin, in which we are unable to add new taxonomies to. After disabling all plugins and putting the post type/taxonomy code directly into the functions.php file: we are still unable to add new taxonomies.

Reproduction: 1. Add a custom post type and a custom post type taxonomy. Add new custom post type "entry/post" from the admin menu or from the Dashboard sidebar. If is a hierarchical custom post type taxonomy, add a new custom post taxonomy. When the "Add New [Taxonomy]" button is pressed, the entered name in the taxonomy text field stays present, the add button seems to disappear from its location, and the add button is now displayed at the end of the custom post taxonomy list. The new taxonomy has not been added. (see image attached)

  1. Add a custom post type and a custom post type taxonomy. Add new custom post type "entry/post" from the admin menu or from the Dashboard sidebar. If is a hierarchical custom post type taxonomy, which already has taxonomy present, check several taxonomies. You will notice that there is "lag" in selecting these current taxonomies (IE8 at least).
  2. Although I have not been able to reproduce this, a client has mentioned that on an add custom post page where there is custom post taxonomy, the page is unusable and can eventually crash in IE8.

Example functions.php

register_post_type('events', array(	'label' => 'Events','description' => '','public' => true,'show_ui' => true,'show_in_menu' => true,'capability_type' => 'post','hierarchical' => false,'rewrite' => array('slug' => ''),'query_var' => true,'supports' => array('title','editor','excerpt','trackbacks','custom-fields','comments','revisions','thumbnail','author','page-attributes',),'labels' => array (
  'name' => 'Events',
  'singular_name' => 'Event',
  'menu_name' => 'Events',
  'add_new' => 'Add Event',
  'add_new_item' => 'Add New Event',
  'edit' => 'Edit',
  'edit_item' => 'Edit Event',
  'new_item' => 'New Event',
  'view' => 'View Event',
  'view_item' => 'View Event',
  'search_items' => 'Search Events',
  'not_found' => 'No Events Found',
  'not_found_in_trash' => 'No Events Found in Trash',
  'parent' => 'Parent Event',
),) );

register_taxonomy('event-city',array (
  0 => 'events',
),array( 'hierarchical' => true, 'label' => 'Locations','show_ui' => true,'query_var' => true,'rewrite' => array('slug' => ''),'singular_label' => 'Location') );

Attachments (7)

WordPress 2012-06-28 13-03-05.png (15.7 KB) - added by lightmensendme 22 months ago.
Screenshot after adding new custom post taxonomy
21106.patch (433 bytes) - added by SergeyBiryukov 22 months ago.
wp3.4_21152.diff (1.9 KB) - added by markjaquith 20 months ago.
21106.2.patch (1.8 KB) - added by SergeyBiryukov 20 months ago.
21106.3.patch (1.8 KB) - added by SergeyBiryukov 20 months ago.
21106.4.patch (1.8 KB) - added by nacin 20 months ago.
21106.5.patch (1.2 KB) - added by SergeyBiryukov 20 months ago.

Download all attachments as: .zip

Change History (34)

lightmensendme22 months ago

Screenshot after adding new custom post taxonomy

comment:1 SergeyBiryukov22 months ago

  • Milestone changed from Awaiting Review to 3.4.2

I can reproduce issues 1 and 2 in Firefox 13 and IE 8.

Introduced in [20337] with jQuery 1.7.2.

comment:2 markjaquith22 months ago

I reproduced issue 1 in Chrome 19 and Firefox 13.

comment:3 follow-up: markjaquith22 months ago

"event-city" starts with the word "even". WordPress uses some colon-separated class names. like: add:event-citychecklist. See the :even in there? The issue manifests if you replace "event-city" with "oddsfish".

On line 81 of wp-includes/js/wp-lists.dev.js has the following line:

		if ( !e.is('[class^="add:' + list.id + ':"]') )

This test is failing when it shouldn't be.

Here's my theory: there was a bug introduced in jQuery 1.7.2 regarding attribute selector quoting. It's seeing :even or :odd in a quoted class selector and it's parsing that as the :even/:odd pseudo-classes that jQuery supports.

What's really frustrating is that I can't reproduce it more simply. I've been trying to make a simple proof of concept on jsfiddle.net

http://jsfiddle.net/g4CCw/

Maybe someone else will have better luck.

SergeyBiryukov22 months ago

comment:4 SergeyBiryukov22 months ago

21106.patch fixes issue 1 by checking id attribute instead of class.

comment:5 in reply to: ↑ 3 SergeyBiryukov22 months ago

We have similar code in at least one more place:
http://core.trac.wordpress.org/browser/trunk/wp-includes/js/wp-lists.dev.js?rev=17981#L392

Not sure at a glance if it fails or not. Would be great to pinpoint the exact change in jQuery behaviour, but so far I could not reproduce it in a standalone example either.

comment:6 markjaquith22 months ago

Koopersmith says it's probably a really low level thing that changed in their selector engine. The main takeaway is that our use of colons within class names is really ill-advised.

comment:7 markjaquith22 months ago

#21152 for a general "kill colons in class names" ticket.

comment:8 markjaquith22 months ago

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

In [21205]:

Change a jQuery selector to work around a change in jQuery 1.7.2 related to some class names containing colons. props SergeyBiryukov. fixes #21106 for trunk. see #21152

comment:9 markjaquith22 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening so we can consider it for a potential 3.4.2. Not sure this merits it all by itself, but if we have to release one anyway, might consider working it in.

comment:10 SergeyBiryukov22 months ago

Issue 2 (the lag in IE when selecting terms) still remains. Going to take a look by the weekend if no one beats me to it. Might be related to issue 3 as well.

comment:11 nacin21 months ago

  • Keywords has-patch added

comment:12 SergeyBiryukov21 months ago

Still needs a patch for issue 2.

comment:13 follow-up: wonderboymusic20 months ago

I patched the colon classes ticket: https://core.trac.wordpress.org/ticket/21152 - perhaps that can be used in some form?

Version 0, edited 20 months ago by wonderboymusic (next)

markjaquith20 months ago

comment:14 markjaquith20 months ago

Patch to backport [21205] and [21688] to the 3.4 branch.

comment:15 in reply to: ↑ 13 SergeyBiryukov20 months ago

Replying to wonderboymusic:

I patched the colon classes ticket: #21152 - perhaps that can be used in some form?

Tested the patch in IE 7 and 8, doesn't fix the lagging when selecting a taxonomy term. Seems to be caused by something else.

SergeyBiryukov20 months ago

SergeyBiryukov20 months ago

comment:16 SergeyBiryukov20 months ago

Closed #21747 as a duplicate.

In trunk (with jQuery 1.8.1), the symptoms are different:

  1. The lagging appears when checking any category on a regular Edit Post screen as well, in both IE and Firefox.
  2. If there's a taxonomy starting with "even", IE throws an error when checking its terms:
    Stack overflow at line: 2
    

Upon debugging, I've found two particular places which seem to be slowing things down:

  1. '[class^="add:' + list.id + ':"]:not(form)' selector in wpList.process(): http://core.trac.wordpress.org/browser/tags/3.4.1/wp-admin/js/post.dev.js#L345
    As far as I can see, it only runs once, but commenting it out resulted in a significant speedup. With the patch from #21152 the results were the same, which made me think the culprit here is :not(form) rather than the :even part. Removing :not(form) confirmed that.
  2. '#' + taxonomy + 'checklist li.popular-category :checkbox selector in post.js: http://core.trac.wordpress.org/browser/tags/3.4.1/wp-includes/js/wp-lists.dev.js#L400 jQuery.com suggests replacing it with [type="checkbox"]:

    Because :checkbox is a jQuery extension and not part of the CSS specification, queries using :checkbox cannot take advantage of the performance boost provided by the native DOM querySelectorAll() method. For better performance in modern browsers, use [type="checkbox"] instead.

21106.2.patch fixes the stack overflow in IE and the lagging in both Firefox and IE. It should cover all the existing core usage of those class names. Tested in 3.4.1 (with jQuery 1.7.2) as well.

21106.3.patch binds all three non-form functions to both a and input elements, in case plugins might rely on that. Seems is a little bit slower, but perhaps safer. Not sure if that's necessary though.

Last edited 20 months ago by SergeyBiryukov (previous) (diff)

comment:17 azaozz20 months ago

We should be able to escape the : when using the CSS classes in jQuery:

If you wish to use any of the meta-characters ( such as !"#$%&'()*+,./:;<=>?@[\]^`{|}~ )
as a literal part of a name, you must escape the character with two backslashes: \\.
Last edited 20 months ago by azaozz (previous) (diff)

comment:18 nacin20 months ago

In [21717]:

Merge [21205] and [21688] to the 3.4 branch. props SergeyBiryukov, yoavf. fixes #21590. see #21106.

comment:19 follow-up: nacin20 months ago

After [21717], is there anything else necessary for the 3.4 branch?

SergeyBiryukov's comment suggests that this remaining issue has to do with jQuery 1.8.x, which is WP trunk only. The Sizzle selector engine was rebuilt in 1.8, so I would not be surprised if this did not affect 1.7. The original report does suggest this is a problem with 3.4.x. Does 21106.2.patch (or .3.patch) handle both?

comment:20 nacin20 months ago

The selectors in 21106.2.patch are a bit nasty, but look good to me.

We can actually chain in 21106.3.patch, rather than relying on individual $el.find('a, input') calls. That would probably be more performant than .2.patch and lessens the risk in the 3.4 branch.

nacin20 months ago

SergeyBiryukov20 months ago

comment:21 in reply to: ↑ 19 SergeyBiryukov20 months ago

Replying to nacin:

After [21717], is there anything else necessary for the 3.4 branch?

I think so. In 3.4.1, the lagging (~3 seconds) only appears in IE when checking terms of a taxonomy starting with "even" (as described in the ticket). It would be good to fix that.

In trunk, the lagging appears in both IE and Firefox (~1 second in Firefox and IE 9, ~5 seconds in IE 8) when checking any category on a regular Edit Post screen as well. Additionally, a stack overflow error occurs in IE 8 when checking terms of a taxonomy starting with "even".

Upon more testing, it turned out that 21106.2.patch doesn't work as intended (the functions are not binded properly for some reason), so I've decided to simplify it.

21106.5.patch fixes the lagging in IE in 3.4.1. In trunk, it also fixes stack overflow in IE 8 and at least partially fixes the lagging (~0.2 seconds, almost unnoticeable, in FF, ~1 second in IE). We could look into further optimization in a new ticket.

comment:22 SergeyBiryukov20 months ago

Actually, checking categories is not the only slow thing in trunk.

With jQuery 1.8.1, most of the actions on Add New Post screen are extremely slow in IE 8: editing post title, changing post format, etc. This can also be seen on
lessbloat's video (User 6) at 9:30.

Still, 21106.5.patch fixes that.

comment:23 follow-up: azaozz20 months ago

Looked at this too. This part of wp-lists.js is pretty bad.

  1. The process() runs on load with no arg, then on adding elements with the element as arg. This isn't needed if the events are delegated (i.e. attached to $(document) so they will trigger when new elements are added with ajax). Also, on load $el = $(document) so doing $el.find(...) goes through the whole DOM.

For this to work it shouldn't use .find() and only attach the events to $(document) on load. There's no need to run it on each ajax response. All this can be seen in action on the Comments screen.

  1. We hit a bug in jQuery. When a class name contains :event it cannot be used as selector in jQuery even when the : is escaped (it won't select any elements). This affects process() too.
Last edited 20 months ago by azaozz (previous) (diff)

comment:24 nacin20 months ago

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

In [21737]:

Modify a few jQuery selectors to prevent overflows and lagging. Todo, rewrite wp-lists and improve all of these selectors.

props SergeyBiryukov. fixes #21106 for trunk. see #21152.

comment:14 nacin20 months ago

In [21738]:

Modify a few jQuery selectors to prevent overflows and lagging. props SergeyBiryukov. fixes #21106. merges [21737] to the 3.4 branch.

comment:15 nacin20 months ago

The rest of this can be handled in #21152.

comment:16 in reply to: ↑ 23 azaozz20 months ago

Replying to azaozz:

  1. We hit a bug in jQuery. When a class name contains :event it cannot be used as selector in jQuery even when the : is escaped...

For the record: this only happens when wp-lists.js is loaded. So it's not a jQuery bug, it's wp-lists bug :)

Note: See TracTickets for help on using tickets.