-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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.
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
Gallery: Add lightbox support #62906
base: trunk
Are you sure you want to change the base?
Conversation
Size Change: +1.32 kB (+0.07%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
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. |
This PR should close #37652. |
Great! Thank you for working on this @madhusudhand Taking a step back. Doing some brain storming. EDIT:
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:
|
4fdbd86
to
a8ca784
Compare
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. |
A couple of things.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Hmm, I'm not seeing this issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR works for me — I can confirm that the tabbing works as expected, except in following case as mentioned above.
Yes, I confirmed that in my testing. As soon as button gets into disabled state the focus moves to the body element.
No additional comments on the code 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In global styles, enable "Click to expand" feature on image block.
Is this the only way to enable the lightbox effect for the Gallery block at the moment? I was initially testing by inserting a Gallery block into a post, and there doesn't seem to be a way to enable it from the post editor. I think this can happen in a follow-up, I just wanted to make sure I wasn't missing anything.
When I enable "expand on click" for the Image block from Global Styles, the lightbox opens for each image but the prev/next icons don't seem to appear for me. The arrow keys work fine:
Screen.Recording.2024-07-31.at.11.57.51.mov
Edit: I've just tested this again after refreshing my local env, and I can't see this issue anymore. The arrow icons are in the correct position 👍 In that case, this is working great for me, pending addressing the accessibility feedback in this comment.
I want to bring up @t-hamano Aki's above comment: #62906 (comment) |
I tested with the NDVA screen reader and there is no notification when switching between images. I think it should say something like Additionally, when reaching the first or last image, screen readers seem to unintentionally read out the document title. I have no experience with what attributes are needed to achieve this, but it may be related to this comment. 7581b0b92d0701eef09eeffc250f332f.mp4 |
Yes. In the first iteration, enabling the lightbox in the image block is the only way. The control is being added to the gallery block in #64014 |
f9f6752
to
91c1930
Compare
I have tried to solve the accessibility issues by referring to the W3C carousel tutorial. It seems that to inform screen readers of the currently displayed item, it is a good idea to use an element with the The screen reader should read the following text:
2461cd96d85937d555c63869beb54992.mp4P.S. When I merged the latest trunk into this PR, the Previous/Next button became invisible for some reason, so I reverted the merge. Maybe some changes have been made to the Interactivity API or the Image block. If we are going to ship this PR, it might be a good idea to merge the latest trunk and test it beforehand. |
? `Image ${ state.currentImageIndex + 1 } of ${ | ||
state.images.length | ||
}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to make front-end text translatable in the current WordPress/Gutenberg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. @madhusudhand is working on it, if I'm not mistaken 🙂
Flaky tests detected in 91c1930. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10414344881
|
What?
This introduces lightbox functionality to
Gallery
block.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