WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 5 months ago

Last modified 5 months ago

#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)

cropped-900e8ed0-2089-3e18-9f4f-88d37af58cd2-1[1].jpg (74.5 KB) - added by WebTrooper 7 months ago.
bc-twentyseventeen-childtheme.zip (889 bytes) - added by WebTrooper 7 months ago.
Twenty Seventeen Child Theme
1.jpg (233.3 KB) - added by WebTrooper 7 months ago.
Cropping the image, dragged to right handle toward the left.
2.jpg (226.8 KB) - added by WebTrooper 7 months ago.
The result seen in the customizer.
3.jpg (223.8 KB) - added by WebTrooper 7 months ago.
After publishing and closing the customizer.
cropped-bigstock-Windows-In-Venice-Italy-2722647-1.jpg (367.7 KB) - added by WebTrooper 7 months ago.
The actual "cropped" image.
42646.diff (2.5 KB) - added by adamsilverstein 6 months ago.

Download all attachments as: .zip

Change History (40)

#1 @WebTrooper
7 months ago

Edit: Sorry, I mean when flex-width and flex-height are both set to "false".

#2 @westonruter
7 months 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 @WebTrooper
7 months ago

Tested on 4.8.3 and it works properly. It appears this is a bug that came with 4.9.

#4 follow-up: @westonruter
7 months ago

  • Keywords reporter-feedback removed
  • Milestone changed from Awaiting Review to 4.9.1

@joemcgill Could this be related to #40211 or #21819?

#5 in reply to: ↑ 4 @joemcgill
7 months ago

Replying to westonruter:

@joemcgill Could this be related to #40211 or #21819?

#40211 looks like the more likely candidate. cc: @adamsilverstein.

#6 @adamsilverstein
7 months ago

I'll take a look.

#7 @johnbillion
7 months ago

  • Milestone changed from 4.9.1 to 4.9.2

#8 @adamsilverstein
7 months 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.

@WebTrooper
7 months ago

Twenty Seventeen Child Theme

@WebTrooper
7 months ago

Cropping the image, dragged to right handle toward the left.

@WebTrooper
7 months ago

The result seen in the customizer.

@WebTrooper
7 months ago

After publishing and closing the customizer.

@WebTrooper
7 months ago

The actual "cropped" image.

#9 @WebTrooper
7 months 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 @WebTrooper
7 months 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 @adamsilverstein
7 months ago

@WebTrooper thanks for clarifying and the code to reproduce, I'll take a look at this issue again.

#12 @adamsilverstein
6 months 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.

Last edited 6 months ago by adamsilverstein (previous) (diff)

This ticket was mentioned in Slack in #core-media by adamsilverstein. View the logs.


6 months ago

#14 @WebTrooper
6 months 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 @adamsilverstein
6 months 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 @WebTrooper
6 months 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 @adamsilverstein
6 months 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.


5 months ago

#19 @westonruter
5 months ago

@WebTrooper please test the changes in 42646.diff and report back if it fixes the issue you observed. Thanks!

#20 @dd32
5 months ago

  • Milestone changed from 4.9.2 to 4.9.3

Bumping to 4.9.3 due to 4.9.2s release

#21 @WebTrooper
5 months 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 @adamsilverstein
5 months 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.

#23 @adamsilverstein
5 months ago

  • Owner set to adamsilverstein
  • Status changed from reopened to assigned

#24 follow-up: @WebTrooper
5 months 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 @WebTrooper
5 months 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 );
Last edited 5 months ago by WebTrooper (previous) (diff)

#26 in reply to: ↑ 24 @westonruter
5 months 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.

#27 @WebTrooper
5 months ago

Thanks Weston, that did the trick. It works perfectly now. :)

#28 @adamsilverstein
5 months ago

  • Keywords commit added; needs-testing removed

Perfect, thanks for confirming @WebTrooper and thanks for the tip @westonruter!

This ticket was mentioned in Slack in #core by desrosj. View the logs.


5 months ago

#30 @SergeyBiryukov
5 months ago

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

In 42595:

Media: Store and reuse image cropper ratio settings if available, instead of overwriting.

Props adamsilverstein.
Fixes #42646.

#31 @SergeyBiryukov
5 months ago

In 42596:

Media: Store and reuse image cropper ratio settings if available, instead of overwriting.

Props adamsilverstein.
Merges [42595] to the 4.9 branch.
Fixes #42646.

#32 @SergeyBiryukov
5 months ago

In 42599:

Media: Fix JSHint error after [42595].

See #42646.

#33 @SergeyBiryukov
5 months ago

In 42600:

Media: Fix JSHint error after [42595].

Merges [42599] to the 4.9 branch.
See #42646.

Note: See TracTickets for help on using tickets.