WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#21152 closed defect (bug) (fixed)

Kill use of colons in class names

Reported by: markjaquith Owned by: azaozz
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.4
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

Some of the WordPress admin uses class names with colon-separated values that have special meanings in our JS code This is a bad idea. Colons are a special character in CSS selector engines.

See #21106 for a really odd bug that cropped up because a css class contained the text foo:event which got parsed as an :even pseudo-class in JS.

HTML5 data attributes are the better way forward, in terms of attaching data to dom elements. We should kill all instances of colons in favor of data attributes.

Attachments (7)

remove-colon-classes.diff (29.1 KB) - added by wonderboymusic 6 years ago.
21152.2.diff (29.2 KB) - added by SergeyBiryukov 6 years ago.
Refreshed after [21688]
21152.3.diff (29.3 KB) - added by SergeyBiryukov 6 years ago.
21152.4.diff (29.4 KB) - added by SergeyBiryukov 6 years ago.
wp-lists.js.diff (393 bytes) - added by jackreichert 6 years ago.
fix for custom field ajax
21152.5.diff (29.5 KB) - added by SergeyBiryukov 6 years ago.
Refreshed after [21955]
21152.6.diff (29.5 KB) - added by SergeyBiryukov 6 years ago.

Download all attachments as: .zip

Change History (36)

#1 @markjaquith
6 years ago

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

#2 @azaozz
6 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 3.5

Yes, these classes are older than four years. Colons in class names weren't a problem at the time. +1 to use data attributes instead, and the sooner that's fixed, the better.

#3 @markjaquith
6 years ago

We should also do it completely within the 3.5 cycle. This might break some plugins that depend on those class names for JS or CSS. Better to do a clean break that we can announce early in the cycle.

#4 @azizur
6 years ago

  • Cc azizur added

#5 @azaozz
6 years ago

To fix this we would need to change the way wp-lists.js works. Currently it passes values in the class names separated with colons.

#6 @scribu
6 years ago

Since it needs refactoring, maybe we could transform wp-lists.js into a Backbone view. (I have 0 experience with Backbone.js)

#7 @wonderboymusic
6 years ago

  • Keywords has-patch added; needs-patch removed

There is one remaining piece: I can't figure out why nav menu admin lists need the class="list:{$post_type} ..." but they break without it. Other than that, everything seems to work. Attached a patch.

#8 follow-up: @wonderboymusic
6 years ago

P.S. are the min files auto-generated now?

#9 in reply to: ↑ 8 @scribu
6 years ago

Replying to wonderboymusic:

P.S. are the min files auto-generated now?

Yes, we have bump-bot that does that, but it probably needs an update after #21633.

In any case, you don't need to worry about it.

#10 @wonderboymusic
6 years ago

Updated the patch - fixed the CSS collision happening on Nav Menus, fixed the action context when you spam / unspam / spam / unspam etc. This legacy code, while not wholly inelegant, is brutal to debug.

#11 @nacin
6 years ago

  • Milestone changed from 3.5 to 3.4.2

@SergeyBiryukov
6 years ago

Refreshed after [21688]

#12 @nacin
6 years ago

  • Milestone changed from 3.4.2 to 3.5

Doesn't seem like there have been any bugs other than #21106 that were reported against 3.4. Moving back to the 3.5 milestone.

#13 @nacin
6 years ago

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.

#14 @SergeyBiryukov
6 years ago

#21852 was marked as a duplicate.

#15 @SergeyBiryukov
6 years ago

21152.3.diff moves the whole class names to data-wp-lists="...", as suggested by azaozz in ticket:21852:4. This restores AJAX in custom fields (#21852). Also noticed a couple of places not covered by the previous patches.

Tested:

  • Adding categories on Edit Post screen
  • Adding and deleting categories on Categories screen
  • Adding, deleting and updating custom fields
  • Comment moderation

Could definitely use a second pair of eyes and more testing.

#16 @SergeyBiryukov
6 years ago

21152.4.diff fixes keyboard shortcuts for comment moderation.

#17 follow-up: @jackreichert
6 years ago

  • Cc jack@… added

With the latest SVN I'm still having trouble with updating Custom Fields. Attached is a patch that fixes it. I DID notice that all IDs for the update buttons are the same, this patch does not correct that, only the ability to ajax update the fields.

@jackreichert
6 years ago

fix for custom field ajax

#18 in reply to: ↑ 17 @SergeyBiryukov
6 years ago

Replying to jackreichert:

I DID notice that all IDs for the update buttons are the same

This was fixed in [21781] (for #21829), so wp-lists.js.diff doesn't look correct. Make sure you have the latest revision.

this patch does not correct that, only the ability to ajax update the fields.

AJAX is broken due to some changes in #21598. #21852 was an attempt to fix that, but the consensus was to focus on this ticket instead, and 21152.4.diff should fix that as well.

#19 @ocean90
6 years ago

#22003 was marked as a duplicate.

@SergeyBiryukov
6 years ago

Refreshed after [21955]

#20 @wonderboymusic
6 years ago

<IMO>I understand why switching to data-wp-lists is the easiest fix, but RegEx'ing colon-separated values still makes me want to vom. </IMO>

#21 @aesqe
6 years ago

  • Cc aesqe@… added

#22 @arena
6 years ago

custom field ajax not working in 3.5-beta2-22265

#23 @SergeyBiryukov
6 years ago

Yes, the patch was not committed yet.

#24 @SergeyBiryukov
6 years ago

Refreshed with the fix for adding link categories from #22340.

#25 @SergeyBiryukov
6 years ago

  • Owner set to azaozz
  • Status changed from new to reviewing

#26 @azaozz
6 years ago

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

Fixed in [22396].

#27 follow-up: @arena
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

there are stil some class names with colon-separated values inserted via js in 3.5

such as

wp-admin/js/cat.js:1:jQuery(document).ready(function(b){var a=function(){return""!==b("#newcat").val()};b("#jaxcat").prepend('<span id="ajaxcat"><input type="text" name="newcat" id="newcat" size="16" autocomplete="off"/><input type="button" name="Button" class="add:categorychecklist:jaxcat" id="catadd" value="'+catL10n.add+'"/><input type="hidden"/><input type="hidden"/><span id="howto">'+catL10n.how+'</span></span><span id="cat-ajax-response"></span>');b("#categorychecklist").wpList({alt:"",response:"cat-ajax-response",confirm:a})});

#28 @nacin
6 years ago

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

This ticket was closed as fixed in 3.5. Please open a new ticket.

#29 in reply to: ↑ 27 @SergeyBiryukov
6 years ago

Replying to arena:

<input type="button" name="Button" class="add:categorychecklist:jaxcat" id="catadd"

That input has data-wp-lists attribute in 3.5, not class:
http://core.trac.wordpress.org/browser/tags/3.5/wp-admin/js/cat.js#L3

Note: See TracTickets for help on using tickets.