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.

Sign up for GitHub

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

Image block: Fix responsive sizing in lightbox #51823

Merged
merged 8 commits into from
Jun 29, 2023
Merged

Image block: Fix responsive sizing in lightbox #51823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b34a6fa
Rewrite logic to calculate image dimensions based on aspect ratio
artemiomorales Jun 23, 2023
a0371a8
Add padding for lightbox images using fade animation
artemiomorales Jun 23, 2023
ce931b2
Use window inner dimensions to account for mobile address bar
artemiomorales Jun 27, 2023
49181a5
Revise to deactivate responsive image on lightbox close
artemiomorales Jun 27, 2023
58a11a2
Use built-in directive for mouseover event
artemiomorales Jun 27, 2023
af76853
Add safe area inset to close button positioning
artemiomorales Jun 27, 2023
a562af9
Revert "Use built-in directive for mouseover event"
artemiomorales Jun 28, 2023
f1971e3
Update tests
artemiomorales Jun 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions packages/block-library/src/image/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,8 @@

.close-button {
position: absolute;
top: 12.5px;
right: 12.5px;
top: calc(env(safe-area-inset-top) + 12.5px);
right: calc(env(safe-area-inset-right) + 12.5px);
padding: 0;
cursor: pointer;
z-index: 5000000;
Expand Down Expand Up @@ -231,6 +231,18 @@
}

