Image block: Add aspect ratio support to lightbox #52765
Conversation
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/block-supports/behaviors.php |
|
I can't think of any automated tests to go with this one, please just let me know if any make sense! |
|
Size Change: +429 B (0%) Total Size: 1.5 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in ec5f76a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5732529469
|
artemiomorales
force-pushed
the
update/support-aspect-ratio-in-lightbox
branch
from
1aed7b5 to
a3014ac
Compare
|
As we worked together on this one, I'd prefer if @michalczaplinski or @c4rl0sbr4v0 could take a look at the code for the review 🙂 |
|
The image flickers on Safari browser 😿 weird_movement.mov |
@c4rl0sbr4v0 Thanks for catching this! The latest push fixes this jitteriness in Safari, as well as cleans up the animation code and addresses the use of a regular However, while working on all of that, I noticed a couple of other cases we still haven't accounted for:
In addition, something in my code is causing the PHP linter to fire an error. Will continue hammering at these issues. |
|
I guess the different devices interpret the CSS animations differently, right? It already happened that, using |
Is this the issue that you mention @artemiomorales ? Then, I can reproduce it too: Screen.Recording.2023-07-24.at.15.01.16.mov |
There was a problem hiding this comment.
I think I'm not too familiar with this part of the code to be able to review the code changes properly. A short video to explain what's going on could be useful.
Otherwise, I've tested the PR and it works as expected (sans the issue with thumbnails that I mention above)! 👍
@michalczaplinski Yup, that's the one!
@SantosGuillamot So the issue with the thumbnails is that, when one selects So as far as I see, to get this to work, we need to somehow calculate CSS values for the the full-sized image that would create the initial starting conditions of the thumbnail image while having completely different dimensions. There's probably a way to do it — am still thinking through it. There may also be a way to handle the UX differently, but this seems to me to the approach that users would expect. Am happy to hear any ideas or suggestions! Though this case seems pretty specific for our implementation and constraints, I'll see about investing some time into looking at additional zoom implementations too. |
|
EDIT: Updated to specify that that 16x9 aspect ratio problem is in the special case where a image has set to thumbnail resolution with a custom aspect ratio of 16x9. The latest push seems to handle most cases, though it doesn't play nicely when a thumbnail has been defined with a 16x9 aspect ratio; I also need to add support for the For the moment, it checks the aspect ratio of the image's natural dimensions (for thumbnails, this is 150x150), and compares it to the aspect ratio of the target dimensions (this is the full-sized image). If these aspect ratios differ, then we calculate the space the lightbox image would use as a cover image and apply that as a The logic isn't quite there yet and needs to be cleaned up, but would appreciate any suggestions and thoughts! |
| @media screen and (min-width: 480px) { | ||
| padding: 40px; | ||
| } | ||
|
|
||
| @media screen and (min-width: 1920px) { | ||
| padding: 40px 80px; | ||
| } |
There was a problem hiding this comment.
Margins are paddings are usually defined in styles, can we move this to CSS vars calculations?
|
I've been testing it, and the 16x9 aspect ratio seems to work for me. What issue are you having? Apart from that, I guess that for the contain setting we could create something like this? let originalWidth = offsetWidth;
let originalHeight = offsetHeight;
let originalLeftPosition = screenPosX;
let originalTopPosition = screenPosY;
// If the image has "object-fit: contain", calculate the dimensions without the extra space.
if (event.target.nextElementSibling.style.objectFit === "contain") {
// If the image has blank space in the height, keep the width and calculate the height based on the natural ratio.
if (naturalRatio > offsetRatio) {
originalHeight = offsetWidth / naturalRatio;
const extraHeight = offsetHeight - originalHeight;
originalTopPosition = originalTopPosition + extraHeight / 2;
} else {
// If the image has blank space in the width, keep the height and calculate the width based on the natural ratio.
originalWidth = offsetHeight * naturalRatio;
const extraWidth = offsetWidth - originalWidth;
originalLeftPosition = originalLeftPosition + extraWidth / 2;
}
}On another note, I would like to review the whole zooming implementation deeper. I feel that for each edge case we found (like the Android issue, Safari issue, the aspect ratio, the contain functionality, etc) we are adding more JavaScript to handle each case, and the logic is becoming quite complex. I want to fully understand why those issues are happening, the reasoning behind the JS logic, and ensure it cannot be simplified and moved part of it to CSS. |
|
I merged in changes by @SantosGuillamot refactoring the implementation. It's looking much better! I tested on a couple of mobile devices and it largely functions as expected. We do still have a few scenarios to cover: 1.) This portrait image with thumbnail-cover-9x16.mp42.) Thumbnails with thumbnail-contain.mp43.) In general, we need to fix the inconsistent behavior with 4.) We need to add the padding back in. 5.) The responsive image beneath the large image often shows as a broken image link. |
|
Thanks for testing and raising the issues! I didn't consider that an image could be cropped both in width and height. I just made a commit to fix that use case (I believe). Lightbox.vertical.lightbox.-.28.July.2023.mp4It'd be great if you can test it as it depends on the image resolution. I'll take a look at the issue with contain. |
|
I added support for the contain setting in the latest commit. I've tested it in multiple scenarios, and it seems to work. But it'd be great if you could test it with different images. Lightbox.vertical.lightbox.-.28.July.2023.1.mp4 |
SantosGuillamot
force-pushed
the
update/support-aspect-ratio-in-lightbox
branch
from
768aa10 to
838d6a1
Compare
|
I rebased trunk and I added a couple of commits to clean the code and remove the On another note, I just realized that the fade in animation is not working the same way for the different uses cases. I believe we can reuse the zooming logic. I'll try that. |
|
I added a commit to reuse the zooming logic for the fade animation. It seems to solve all the problems the fade animation had with the different aspect ratio, contain, and thumbnails. However, I'm not 100% sure why there was a logic specific to the zoom, so I may be missing something. |
As part of this commit, I added handling for button styles in all images with the lightbox enabled, as we can't rely on the CSS as was written to set the right dimensions without breaking the layout on mobile.
The combination of 'loading=lazy' and the 'hidden' attributes was breaking progressive image loading in Safari and Chrome; instead we are now taking what seems to be a more reliable approach to accomplish lazy loading of the enlarged image, namely setting the src manually when the lightbox is opened. *This was an approach we had implemented before, but we began using 'loading=lazy' during a code cleanup.
artemiomorales
force-pushed
the
update/support-aspect-ratio-in-lightbox
branch
from
ad63011 to
0a04a6c
Compare
| windowWidth: window.innerWidth, | ||
| windowHeight: window.innerHeight, |
There was a problem hiding this comment.
I don't know if we should use namespaces here as well. It's true that it could be used by any other block, but it could also create conflicts if other blocks declare the same variables.
There was a problem hiding this comment.
@SantosGuillamot Good point — I pushed a commit adding a namespace.
What?
This PR makes it so the lightbox zoom animation functions as expected when configuring a custom aspect ratio on an image.
Why?
Addresses #52192
We want the lightbox to be compatible with all of the image block's features.
How?
It calculates the aspect ratio of the resized image and uses it to calculate the target zoom dimensions.
Testing Instructions
aspect ratioin block settingszoomanimation is selected.If possible, please also test on a mobile device.
Testing Instructions for Keyboard
N/A
Screenshots or screencast
Before
aspect-ratio-before.mp4
After
aspect-ratio-after.mp4