Image block: Lightbox animation improvements #51721
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I still need to do more testing to make sure I didn't inadvertently break the fade animation, as well as add the preloading of the image on hover. While I'm at it, I'd also like to add a slight zoom on hover to improve the UX. Lastly, I've spotted a couple of issues at different responsive sizes where the image gets distorted, so I'd like to address those, though perhaps that can be a separate PR. |
|
Size Change: +13.5 kB (+1%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
I decided it makes more sense to handle these as part of a separate PR for easier review. With that being said, I believe this PR is ready for feedback. |
artemiomorales
added
[Block] Image
|
I did a quick PHP format. Let me know if you need any help setting auto-format in your environment. |
|
Flaky tests detected in be83100c210855ea113c9d61d7b465d3e4ed525f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5351019861
|
cbravobernal
force-pushed
the
fix/lightbox-delayed-response
branch
from
9df81fd
to
848be79
Compare
artemiomorales
commented
Jun 22, 2023
|
@c4rl0sbr4v0 I just pushed up a change that should prevent responsive images from getting loaded unnecessarily 👍 |
We need to have two <img> elements in the DOM inside the lightbox because otherwise the image flickers when we change the src. I removed the src and srcset attributes from the responsive image when the larger one is loaded to signal that the low-res one is no longer in use.
The src attribute on the img elements was causing the srcset attribute to be added as well before the Interactivity API could remove them, causing images to be loaded before they were needed and in the wrong size. This commit removes the src attribute from the img elements before they are output to the DOM, and also updates the tests.
cbravobernal
force-pushed
the
fix/lightbox-delayed-response
branch
from
be83100
to
b4a3c1d
Compare
|
Updated with trunk to see if we solve the e2e testing issues. |
cbravobernal
approved these changes
Jun 23, 2023
cbravobernal
requested changes
Jun 23, 2023
Rather than allowing the browser to pick the lightbox's responsive image, we now explicitly read the currentSrc attribute from the reference image and also prevent users from opening the lightbox until the reference image is fully loaded. Doing this, we can ensure the zoom animation plays smoothly and there are no oddities in the UX.
|
@c4rl0sbr4v0 I just updated the logic to hopefully finally resolve the image flickering or appearing broken when opening the lightbox on production environments. Would you be able to give it a once-over to make sure it's still working as expected? (To simulate the latency, you could try testing with Chrome Dev Tools throttling enabled.) If all looks good, I think we can merge. Let me know what you think! |
Comment on lines
+25
to
+29
| // We can't initialize the lightbox until the reference | ||
| // image is loaded, otherwise the UX is broken. | ||
| if ( ! context.core.image.imageLoaded ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I hope time to interactive does not being affected here too much.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
cbravobernal
approved these changes
Jun 26, 2023
This was referenced Jun 26, 2023
luisherranz
reviewed
Jun 27, 2023
Comment on lines
+173
to
+174
| preloadLightboxImage: ( { context, ref } ) => { | ||
| ref.addEventListener( 'mouseover', () => { |
There was a problem hiding this comment.
Hey @artemiomorales, out of curiosity, why did you use useEffect instead of data-wp-on--mouseover?
There was a problem hiding this comment.
Thanks for catching this! It's an oversight on my part. Will revise in this upcoming PR.
There was a problem hiding this comment.
Oh, no problem. Thanks, Artemio.
There was a problem hiding this comment.
@luisherranz Update: I decided to break this out into a separate PR for easier review and approval.
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
* Add logic to use low-res image while high-res one is loading We need to have two <img> elements in the DOM inside the lightbox because otherwise the image flickers when we change the src. I removed the src and srcset attributes from the responsive image when the larger one is loaded to signal that the low-res one is no longer in use. * Add logic to preload image on hover * Update tests * PHP format * Prevent responsive images from being loaded unnecessarily The src attribute on the img elements was causing the srcset attribute to be added as well before the Interactivity API could remove them, causing images to be loaded before they were needed and in the wrong size. This commit removes the src attribute from the img elements before they are output to the DOM, and also updates the tests. * Prevent warning of undefined variable * Prevent warning of undefined variable - refactored * Replace getimagesize() with wp_getimagesize() * Resolve image flash when opening lightbox Rather than allowing the browser to pick the lightbox's responsive image, we now explicitly read the currentSrc attribute from the reference image and also prevent users from opening the lightbox until the reference image is fully loaded. Doing this, we can ensure the zoom animation plays smoothly and there are no oddities in the UX. --------- Co-authored-by: Carlos Bravo <carlos.bravo@automattic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR will do the following in order to improve the UX around the lightbox animation:
Why?
Addresses #51555
Currently, there is a delay when the user clicks on an image to activate the lightbox. This will improve that experience.
How?
I've edited the code so that we now have two
imgelements inside of the lightbox. The lower resolution one is a copy of the image inside of the body content; there is also a larger resolution one whosesrcattribute is populated dynamically when when the lightbox is opened and the full-sized image finishes loading.Testing Instructions
*Must be tested on a live site (not a local environment)
Testing Instructions for Keyboard
N/A
Screenshots or screencast
Before
246253904-52ad93a6-0812-4faf-842b-aece256af86a.mp4
After
lightbox-delay-fix-muted.mp4