Try using popover API for image lightbox #68726
Draft
+158
−134
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
|
Size Change: +161 B (+0.01%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
Flaky tests detected in 43f3bb7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12939977013
|
Why, because of Kses? |
It looks like
I tried adding it to the |
|
Thanks for clarifying that. Then what if the Aside: I just found that Modern Image Formats prevents that button from ever being visible on hover when Picture Element is enabled. This is because of this CSS rule (source): .wp-lightbox-container img:hover+button {
opacity: 1;
}That rule would not be necessary if the .wp-lightbox-container button.lightbox-trigger:hover > SVG {
opacity: 1;
} |
That might work, but would like to explore other options first to try to make minimal markup changes. That said, can you have an
That's a good catch. Can you report this in a new issue? cc @adamsilverstein |
Yes, that's totally fine: https://g.co/gemini/share/23ec98c2b90d Example with popover: https://codepen.io/westonruter/pen/NPKBdLd
Here you go: WordPress/performance#1805 |
related: WordPress/wordpress-develop#8133 |
|
@felixarntz, is there a tracking issue for these improvements? |
|
No, since they are purely explorations at this point. If something seems worthwhile doing, I would open one. If anything, we could open an issue to encompass these different explorations so that they're linked in one place if that's helpful. |
…roved accessibility out of the box based on popovertarget.
… including the new overlay property to keep elements in top layer until the end of the animation.
…so that it automatically hides lightbox.
|
I made some good progress here and was able to figure out and address the remaining quirks from before. I updated the PR description with all relevant context. All behaviors and animations should now work like they did before, but using a more browser native approach with certain accessibility best practices automatically baked in. |
westonruter
reviewed
Jan 24, 2025
Co-authored-by: Weston Ruter <[email protected]>
| $button = | ||
| $img[0] | ||
| . '<button | ||
| class="lightbox-trigger" |
There was a problem hiding this comment.
maybe worth checking if anything in Gutenberg or other plugins references this class before removing it.
| * If this handler is triggered due to a click on the close button, it should be ignored as that | ||
| * button automatically hides the lightbox via `popovertarget` and `popovertargetaction`. | ||
| */ | ||
| if ( event?.target?.tagName === 'BUTTON' ) { |
There was a problem hiding this comment.
isn't the entire image a button now? does this bit still work/is it still needed?
| @@ -136,10 +164,6 @@ const { state, actions, callbacks } = store( | |||
| const { ref } = getElement(); | |||
| ref.querySelector( 'button' ).focus(); | |||
| } | |||
| // Closes the lightbox when the user presses the escape key. | |||
There was a problem hiding this comment.
very cool that this can be removed!
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.
This PR explores using the
popoverAPI for the image lightbox as well as related animation capabilities, in order to improve accessibility and simplify the implementation.Benefits:
popovertargetattribute used on both the button to open the lightbox and the button to close it (see https://www.w3.org/TR/html-aam-1.0/#att-popovertarget).popoverandpopovertargetattributes, which allows removing some now unnecessary event listeners.tabindex="-1"is now no longer necessary on the lightbox container element as it cannot be focused unless open anyways.imgelement itself with abutton, as the image itself can be clicked to open the lightbox.buttonis now simply a visual indicator that the image can be clicked to open a lightbox, within the actual button.aria-hidden="true"because for screen readers it's not helpful, since the button itself already provides the information about the possible interaction.z-indexhacks (top-level is higher than anyz-indexcan be).::backdroppseudo element that can be styled, which allows removing the semantically pointlessdiv.scrimelement.Escapeautomatically closes the lightbox.transitionCSS property on the[popover]element and its::backdropin combination with specialoverlayproperty and theallow-discretevalue to keep the elements in the DOM after closure until the animation is completed, avoiding the need for the previous workaround with theshow-closing-animationextra class.I have tested and compared this in depth with the original behavior, both with and without the (hard-coded)
zoomclass on the lightbox container (which based on its presence will lead to different animation behavior). From what I can tell, everything is working as expected. Of course further testing is much appreciated.Learn more:
Previous related experiment is #68725, which however is not as suitable for an approach.