Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#23954 closed defect (bug) (fixed)

Checkbox value is not updated correctly inside media-views.js

Reported by: numberkruncher's profile NumberKruncher Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.0 Priority: normal
Severity: major Version: 3.5.1
Component: Media Keywords: has-patch
Focuses: Cc:

Description

I added two custom fields to the "Edit Gallery" interface using a hook and discovered that my "select" and "text" inputs were working fine, but my "checkbox" input did not work at all.

I enabled SCRIPT_DEBUG so that I could explore further and discovered that the bug on line 3939 of media-views.js.

A double-bang has been incorrectly used to cast from a string encoded boolean value to an actual boolean value (since !!"false" is actually true).

This is the current implementation:

} else if ( $setting.is('input[type="checkbox"]') ) {
    $setting.attr( 'checked', !! value );
}

This is my recommendation which resolves the issue:

} else if ( $setting.is('input[type="checkbox"]') ) {
    value = value !== false && value !== 'false';
    $setting.attr( 'checked', value );
}

I would submit a patch, but I do not know how to do that.

Attachments (1)

23954.diff (470 bytes) - added by adamsilverstein 11 years ago.
fix custom checkmarks in media modal

Download all attachments as: .zip

Change History (8)

#1 @adamsilverstein
11 years ago

  • Cc adamsilverstein@… added

thanks for submitting that bug report - i could work on the patch, but first can you provide the code that you are using to produce the problem, the code that uses the hook; being able to reproduce the bug here will be critical to testing whether your proposed fix works in my dev environment.

i have a question about your code: why are you checking for both false and 'false' can't we be sure it will be one or the other?

#2 @NumberKruncher
11 years ago

Replying to adamsilverstein:

thanks for submitting that bug report - i could work on the patch, but first can you provide the code that you are using to produce the problem, the code that uses the hook; being able to reproduce the bug here will be critical to testing whether your proposed fix works in my dev environment.

Here is a video that demonstrates the problem:
http://www.youtube.com/watch?v=LRkWqIEiRqk

This is the plugin used in the demonstration video:

<?php defined('WPINC') or die('Access denied.');

/*
Plugin Name: GalleryBugReport
*/

class GalleryBugReport {

	function __construct() {
		add_action('print_media_templates', array(&$this, 'on_print_media_templates'));
	}

	function on_print_media_templates() {
		if (!isset(get_current_screen()->id) || get_current_screen()->base != 'post')
			return;
?>
<script type="text/javascript">
jQuery(function() {
	var $ = jQuery;
	var media = wp.media;

	// Wrap the render() function to append controls
	media.view.Settings.Gallery = media.view.Settings.Gallery.extend({
		render: function() {
			media.view.Settings.prototype.render.apply(this, arguments);

			// Append the custom template.
			this.$el.append(media.template('custom-gallery-settings'));

			// Save the setting.
			media.gallery.defaults.show_captions = true;
			this.update.call(this, 'show_captions');
			return this;
		}
	})
})
</script>
<script type="text/html" id="tmpl-custom-gallery-settings">
	<label class="setting">
		<span>Show Captions</span>
		<input type="checkbox" data-setting="show_captions"/>
	</label>
</script>
<?php
	}

}

new GalleryBugReport();

?>

i have a question about your code: why are you checking for both false and 'false' can't we be sure it will be one or the other?

Sometimes the value comes through as boolean and sometimes the value comes through as a string encoded representation of the boolean value. So the code needs to cater for both approaches.

I assume that the double-bang was intended to prevent this very issue (because I cannot think of any other reason to use a double-bang there).

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

#3 @NumberKruncher
11 years ago

A better solution might be to use the following:

$setting.attr( 'checked', !!value && value !== 'false' );

Just in case the double-bang was used for a browser compatibility issue or such.

Edit: The double-bang is indeed needed because the checked value can also be undefined which must be converted into a boolean value so that jQuery updates the checked state.

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

#4 @NumberKruncher
11 years ago

When the checkbox is ticked the shortcode attribute is not outputted as show_captions="true" which I guess is fine. I notice that this can be explicitly forced by overriding wp.media.gallery.shortcode in my plugin, though this really doesn't bother me.

However, when the "Edit Gallery" interface is first shown, the attribute "show_captions" is not present because its value is implied. So the checkbox is not checked (even though it should be).

To fix this I added the following to my plugin:

var mediaGalleryAttachments = media.gallery.attachments;
media.gallery.attachments = function(shortcode) {

	// Needed to ensure that shortcode attribute is added. When "Show Captions"
	// is checked the shortcode is not added to output because the value `true`
	// seems to be implied. But in order for this to work we must detect when
	// attribute is not specified so that checkbox is ticked when the "Edit Gallery"
	// interface is first shown.

	if (shortcode && shortcode.tag === 'gallery') {
		if (shortcode.attrs.named['show_captions'] === undefined)
			shortcode.attrs.named['show_captions'] = true;
	}

	return mediaGalleryAttachments(shortcode);
};

This bug fix and the above snippet allow me to add my own checkboxes to the "Edit Gallery" interface. Once this bug fix has been applied I will write a tutorial in my blog for others who would like to add a checkbox to the "Edit Gallery" interface.

@adamsilverstein
11 years ago

fix custom checkmarks in media modal

#5 @adamsilverstein
11 years ago

  • Keywords has-patch dev-feedback 2nd-opinion added; needs-patch removed

thanks for finding that apparent bug and the solution, made my part easy!

the attached patch ( 23954.diff ) includes your second correction. i tested and verified that after applying the patch, the checkmark works after applying. i think this is a valuable solution, but we'll let some more seasoned core developers weigh in on the approach.

the second issue you describe might be considered normal behavior - when you choose the default of any option in the gallery its option name does not appear in the shortcode. for example checking order by rand and then unchecking, or changing to 4 columns then back to 3, once you are at the default value the shortcode value goes away. so if captions are on by default there would be no captions="true" in the shortcode at all.

i noticed the built in random order checkbox uses a value "on" when its on, maybe if you go this route the 'false' !== false issue goes away - you just test for 'on' otherwise assume its off?

i wasn't familiar with the !! operator: its taking the value and returning a boolean, explained well here

creating these patches isn't hard, you should give it a try - your efforts will be greatly appreciated!

start here: http://codex.wordpress.org/Contributing_to_WordPress or here: http://make.wordpress.org/core/handbook/ to learn more

#6 @wonderboymusic
10 years ago

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

In 28371:

In wp.media.view.Settings::update(), when handling checkboxes, check for a value of 'false' in addition to casting value to boolean. !! 'false' evaluates to true.

Props adamsilverstein.
Fixes #23954.

#7 @wonderboymusic
10 years ago

  • Keywords dev-feedback 2nd-opinion removed
  • Milestone changed from Awaiting Review to 4.0
Note: See TracTickets for help on using tickets.