#42646 closed defect (bug) (fixed)
Customizer cropped images issue with flex-width=false and flex-height=false
Reported by: | WebTrooper | Owned by: | adamsilverstein |
---|---|---|---|
Milestone: | 4.9.3 | Priority: | normal |
Severity: | normal | Version: | 4.9 |
Component: | Customize | Keywords: | has-patch has-screenshots commit |
Focuses: | javascript | Cc: |
Description
The issue occurs in both, add_theme_support('custom-header') and WP_Customize_Cropped_Image_Control
When flex-width and flex-height are both set to true, I am still able to drag the vertical and horizontal cropping handles independently, thus changing the proportion of the cropped area of the image. The resulting "cropped" image is then stretched or squished to the correct crop size.
With flex-width and flex-height both set to false (or left to default), only the corners of the cropping area should be draggable, thus maintaining the aspect ratio. I believe it used to work that way, but now it doesn't.
Tested on WordPress 4.9, PHP 7
Attachments (7)
Change History (40)
#2
@
7 years ago
- Keywords reporter-feedback added
@WebTrooper is this a new issue in 4.9 or did it exist in previous versions of WordPress? Would you please check in 4.8.3?
#3
@
7 years ago
Tested on 4.8.3 and it works properly. It appears this is a bug that came with 4.9.
#4
follow-up:
↓ 5
@
7 years ago
- Keywords reporter-feedback removed
- Milestone changed from Awaiting Review to 4.9.1
#5
in reply to:
↑ 4
@
7 years ago
#8
@
7 years ago
@WebTrooper can you provide steps to reproduce with one of the default WordPress themes or some code with a control that doesn't crop as you expect it? I tried to reproduce and am seeing correct cropping behavior in the customizer with twentyseventeen and its icon and header selectors. The cropping seems unchanged from previous versions.
#9
@
7 years ago
- Keywords has-screenshots added
Sorry for the late reply... holiday weekend. I'm not sure how much code I can provide as it's a simple matter to set the args to false. Attached is a twentyseventeen child theme with code as I described (see the functions.php)
In image 1 I dragged the right handle toward the left to get a tall, narrow crop, which I shouldn't be able to do.
Image 2 shows the cropped (squished) image in the customizer.
Image 3 shows the image in the theme after publishing and closing the customizer.
Image 4 is the actual image that I right clicked and saved (not a screen shot) from my browser.
#10
@
7 years ago
Just to be clear, you can add the following to twentyseventeen or a childtheme thereof, to the functions.php.
function bc_twentyseventeen_custom_header_setup() {
remove_theme_support( 'custom-header');
add_theme_support( 'custom-header', apply_filters( 'twentyseventeen_custom_header_args', array(
'default-image' => get_parent_theme_file_uri( '/assets/images/header.jpg' ),
'width' => 2000,
'height' => 1200,
'flex-height' => false,
'flex-width' => false,
'video' => true,
'wp-head-callback' => 'twentyseventeen_header_style',
) ) );
}
add_action( 'after_setup_theme', 'bc_twentyseventeen_custom_header_setup', 11 );
#11
@
7 years ago
@WebTrooper thanks for clarifying and the code to reproduce, I'll take a look at this issue again.
#12
@
7 years ago
- Focuses javascript added
- Keywords reporter-feedback has-patch added
@WebTrooper Can you please give 42646.diff a test and let me know if this resolves the issue you were experiencing? it does in my testing. This patch fixes an issue where the cropper settings were essentially overwritten after a change to make the shift key restrict dimensions to a 1:1 ration. Now they are stored and used if available, also restored after the shift drag behavior.
Thanks.
This ticket was mentioned in Slack in #core-media by adamsilverstein. View the logs.
7 years ago
#14
@
7 years ago
- Resolution set to maybelater
- Status changed from new to closed
Much as I'd like to be helpful I'm afraid I'll have to bow out of this issue. The fix does not work for me but actually, I'm not forcing crop dimensions on any of my sites anyway. It's just something I noticed while messing around and thought the dev team should know about. If I'm the only person reporting it and doesn't seem to be an issue with anybody else, then maybe it's just me. I'm not a developer and don't have a developer setup for testing and debugging, and /wp-includes/js/media/views/cropper.js
doesn't seem to exist in version 4.8 or 4.9. I did the edits in /wp-includes/js/media-views.js
but it didn't fix the issue. I'm sorry to have taken up your valuable time with what appears to be a non-issue. You guys do awesome work and I sincerely appreciate it. Thank you for looking into this matter. If nobody else is reporting this issue then maybe it's time to move on to more important bug fixes.
#15
@
7 years ago
- Keywords needs-testing added; reporter-feedback removed
- Resolution maybelater deleted
- Status changed from closed to reopened
@WebTrooper actually, i was able to reproduce the issue you raised, and verify the commit that broke the behavior. This patch should fix the issue, hopefully we can get some additional testers to confirm.
#16
@
7 years ago
Okay, that's different. I didn't realize you reproduced the issue. I'll give it another shot soon as I can but probably won't be until the weekend. I'll also read up on testing, tools, protocols, etc. Ya never know, I might learn something.
#17
@
7 years ago
Yes, thank you for the bug report. I used git to take wordpress back to the this commit: r41557. Before this commit, the behavior was different.
This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.
7 years ago
#19
@
7 years ago
@WebTrooper please test the changes in 42646.diff and report back if it fixes the issue you observed. Thanks!
#21
@
7 years ago
Please tell me if I'm doing this wrong.
There is no /wp-includes/js/media/views/cropper.js file. In fact, there is no /wp-includes/js/media/ directory. However, the code and changes in 42646.diff appear to be identical to that of /wp-includes/js/media-views.js, so that's where I made the changes.
I commented out everything that is highlighted in red. I added everything that is highlighted in green.
There is no difference -- I can still drag the crop handles "almost" anyway I please. The only exception is that I cannot drag the bottom crop handle downward to make a larger cropped area, which was the case before editing /wp-includes/js/media-views.js
This is on a basic WP 4.9.2 install with Wordpress Beta Tester installed, set to Point Release, then ran Update.
#22
@
7 years ago
@WebTrooper thanks for testing. the folder wp-includes/js/media is part of the development repo and you won't see this in a regular WordPress install. All you should need to do for this ticket is patch the media-views.js file which is sounds like you did, however perhaps you missed some part.
I'm going to capture some before/after screencasts showing the change and then get this fix committed, once its in trunk you should be able to test it more easily with the beta testers plugin.
#24
follow-up:
↓ 26
@
7 years ago
Adam, thank you for the explanation. Perhaps I did miss something when applying the changes. I'll try again and let you know the results. Also thank you for taking over this ticket, as though you don't already have enough on your plate :) I look forward to your screen cast. Cheers!
#25
@
7 years ago
The following is my exact changes to /wp-includes/js/media-views.js
I can still crop any width/height as I please. The only thing I can't do is drag the bottom crop handle downward, which I could not do before the the changes. I can still move the handles freely and crop without respect to aspect ratios. Here's a video - https://youtu.be/XBNMmlf1X1k
imgOptions = _.extend(imgOptions, { parent: this.$el, onInit: function() { // this.parent.children().on( 'mousedown touchstart', function( e ){ // if ( e.shiftKey ) { // Store the set ratio. var setRatio = imgSelect.getOptions().aspectRatio; // On mousedown, If no ratio is set and the shift key is down, used a 1:1 ratio. this.parent.children().on( 'mousedown touchstart', function( e ) { // If no ratio is set and the shift key is down, used a 1:1 ratio. if ( ! setRatio && e.shiftKey ) { imgSelect.setOptions( { aspectRatio: '1:1' } ); // } else { // imgSelect.setOptions( { // aspectRatio: false // } ); } } ); this.parent.children().on( 'mouseup touchend', function( e ) { // Restore the set ratio. imgSelect.setOptions( { aspectRatio: setRatio ? setRatio : false } ); } ); } } ); this.trigger('image-loaded');
And here is my theme settings
function bc_twentyseventeen_custom_header_setup() { remove_theme_support( 'custom-header'); add_theme_support( 'custom-header', apply_filters( 'twentyseventeen_custom_header_args', array( 'default-image' => get_parent_theme_file_uri( '/assets/images/header.jpg' ), 'width' => 2000, 'height' => 1200, 'flex-height' => false, 'flex-width' => false, 'video' => true, 'wp-head-callback' => 'twentyseventeen_header_style', ) ) ); } add_action( 'after_setup_theme', 'bc_twentyseventeen_custom_header_setup', 11 );
#26
in reply to:
↑ 24
@
7 years ago
Replying to WebTrooper:
Adam, thank you for the explanation. Perhaps I did miss something when applying the changes. I'll try again and let you know the results. Also thank you for taking over this ticket, as though you don't already have enough on your plate :) I look forward to your screen cast. Cheers!
@WebTrooper Have you added define('SCRIPT_DEBUG', true);
to your wp-config.php
? Without that, WP will use the minified JS instead of the JS you patch.
#28
@
7 years ago
- Keywords commit added; needs-testing removed
Perfect, thanks for confirming @WebTrooper and thanks for the tip @westonruter!
Edit: Sorry, I mean when flex-width and flex-height are both set to "false".