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

Jump to bottom

Image block: Lightbox animation improvements #51721

Merged
merged 9 commits into from
Jun 26, 2023
Merged

Image block: Lightbox animation improvements #51721

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1026f82
Add logic to use low-res image while high-res one is loading
artemiomorales Jun 21, 2023
8b4ae20
Add logic to preload image on hover
artemiomorales Jun 21, 2023
135105c
Update tests
artemiomorales Jun 21, 2023
54047cb
PHP format
cbravobernal Jun 22, 2023
b4a3c1d
Prevent responsive images from being loaded unnecessarily
artemiomorales Jun 22, 2023
549e795
Prevent warning of undefined variable
cbravobernal Jun 23, 2023
c0baefa
Prevent warning of undefined variable - refactored
cbravobernal Jun 23, 2023
e713211
Replace getimagesize() with wp_getimagesize()
artemiomorales Jun 23, 2023
7d3b3b5
Resolve image flash when opening lightbox
artemiomorales Jun 23, 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
35 changes: 19 additions & 16 deletions lib/block-supports/behaviors.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,17 +89,17 @@ function gutenberg_render_behaviors_support_lightbox( $block_content, $block ) {
$z->next_tag( 'img' );

if ( isset( $block['attrs']['id'] ) ) {
$img_src = wp_get_attachment_url( $block['attrs']['id'] );
$img_metadata = wp_get_attachment_metadata( $block['attrs']['id'] );
$img_width = $img_metadata['width'];
$img_height = $img_metadata['height'];
$img_srcset = wp_get_attachment_image_srcset( $block['attrs']['id'] );
$img_uploaded_src = wp_get_attachment_url( $block['attrs']['id'] );
$img_metadata = wp_get_attachment_metadata( $block['attrs']['id'] );
$img_width = $img_metadata['width'];
$img_height = $img_metadata['height'];
$img_uploaded_srcset = wp_get_attachment_image_srcset( $block['attrs']['id'] );
} else {
$img_src = $z->get_attribute( 'src' );
$img_dimensions = wp_getimagesize( $img_src );
$img_width = $img_dimensions[0];
$img_height = $img_dimensions[1];
$img_srcset = '';
$img_uploaded_src = $z->get_attribute( 'src' );
$img_dimensions = wp_getimagesize( $img_uploaded_src );
$img_width = $img_dimensions[0];
$img_height = $img_dimensions[1];
$img_uploaded_srcset = '';
}

$w = new WP_HTML_Tag_Processor( $content );
Expand All @@ -111,30 +111,34 @@ function gutenberg_render_behaviors_support_lightbox( $block_content, $block ) {
sprintf(
'{ "core":
{ "image":
{ "initialized": false,
{ "imageLoaded": false,
"initialized": false,
"lightboxEnabled": false,
"hideAnimationEnabled": false,
"preloadInitialized": false,
"lightboxAnimation": "%s",
"imageSrc": "%s",
"imageUploadedSrc": "%s",
"imageCurrentSrc": "",
"imageSrcSet": "%s",
"targetWidth": "%s",
"targetHeight": "%s"
}
}
}',
$lightbox_animation,
$img_src,
$img_srcset,
$img_uploaded_src,
$img_uploaded_srcset,
$img_width,
$img_height
)
);
$w->next_tag( 'img' );
$w->set_attribute( 'data-wp-effect', 'effects.core.image.setCurrentSrc' );
$body_content = $w->get_updated_html();

// Wrap the image in the body content with a button.
$img = null;
preg_match( '/<img[^>]+>/', $content, $img );
preg_match( '/<img[^>]+>/', $body_content, $img );
$button = '<div class="img-container">
<button type="button" aria-haspopup="dialog" aria-label="' . esc_attr( $aria_label ) . '" data-wp-on--click="actions.core.image.showLightbox" data-wp-effect="effects.core.image.preloadLightboxImage"></button>'
. $img[0] .
Expand All @@ -148,7 +152,6 @@ function gutenberg_render_behaviors_support_lightbox( $block_content, $block ) {
$m->next_tag( 'img' );
$m->set_attribute( 'src', '' );
$m->set_attribute( 'data-wp-bind--src', 'selectors.core.image.responsiveImgSrc' );
$m->set_attribute( 'data-wp-bind--srcset', 'selectors.core.image.responsiveImgSrcSet' );
$initial_image_content = $m->get_updated_html();

$q = new WP_HTML_Tag_Processor( $content );
Expand Down
45 changes: 26 additions & 19 deletions packages/block-library/src/image/interactivity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ store( {
core: {
image: {
showLightbox: ( { context, event } ) => {
// We can't initialize the lightbox until the reference
// image is loaded, otherwise the UX is broken.
if ( ! context.core.image.imageLoaded ) {
return;
}
Comment on lines +25 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

I hope time to interactive does not being affected here too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on the user's bandwidth. But also, I think it's reasonable to prevent a user from zooming in on an image that hasn't fully loaded yet? 🤔

Another note: We can likely make the time to interactivity instant with the fade animation; it's just the zoom animation that causes the trouble.

If it's worth exploring, I could make this initialization step specific to the zoom animation.

Another note: Is the concern here that users may want to go immediately to the zoomed image without needing to wait for the rest of the page to load? @mtias has mentioned before the idea of having a separate URL for the zoomed version of the image inside of the lightbox as an overall usability improvement for WordPress sites.

Perhaps we can continue to explore this in a subsequent PR where we start to identify how this would behave, (what should the URL be, how to implement the experience of initializing the page with the lightbox already open, crossover or not with the Media page, etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends on the user's bandwidth. But also, I think it's reasonable to prevent a user from zooming in on an image that hasn't fully loaded yet? 🤔

It is indeed. But, for example, IGDB on their Lightbox, shows a loading text if the image is not fully loaded. Still, is something that we can explore in future PRs.

Is the concern here that users may want to go immediately to the zoomed image without needing to wait for the rest of the page to load?

Yes, this could happen. Usually, clicking on an image and wait for the zoom to appear is not ideal.

We can wait and see how that ticket is evolving.

context.core.image.initialized = true;
context.core.image.lastFocusedElement =
window.document.activeElement;
Expand Down Expand Up @@ -50,7 +55,10 @@ store( {
imgDom.onload = function () {
context.core.image.activateLargeImage = true;
};
imgDom.setAttribute( 'src', context.core.image.imageSrc );
imgDom.setAttribute(
'src',
context.core.image.imageUploadedSrc
);
},
hideLightbox: async ( { context, event } ) => {
context.core.image.hideAnimationEnabled = true;
Expand Down Expand Up @@ -135,26 +143,13 @@ store( {
return context.core.image.lightboxEnabled ? 'dialog' : '';
},
responsiveImgSrc: ( { context } ) => {
if (
! context.core.image.initialized ||
context.core.image.activateLargeImage
) {
return '';
}
return context.core.image.imageSrc;
},
responsiveImgSrcSet: ( { context } ) => {
if (
! context.core.image.initialized ||
context.core.image.activateLargeImage
) {
return '';
}
return context.core.image.imageSrcSet;
return context.core.image.activateLargeImage
? ''
: context.core.image.imageCurrentSrc;
},
enlargedImgSrc: ( { context } ) => {
return context.core.image.initialized
? context.core.image.imageSrc
? context.core.image.imageUploadedSrc
: '';
},
},
Expand All @@ -163,14 +158,26 @@ store( {
effects: {
core: {
image: {
setCurrentSrc: ( { context, ref } ) => {
if ( ref.complete ) {
context.core.image.imageLoaded = true;
context.core.image.imageCurrentSrc = ref.currentSrc;
} else {
ref.addEventListener( 'load', function () {
context.core.image.imageLoaded = true;
context.core.image.imageCurrentSrc =
this.currentSrc;
} );
}
},
preloadLightboxImage: ( { context, ref } ) => {
ref.addEventListener( 'mouseover', () => {
if ( ! context.core.image.preloadInitialized ) {
context.core.image.preloadInitialized = true;
const imgDom = document.createElement( 'img' );
imgDom.setAttribute(
'src',
context.core.image.imageSrc
context.core.image.imageUploadedSrc
);
}
} );
Expand Down
10 changes: 8 additions & 2 deletions 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 @@ -990,7 +990,10 @@ test.describe( 'Image - interactivity', () => {
const responsiveImage = lightbox.locator( '.responsive-image img' );
const enlargedImage = lightbox.locator( '.enlarged-image img' );

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

await page.getByRole( 'button', { name: 'Enlarge image' } ).click();
Expand Down Expand Up @@ -1182,7 +1185,10 @@ test.describe( 'Image - interactivity', () => {
const responsiveImage = lightbox.locator( '.responsive-image img' );
const enlargedImage = lightbox.locator( '.enlarged-image img' );

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

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