Image block: Add animation toggle to lightbox behavior #51357
Conversation
|
A few outstanding items I'm still working on:
|
|
@jasmussen I'd appreciate your feedback on the design for the UI! I know this is still an experiment so not sure how much we should think through it, but currently the UI looks like this, with the Animation dropdown being activated when 
|
|
Size Change: +1.5 kB (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
artemiomorales
added
[Type] Enhancement
|
Nice, thanks for the ping. Just looking at the box here:
|
|
Flaky tests detected in 7fc4425. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5279095792
|
|
I tested the working implementation on an iPhone XS running iOS 16.0 and unfortunately the animation is shaky; the issue is present in Chrome, Safari, and Firefox. iphone-xs-zoom.movIt's also present but less severe on iPad Pro running iPadOS 15.6.1 ipad-zoom.mp4None of the shakiness is present on desktop at responsive sizes. Am currently looking at identifying the cause and possible solutions. As I go down this rabbit hole, I want to check my assumptions here — should we even have the zoom animation on mobile? I wonder if we have the same kind of benefit in this scenario that we do on desktop where...
I also wonder if there would be accessibility considerations to consider with pinch to zoom or the lightbox behavior on mobile in general. Pinging @jasmussen @SaxonF @jameskoster @andrewhayward for guidance. |
@jasmussen Great, thanks! These changes have been implemented.
This was part of a separate PR by @c4rl0sbr4v0 to remove the custom attributes from the block markup, which from what I understand can't be easily accomplished using the dropdowns, though I'm not sure that this is apparent to end users. Is it worth exploring this further for the moment or should we go with what we have for now and await feedback? |
It's been a while since I was deep in CSS and animation, but you could look at
Good question. The image that gets zoomed, is that always the same fidelity version that is used in the resting state? I.e. is it a scaled down version, or does a larger fidelity version get loaded? If the former, it'd be good to separtely look at a way to lazy-load and replace the low fidelity version with a higher fidelity version when zoomed. And to answer your question, yes I still think it can be valuable to have the lightbox on mobile. Not just to ensure you get the highest fidelity version, but to ensure you get it centered on the screen and with the viewport locked to prevent scrolling (I know that last part is harder on Mobile Safari, but it's something to strive for still). In that situation, you can more comfortably pinch to zoom and pan on a high res lightbox-isolated image.
I think we can be smarter with the padding. Since the image is always vertically centered, we can probably use a 
The top and bottom padding we can keep fixed, e.g. at 80px always, as usually phones are taller aspect ratio than portraits: 
Just to be sure I'm reading this right: both the "Reset" button, and selecting the default value (None) in the top dropdown, remove the attributes from the markup? In that case I'd remove the reset button. As noted, it is not a pattern we are using widely, lest we end up with reset buttons everywhere. Further, it's always important we start with less and add only when it's clear it is necessary. Because if it's not necessary, omitting it will provide the clearest interface, and if it is necessary, that will become clear quickly and easy to follow up on. On the flipside, if we start with more, then it won't be clear if it's needed at all. |
Hi @jasmussen ! In this case, those are different options, as 'None' could not be the default value. The default option can be set by the That's the reason for having a Adding or removing the button is not a really big change, and we are happy to listen for feedback and adapt its behavior to a better approach, maybe we could add a |
Yes, I'd think a "Default" you can set it back to is a great alternative. |
I did a proof of concept and I still don't like the user experience, as you choose an option and instantly goes to the default one. I'd like to see how this would work in a future with more than one behavior. So we could just use this approach at the moment and wait for more design feedback. Screen.Recording.2023-06-13.at.16.37.22.mov |
|
Thank you for the video and clarification. I recognize the matrix:
The reason why it's important we don't add a "clear" button here, is because this challenge is identical to every other style inherited from global styles, and we need to seek out a generic solution that works across all. If we make local solutions we end up encumbering the UI. The issue where we're discussing it is here: #43082, and a solution to that would also be a solution here. So what you can do is have 3 options:
I realize that #51464 almost does the above. But from what I can tell, when you choose default, it automatically selects the "Lightbox" option. Presumably this is smarts that shows that option to indicate it is inherited. I wouldn't do that last part — just let the control rest on "Default", we do the same for the Font family: 
As noted, the larger issue is one we need to solve in a universal way across all design tools. But the important part for now is that we don't add too many local behaviors to these controls — they should all feel related and behave similarly. What do you think? |
Ok got it @jasmussen. I've pushed up some changes fixing the mobile animation on iOS. I don't have a physical Android device to test with though — while I'll look around for services so I can do that remotely, I'd greatly appreciate if some Android users could test this on their own devices 😄 I've also incorporated your padding suggestion.
We load a high-fidelity version when the image gets clicked. I've also added the styles for |
I updated the code, now it stays at defaults if the option is to inherit (attribute removed from markup) |
|
All sounds great. I run Android and can test this and hopefully give a green check. I can't test today, but can test in my morning. |
artemiomorales
force-pushed
the
update/lightbox-zoom-animation
branch
from
45dff3c to
7ea0889
Compare
artemiomorales
changed the base branch from
trunk
to
update/move-reset-button-to-default-option
Ok perfect, I rebased on #51464 and set it as the target branch for now so we can clearly see the diff. How does this implementation for the UI look, @c4rl0sbr4v0? I wasn't sure whether to create a separate component or implement the settings for the animation the way I did, with a separate
@jasmussen Great. thanks! Note that I think I've set up the logic properly to make sure the image doesn't stretch incorrectly in horizontal or vertical orientation, though will go over it again tomorrow with fresh eyes to make sure and see if the code around that can also be clearer. |
cbravobernal
force-pushed
the
update/move-reset-button-to-default-option
branch
from
8db1d0d to
286541f
Compare
cbravobernal
requested review from
spacedmonkey,
kevin940726 and
Mamaduka
as code owners
Because the image in the lightbox has absolute positioning and does not respect the padding of its parent container, we need to get references to both the parent <figure> element and the <image> element to set the right scale and smoothly animate the zoom. To accomplish that, since we don't have access to the img src or its natural width and height until it actually appears in the DOM, I needed to hoist the imgSrc up to the parent context to allow for retrieval of the target dimensions from an <img> element created on the fly.
The previous method of centering the image was peforming poorly on mobile. By doing more manual calculation, the animation now performs much better.
The 'has-lightbox-open' class was previously causing the content to shift before the image animation occurred and looked like a mistake. I've now moved the declaration so that the class is added during the animation so it draws less attention.
Mostly moved code around, renamed variables for clarity, and add comments. Fixed a bug wherein the lightbox wouldn't close on scroll when using a fade animation.
Removed stylesheet padding declarations for the lightbox and cleaned up logic to ensure correct dimensions get set for vertical images on mobile devices.
cbravobernal
force-pushed
the
update/lightbox-zoom-animation
branch
from
0f36f61 to
f557f74
Compare
|
Tested and worked as expected! |
|
I'm fixing the tests right now 😄 |
| @keyframes lightbox-zoom-out { | ||
| 0% { | ||
| visibility: visible; | ||
| left: var(--lightbox-target-left-position); |
There was a problem hiding this comment.
👋 Just dropping by and skimming through the code, and noticed that you may be animating the position of the lightbox via top and left CSS properties — have you considered using transform (or the new translate prop) for more performant animations?
There was a problem hiding this comment.
Great, thanks for the suggestion! Will take a look
| @@ -162,8 +130,8 @@ test.describe( 'Testing behaviors functionality', () => { | |||
| // attributes takes precedence over the theme's value. | |||
| await expect( select ).toHaveValue( 'lightbox' ); | |||
|
|
|||
| // There should be 2 options available: `No behaviors` and `Lightbox`. | |||
| await expect( select.getByRole( 'option' ) ).toHaveCount( 2 ); | |||
| // There should be 3 options available: `No behaviors` and `Lightbox`. | |||
There was a problem hiding this comment.
We may want to update this comment to explain what is the third option that we're expecting to find?
* Remove reset button in behaviors, use dropdown option instead * Use default option reading from the theme, prevent autoupdate * Add zoom animation UI and styles * Add logic to use original image and scale its dimensions Because the image in the lightbox has absolute positioning and does not respect the padding of its parent container, we need to get references to both the parent <figure> element and the <image> element to set the right scale and smoothly animate the zoom. To accomplish that, since we don't have access to the img src or its natural width and height until it actually appears in the DOM, I needed to hoist the imgSrc up to the parent context to allow for retrieval of the target dimensions from an <img> element created on the fly. * Remove extraneous help text * Manually center lightbox image to improve animation performance The previous method of centering the image was peforming poorly on mobile. By doing more manual calculation, the animation now performs much better. * Move and reenable class declaration for overflow The 'has-lightbox-open' class was previously causing the content to shift before the image animation occurred and looked like a mistake. I've now moved the declaration so that the class is added during the animation so it draws less attention. * Add prefers reduced motion accessibility styles * Modify fade styles to prevent image flashing * Simplify code for lightbox UI * Fix PHP error and linter syntax * Clean up code; fix bug, add comments Mostly moved code around, renamed variables for clarity, and add comments. Fixed a bug wherein the lightbox wouldn't close on scroll when using a fade animation. * Fix bug wherein newly placed images were not setting lightbox animation * Fix bug wherein vertical images were stretched on mobile Removed stylesheet padding declarations for the lightbox and cleaned up logic to ensure correct dimensions get set for vertical images on mobile devices. * Update e2e tests * Update e2e tests, fix selector showing when it should not --------- Co-authored-by: Carlos Bravo <[email protected]>
What?
This adds a dropdown allowing users to select between the zoom animation or the fade animation for the experimental image lightbox.
Why?
Addresses #51055
We'd like to give users the option to opt for their preferred animation and get feedback on the UX for this functionality.
How?
It adds the option and some conditionals to the behaviors UI in block supports, and also adds the styles needed to make the zoom work.
Testing Instructions
-- A horizontal image
-- A vertical image
zoomset as the default.fadeto ensure that works.Testing Instructions for Keyboard
N/A