WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 19 months ago

Last modified 19 months 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 23 months ago.
21152.2.diff (29.2 KB) - added by SergeyBiryukov 23 months ago.
Refreshed after [21688]
21152.3.diff (29.3 KB) - added by SergeyBiryukov 22 months ago.
21152.4.diff (29.4 KB) - added by SergeyBiryukov 22 months ago.
wp-lists.js.diff (393 bytes) - added by jackreichert 22 months ago.
fix for custom field ajax
21152.5.diff (29.5 KB) - added by SergeyBiryukov 22 months ago.
Refreshed after [21955]
21152.6.diff (29.5 KB) - added by SergeyBiryukov 21 months ago.

Download all attachments as: .zip

Change History (36)

comment:1 markjaquith2 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

comment:2 azaozz2 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.

comment:3 markjaquith2 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.

comment:4 azizur2 years ago

  • Cc azizur added

comment:5 azaozz23 months 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.

comment:6 scribu23 months ago

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

comment:7 wonderboymusic23 months 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.

comment:8 follow-up: wonderboymusic23 months ago

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

comment:9 in reply to: ↑ 8 scribu23 months 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.

comment:10 wonderboymusic23 months 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.

comment:11 nacin23 months ago

  • Milestone changed from 3.5 to 3.4.2

SergeyBiryukov23 months ago

Refreshed after [21688]

comment:12 nacin23 months 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.

comment:13 nacin23 months 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.

SergeyBiryukov22 months ago

comment:14 SergeyBiryukov22 months ago

#21852 was marked as a duplicate.

comment:15 SergeyBiryukov22 months 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.

SergeyBiryukov22 months ago

comment:16 SergeyBiryukov22 months ago

21152.4.diff fixes keyboard shortcuts for comment moderation.

comment:17 follow-up: jackreichert22 months 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.

jackreichert22 months ago

fix for custom field ajax

comment:18 in reply to: ↑ 17 SergeyBiryukov22 months 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.

comment:19 ocean9022 months ago

#22003 was marked as a duplicate.

SergeyBiryukov22 months ago

Refreshed after [21955]

comment:20 wonderboymusic22 months 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>

comment:21 aesqe22 months ago

  • Cc aesqe@… added

comment:22 arena21 months ago

custom field ajax not working in 3.5-beta2-22265

comment:23 SergeyBiryukov21 months ago

Yes, the patch was not committed yet.

SergeyBiryukov21 months ago

comment:24 SergeyBiryukov21 months ago

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

comment:25 SergeyBiryukov21 months ago

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

comment:26 azaozz20 months ago

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

Fixed in [22396].

comment:27 follow-up: arena19 months 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})});

comment:28 nacin19 months 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.

comment:29 in reply to: ↑ 27 SergeyBiryukov19 months 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.