#23954 closed defect (bug) (fixed)
Checkbox value is not updated correctly inside media-views.js
Reported by: | NumberKruncher | Owned by: | 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)
Change History (8)
#2
@
12 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).
#3
@
12 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.
#4
@
12 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.
#5
@
12 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
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?