Gallery: Add lightbox support #62906
Conversation
|
Size Change: +2.02 kB (+0.11%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
madhusudhand
force-pushed
the
gallery/lightbox
branch
3 times, most recently
from
5af6a6b
to
4fdbd86
Compare
|
@jasmussen @WordPress/gutenberg-design This is a first iteration of #56310.
The same design doesn't work for smaller devices as the image fits to 100% width. 
While icons can be avoided in touch based devices, that may not be accessible. |
t-hamano
added
[Type] Enhancement
|
This PR should close #37652. |
|
The chevrons look a bit awkward when they get this close to the image: 
Ideally there should be no overlap. When viewing the first or last image in the lightbox, the next / previous button is disabled accordingly, but there's no visual indication which makes the button feel broken. Two options to explore:
I'm not sure the chevrons need to appear on mobile, swiping left/right is probably more intuitive? |
|
Great! Thank you for working on this @madhusudhand Taking a step back. Doing some brain storming. The Gallery needs a Link To option - Expand on click so that all the images inside a gallery will use Expand on click.
Add a toggle to add left/right pagination for the Expand option. So it would be something like this. Thanks! |
|
In #62762 we are moving the Link to option from the block sidebar to the block toolbar. If we make this change, will it conflict with this PR? |
|
I think we need to consider various scenarios. Here are some examples:
|
madhusudhand
force-pushed
the
gallery/lightbox
branch
from
4fdbd86
to
a8ca784
Compare
madhusudhand
force-pushed
the
gallery/lightbox
branch
from
a8ca784
to
bd0700e
Compare
As per the current changes, lightbox invoked by clicking on any image except 3, and next/prev navigation moves the lightbox images between 1,2,4,5 without exiting after image 2 (on next). I would imagine this would be better experience have lightbox as single set [1,2,4,5], instead of breaking into two sets [1,2] and [3,4]. Please suggest.
@t-hamano This PR now only based on the image having "Expand to click" enabled. I don't think it will conflict. The behaviour should be same as long as the image gets the value for lightbox.
This will be
They will be skipped and it is the right thing to do, because clicking on those images directly doesn't invoke lightbox.
This is not relevant for this iteration. But #62762 makes it possible, when the setting is enabled at a gallery block level. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Images are given a better padding to avoid overlaps with the icon.
Yes. The current change disables the buttons. Suggest on opacity.
They are removed on mobile devices, meaning the current PR has no effect on mobile devices and swipe actions will be added in a followup PR. |
|
Thanks for the detailed explanation. Since it's based only on whether the Gallery block has images with lightbox enabled, does that mean there's no way to disable navigation between images? Do we need to provide some way to disable (or enable) navigation between images? |
|
The Link to option in the Gallery block is just a global change to the With that in mind, I think there might need to be an option to explicitly enable the Gallery lightbox. For example, the UI would look like this:
However, if we move the block sidebar's "Link to" control to the block toolbar in #62762, the question is whether to leave the "Enable lightbox Gallery" toggle. |
To be honest, I don't see many cases where people would want to disable image navigation. It just bothers me that there's no way to opt out of it 😅 |
I think ideally these are |
There was a problem hiding this comment.
Code-wise, it looks good to me.
Switching from currentImageId to an images array and currentImageIndex makes sense.
I'm not entirely sure if the implementation should mention the gallery (state.isGallery) or if simply the previous and next image buttons should appear when the state.images array contains more than one image. But that's something that can be changed in the future when other use-cases appear, like navigating across all the exapandable images of a post.
|
A couple of things. A new PR should be added to add Expand on click option to the LINK To drop down. @akasunil @madhusudhand Then this PR: Add lightbox support. This means the process is so: |
|
Thanks for working on this @madhusudhand! I've taken an initial look and it works 🙌
Regarding this point and other discussion I've been reading, I posted a comment that can hopefully offer some points to consider and help with alignment. |
Lets say I am not sure if |
@artemiomorales Thanks for sharing a great summary. As mentioned here #56310 (comment) I believe, the new option can be safely added in a followup PR and it doesn't block the current change. |
There was a problem hiding this comment.
Excellent work on adding the lightbox support for individual Gallery blocks. I did a round of testing and I can confirm that the navigation between images is limited to the currently active Gallery. It also correctly ignores the images that opted out from expand on click individually. That also covers the case when only one image in the Gallery is set or only one opted in into the expand of click functionality.
We will need to verify the keyboard navigation before landing this PR, as I'm afraid it isn't fully accessible at the moment. Namely, the focus in the lightbox is always set to the close button and it is impossible to tab to the arrow buttons. Arrow left and right keys work correctly as for changing images, but there is no information announced when using assistive technologies how many items there are to browse in the lightbox, and the information announced when reaching the first and last item might be confusing.
Lightbox.navigation.with.keyboard.mov
There should always be ways to opt out. There should be an option on the Gallery level that disables the lightbox experience. The only question is how that impacts individual images. Would it mean it automatically disables the ligthbox also for individual images, or users could still enlarge individual images but not navigate between them. |
This commit 59b06b3 brings the focus to all elements and makes the other document elements
The above fix brings the focus and announces |
| @@ -307,6 +310,12 @@ class="wp-lightbox-overlay zoom" | |||
| <button type="button" aria-label="$close_button_label" style="fill: $close_button_color" class="close-button"> | |||
| <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="20" height="20" aria-hidden="true" focusable="false"><path d="m13.06 12 6.47-6.47-1.06-1.06L12 10.94 5.53 4.47 4.47 5.53 10.94 12l-6.47 6.47 1.06 1.06L12 13.06l6.47 6.47 1.06-1.06L13.06 12Z"></path></svg> | |||
| </button> | |||
| <button type="button" aria-label="$prev_button_label" style="fill: $close_button_color" class="prev-button" data-wp-bind--hidden="!state.hasNavigation" data-wp-on--click="actions.showPreviousImage" data-wp-bind--disabled="!state.hasPreviousImage"> | |||
There was a problem hiding this comment.
It needs to be further validate, but it's likely that we will have to set aria-disabled instead of disabled so the button doesn't loose focus when there are no more items to navigate to with the keyboard interactions using space or enter.
There was a problem hiding this comment.
Yes, I confirmed that in my testing. As soon as button gets into disabled state the focus moves to the body element. In general, tabable elements should always be in the same order so it is predictable. Inactive buttons should be present but announce their disabled state.
I'm also not entirely sure how it all should work with arrow right and left when focus is inside the lightbox on the close button or arrow keys. I would appreciate some hepl from accessibility experts here.
Nice, I can confirm that I can now |
@afercia, I wonder if we should take this opportunity to add the Can you confirm that adding |
|
Is there something preventing styles from loading?
|
What?
This introduces lightbox functionality to
Galleryblock.How?
Following is the behaviour in various scenarios.
Case 1: "Expand on click" is enabled for the image block globally.
All images inside (unless overridden at block level), will invoke the lightbox on click and next and previous navigation is possible.
Case 2: One of the image (lets say 2nd image) is set to "Link to image file"
Lightbox will be invoked on click of 1st image and on click of next will move to 3rd image.
Case 3: "Expand on click" is not enabled globally, but few images are set with this option in gallery.
This is similar to case 2, lightbox opens with only those images.
What's in future PRs?
Screenshots (Draft)
Testing instructions
Related issues
First iteration of #56310