&.fade {
.wp-block-image {
padding: 40px 0;

@media screen and (min-width: 480px) {
padding: 40px;
}

@media screen and (min-width: 1920px) {
padding: 40px 80px;
}
}

&.active {
visibility: visible;
animation: both turn-on-visibility 0.25s;
Expand Down
61 changes: 31 additions & 30 deletions packages/block-library/src/image/view-interactivity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ store( {
return context.core.image.lightboxEnabled ? 'dialog' : '';
},
responsiveImgSrc: ( { context } ) => {
return context.core.image.activateLargeImage
return context.core.image.activateLargeImage &&
context.core.image.hideAnimationEnabled
? ''
: context.core.image.imageCurrentSrc;
},
Expand Down Expand Up @@ -206,48 +207,48 @@ function setZoomStyles( imgDom, context, event ) {
let targetWidth = context.core.image.targetWidth;
let targetHeight = context.core.image.targetHeight;

const verticalPadding = 40;
// Since the lightbox image has `position:absolute`, it
// ignores its parent's padding, so we need to set padding here
// to calculate dimensions and positioning.

// As per the design, let's allow the image to stretch
// to the full width of its containing figure, but for the height,
// constrain it with a fixed padding
const containerWidth = context.core.image.figureRef.clientWidth;
// As per the design, let's constrain the height with fixed padding
const containerOuterHeight = window.innerHeight;
const verticalPadding = 40;
const containerInnerHeight = containerOuterHeight - verticalPadding * 2;

// The lightbox image has `positione:absolute` and
// ignores its parent's padding, so let's set the padding here,
// to be used when calculating the image width and positioning
// Let's set a variable horizontal padding based on the container width
const containerOuterWidth = window.innerWidth;
let horizontalPadding = 0;
if ( containerWidth > 480 ) {
if ( containerOuterWidth > 480 ) {
horizontalPadding = 40;
} else if ( containerWidth > 1920 ) {
} else if ( containerOuterWidth > 1920 ) {
horizontalPadding = 80;
}

const containerHeight =
context.core.image.figureRef.clientHeight - verticalPadding * 2;
const containerInnerWidth = containerOuterWidth - horizontalPadding * 2;

// Check difference between the image and figure dimensions
const widthOverflow = Math.abs(
Math.min( containerWidth - targetWidth, 0 )
Math.min( containerInnerWidth - targetWidth, 0 )
);
const heightOverflow = Math.abs(
Math.min( containerHeight - targetHeight, 0 )
Math.min( containerInnerHeight - targetHeight, 0 )
);

// If image is larger than its container any dimension, resize along its largest axis.
// For vertically oriented devices, always maximize the width.
// If the image is larger than the container, let's resize
// it along the greater axis relative to the container
if ( widthOverflow > 0 || heightOverflow > 0 ) {
if (
widthOverflow >= heightOverflow ||
containerHeight >= containerWidth
) {
targetWidth = containerWidth - horizontalPadding * 2;
const containerInnerAspectRatio =
containerInnerWidth / containerInnerHeight;
const imageAspectRatio = targetWidth / targetHeight;

if ( imageAspectRatio > containerInnerAspectRatio ) {
targetWidth = containerInnerWidth;
targetHeight =
imgDom.naturalHeight * ( targetWidth / imgDom.naturalWidth );
( targetWidth * imgDom.naturalHeight ) / imgDom.naturalWidth;
} else {
targetHeight = containerHeight;
targetHeight = containerInnerHeight;
targetWidth =
imgDom.naturalWidth * ( targetHeight / imgDom.naturalHeight );
( targetHeight * imgDom.naturalWidth ) / imgDom.naturalHeight;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could have imgDom.naturalWidth, imgDom.naturalHeight, containerInnerHeight or targetHeight 0 as value?

If they are 0 or NaN in any moment, they will cause a warning/error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If they are 0 or NaN in any moment, they will cause a warning/error.

Thanks for bringing this up! The only way I could see this happening is if a user inserts a broken external image with an incorrect URL, in which case other parts of WordPress also break. If an image is not fully loaded, it will return 0 for the naturalWidth and naturalHeight, but since we wait until the image is fully loaded to activate this part of the code, I don't believe that's currently an issue.

We can revisit this if any issues do arise in the future or if we revise the functionality, but for the moment I don't think handling this should be a blocker for this PR 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens then if the user inserts a working external image, and then, suppose in couple of months, the external site closes and all the images are gone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double checked this and we get some PHP errors due to getimagesize() that can actually likely be circumvented. I've captured the getimagesize() in an issue and the implementation is going to change soon, so I can handle this issue then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I opened a pull request that fixes this 👍

}
}

Expand All @@ -261,16 +262,16 @@ function setZoomStyles( imgDom, context, event ) {

// Get values used to center the image
let targetLeft = 0;
if ( targetWidth >= containerWidth ) {
if ( targetWidth >= containerInnerWidth ) {
targetLeft = horizontalPadding;
} else {
targetLeft = ( containerWidth - targetWidth ) / 2;
targetLeft = ( containerOuterWidth - targetWidth ) / 2;
}
let targetTop = 0;
if ( targetHeight >= containerHeight ) {
if ( targetHeight >= containerInnerHeight ) {
targetTop = verticalPadding;
} else {
targetTop = ( containerHeight - targetHeight ) / 2 + verticalPadding;
targetTop = ( containerOuterHeight - targetHeight ) / 2;
}

const root = document.documentElement;
Expand Down
16 changes: 15 additions & 1 deletion test/e2e/specs/editor/blocks/image.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -853,7 +853,10 @@ test.describe( 'Image - interactivity', () => {

await page.getByRole( 'button', { name: 'Enlarge image' } ).click();

await expect( responsiveImage ).toHaveAttribute( 'src', '' );
await expect( responsiveImage ).toHaveAttribute(
'src',
new RegExp( filename )
);
await expect( enlargedImage ).toHaveAttribute(
'src',
new RegExp( filename )
Expand All @@ -866,6 +869,12 @@ test.describe( 'Image - interactivity', () => {
} );
await closeButton.click();

await expect( responsiveImage ).toHaveAttribute( 'src', '' );
await expect( enlargedImage ).toHaveAttribute(
'src',
new RegExp( filename )
);

await expect( lightbox ).toBeHidden();
} );

Expand Down Expand Up @@ -1093,6 +1102,11 @@ test.describe( 'Image - interactivity', () => {

await page.getByRole( 'button', { name: 'Enlarge image' } ).click();

await expect( responsiveImage ).toHaveAttribute( 'src', imgUrl );
await expect( enlargedImage ).toHaveAttribute( 'src', imgUrl );

await page.getByRole( 'button', { name: 'Close' } ).click();

await expect( responsiveImage ).toHaveAttribute( 'src', '' );
await expect( enlargedImage ).toHaveAttribute( 'src', imgUrl );
} );
Expand Down