Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a bug that caused the display ratio of images to be corrupted starting from 6.3.( fix #53555 ) #53598

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

ddryo
Copy link
Contributor

@ddryo ddryo commented Aug 12, 2023

What?

Since 6.3, there is a bug that the display ratio is broken when the size is specified in the image block. (#53555)

This PR fixes the above problem.

How?

If both width and height have size specifications, they are converted to aspect-ratio rather than being output as they are in the style attribute.

Testing Instructions

  • Check with v6.3
    1. Specify a size for the image block.
    2. Confirm with phone size, etc.
  • Check blocks placed before v.6.2
    1. Place image block in v.6.2 and specify 75%, etc.
    2. Copy and paste that block and make sure it is converted to the new specifications.
    3. Confirm with phone size, etc.

As an example, I share one image block code I installed in 6.2.

<!-- wp:image {"width":600,"height":450,"sizeSlug":"large"} -->
<figure class="wp-block-image size-large is-resized"><img src="https://s.w.org/images/core/5.3/MtBlanc1.jpg" alt="" width="600" height="450"/></figure>
<!-- /wp:image -->

Note

There was already a PR for 53274 that fixes another problem, which was already different from the 6.3 situation.

The 53274 changed the width, height to a string type, but blocks placed before 6.2 were not migrated correctly to a string type, so we are fixing those as well.

(In deprecated.js, migrate process was also added to v6~v4)

@ddryo ddryo requested a review from ajitbohra as a code owner August 12, 2023 06:55
@github-actions
Copy link

github-actions bot commented Aug 12, 2023

Warning: Type of PR label error

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Accessibility (a11y), [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: [Block] Image.

Read more about Type labels in Gutenberg.

@gamebits
Copy link

@scruffian Since you reviewed the similar #53274, could we get your eyes on this one, too?

@getdave
Copy link
Contributor

getdave commented Nov 23, 2023

@MaggieCabrera Is this what we observed with our testing earlier this week when that one image didn't show the controls?

@MaggieCabrera
Copy link
Contributor

@MaggieCabrera Is this what we observed with our testing earlier this week when that one image didn't show the controls?

It could be happening that after we use "Replace" on an existing image (this is the code for the original image in TT4) the dimensions are added to the block and that makes aspect ratio fail

@ajlende ajlende self-assigned this Dec 4, 2023
Comment on lines +56 to +57
// Note that only width may be "auto".
if ( ! aspectRatio && height && width && 'auto' !== width ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

height can be set to auto in the editor with a fixed width, so we can't make this assumption here. We want to support layouts where that is required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If migrate needs to be added to all these deprecations, that seems to me like a bug in applyBlockDeprecatedVersions, not the image block deprecations. The documentation for block deprecations doesn't mention this, and it seems rather awkward and error-prone to have to add them everywhere.

Comment on lines +58 to +59
const w = parseInt( width );
const h = parseInt( height );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea for the image block moving forward is to support units other than px, so we don't want to parseInt without confirming that they are pixel values first.

Comment on lines +57 to +66
if ( ! aspectRatio && height && width && 'auto' !== width ) {
const w = parseInt( width );
const h = parseInt( height );
if ( ! isNaN( w ) && ! isNaN( h ) ) {
imgStyle.aspectRatio = `${ w }/${ h }`;
}
} else {
if ( 'string' === typeof width ) imgStyle.width = width;
if ( 'string' === typeof height ) imgStyle.height = height;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here should be moved to edit.js to allow for more flexibility of styling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants