Skip to content
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

Image: Add lightbox using directives. #50373

Conversation

artemiomorales
Copy link
Contributor

@artemiomorales artemiomorales commented May 5, 2023

Updated testing instructions and screengrabs on 24 May 2023.
Updated testing instructions with new Gutenberg Experiments setting on 30 May 2023.

What?

This pull request is a replacement for #49621, which was closed by Github's UI due to the target branch being merged to trunk. We can continue the conversation here.

This is part of the experiments section with the interactivity API in revising core blocks.
It uses directives from the Directives hydration system

Why?

It would enable native support for image lightboxes, a commonly used feature in many websites, without the use of jQuery or other external libraries.

How?

It adds directives on click to images, presenting a larger version of the image along with an overlay, which can be hidden with an additional click, by scrolling, or pressing the escape key.

Testing Instructions

  1. in the admin, enable Gutenberg > Experiments > Interactivity API and Behaviors UI
  2. Add an image to a post
  3. Test enabling and disabling the lightbox using image's Advanced panel
  4. View the post
  5. Click on the image

Testing Instructions for Keyboard

  1. Tab to the image
  2. Press Enter to open the lightbox
  3. Test pressing Escape to close the lightbox; also test pressing Enter on the close button

Notes

  • This is an early implementation; the idea is for themes to be able to enable or disable the lightbox behavior, for child themes to be able to override that behavior, and for users to be able to toggle lightboxes for all images sitewide or at the block level.
  • This is part of a broader initiative to enable adding Interactivity API behaviors to components using the editor.
  • Currently, we can just toggle the lightbox for a particular image at the block level under the Advanced panel (see below)

Screenshots or screencast

Editor

lightbox-dropdown.mp4

Frontend

lightbox-fade.mp4

@gziolo
Copy link
Member

gziolo commented May 10, 2023

I paired with Artemio yesterday, and I wanted to reshare some high-level comments:

  • I think the lightbox should be disabled by default for a start, the user would need to explicitly enable it, or with the work on Block Behaviors (Behaviors UI #49972), it’s the theme author who could enable it for all Image blocks with a single setting.
  • We should consider putting the lighbox behind the feature flag that needs to be enabled on the Gutenberg plugin settings page. We could reuse the same flag as the Navigation block (Experimental: Navigation block with the Interactivity API #50041). The reason is that they both use Interactivity API, so it would be beneficial to have a shared way to control whether the experience is present on the site or not.

Otherwise, I think that this PR is shaping up nicely, and we should start testing the logic to plan for landing it in the Gutenberg plugin.

@artemiomorales
Copy link
Contributor Author

@gziolo I've just pushed up the tests.
The two tests potentially missing are 1.) tabbing to the lightbox and 2.) having multiple lightboxes on the page at once.

I'm not sure if the first is necessary and haven't figured out a way to tab through the page elements and compare locators to determine if the lightbox button is selected.

The second would be nice to cover, and I'll get to it when I can.

For the moment, syncing this with WIP: Behaviors UI seems more important than either of these tests, so I'm going to start looking at that.

event.preventDefault();
}

if ( escapeKeyPressed || isTabKeyPressed ) {
Copy link
Member

@gziolo gziolo May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale behind using a Tab key to close the lightbox?

The handling is for the container with the dialog role, so I assumed that you the Escape key would trigger that, or pressing Enter or Space on the close button.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes from the original set of recommendations you posted in the initial PR.

Either of these:
a. Tabbing beyond the last link in the lightbox or shift-tabbing before the first link should close the lightbox
b. Tabbing should cycle within the actionable links within the lightbox until user clicks a close link.

Since we only have one actionable item inside the lightbox, the close button, it made sense to me to go with option A rather than create a focus trap.

Also, rather than tabbing to some unspecified next link on the page, I thought it made sense to put focus on the enable lightbox button when the lightbox closes, that way folks don't lose their place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to @gziolo's comment. Only Esc, Enter, and Space should close the dialog.

@andrewhayward
Copy link
Contributor

I wanted to make sure that @joedolson and @afercia's accessibility comments didn't get lost in the move, and mirror their thoughts around modal dialog interaction.

To summarise, and as previously stated, while the interaction might seem simple and straightforward, and the user experience not that of a typical modal dialog, semantically it essentially just that. Which means we should be careful to provide appropriate functionality.

Any trigger that opens the image overlay should (semantically) present as a button, and be labelled appropriately to explain what it does. This means we cannot use the image itself as the trigger, because that should (semantically) present as an image, and have a label reflecting what it actually is ('alt text').

We could hide the trigger button until it receives focus, or the image is hovered, but that brings its own discovery issues and cognitive accessibility questions. Ideally it would be present at all times.

When open, everything behind it should be inert. There should be an obvious way to close the overlay (typically an × button), as well as respecting the esc key. Generally, tapping the overlay background closes it too. Focus should return to the element that opened it; most likely the original trigger button.


I'm not sure how much of that has been covered, but comments in the previous PR would suggest some of it is already tackled. The most important thing to remember is that while it might visually be a pretty lightweight experience, we should still make sure the appropriate controls and affordances are in place to cover different access needs.

Copy link
Contributor

@joedolson joedolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I've flagged a number of continuing accessibility issues that'll need to be handled before this can move forward.

packages/block-library/src/image/index.php Outdated Show resolved Hide resolved
packages/block-library/src/image/index.php Show resolved Hide resolved
event.preventDefault();
}

if ( escapeKeyPressed || isTabKeyPressed ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to @gziolo's comment. Only Esc, Enter, and Space should close the dialog.

data-wp-context='{ "core": { "image": { "initialized": false, "lightboxEnabled": false, "lastFocusedElement": null } } }'>
<button aria-haspopup="dialog" aria-label="$aria_label" data-wp-on.click="actions.core.image.showLightbox">
$content
</button>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image captions can contain links; with this model, those links would be nested inside a button, which is a) not allowed and b) creates really unpredictable user interactions.

This needs some further working out to cover scenarios like captions containing links and images that are linked.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I've updated the implementation so the image is no longer inside of a button, so that should cover the caption scenario.

As for the image being a link, the lightbox functionality will be overriden and its markup won't render if the user configures the image to be a link.

packages/block-library/src/image/index.php Show resolved Hide resolved
( event.type === 'click' &&
event.pointerType === '' )
) {
context.core.image.lastFocusedElement.focus();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing, this is failing for the Enter and Space keys when a screen reader is running; the focus is set correctly using Esc and Tab (although tab shouldn't close the dialog.) This is probably because the screen reader is intercepting keypresses at some level. Tested with NVDA/Chrome and NVDA/Firefox. Focus is being lost.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case we don't want to focus the lastFocusedElement when the lightbox is hidden? Couldn't we just move the context.core.image.lastFocusedElement.focus(); outside of the conditional?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just testing it and removing the conditional, and just calling context.core.image.lastFocusedElement.focus(), seems to solve the issue with NVDA. It works with Enter and Space as well.

Don't we want to focus the last element when the lightbox is hidden after scrolling or when clicking outside?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to focus the last element when the lightbox is hidden after scrolling or when clicking outside?

@SantosGuillamot Thanks for looking into this! I previously didn't take this approach because setting the focus when closing the dialog with a mouse click produced a visible focus outline, which would have been distracting for mouse users. However, in the latest push, I moved the image outside of the button element, which fortunately also resolves the UI clutter, so I've been able to remove this conditional 👍

@draganescu
Copy link
Contributor

draganescu commented May 16, 2023

Hi @andrewhayward - I am no accessibility expert just a dev who reading this bit:

We could hide the trigger button until it receives focus, or the image is hovered, but that brings its own discovery issues and cognitive accessibility questions.

... felt a bit caught off guard. The fact that "hide the trigger button until it receives focus" which we do all over the place via a VisuallyHidden component is problematic for cognitive accessibility needs raises a question to me:

  • how much of web accessibility is making web content (be it document or application) available to assisitive technology users and how much of the scope of all accessibility needs are the responsibility of the assistive technology itself to handle?

In particular, using VisuallyHidden elements that are semantic and focusable, just not visible, should be something that the assisitive technology used by the user should handle for cognitive accessibility needs and describe the semantics - which exist - in a way that is predictable and actionable including in cognitive accessibility needs contexts. Is this a wrong line of thinking?

@andrewhayward
Copy link
Contributor

Thanks for follow-up, @draganescu. Great questions.

How much of web accessibility is making web content (be it document or application) available to assistive technology users and how much of the scope of all accessibility needs are the responsibility of the assistive technology itself to handle?

In particular, using VisuallyHidden elements that are semantic and focusable, just not visible, should be something that the assistive technology used by the user should handle for cognitive accessibility needs and describe the semantics - which exist - in a way that is predictable and actionable including in cognitive accessibility needs contexts. Is this a wrong line of thinking?

Very often when we think about accessibility, we consider the technologies that users might make use of to access content, but fall short of thinking about the users themselves. Cognitive accessibility falls into a murky area between traditional accessibility standards and UX.

There's a lot to go into that would be a whole discussion in its own right, but the biggest takeaway in this context is that not everyone who has particular access needs meets them with assistive technology. So your line of thinking isn't necessarily wrong, just maybe not broad enough either. We cannot know who is accessing content, or how, so we have a responsibility to make it as clear and straightforward as we can, both semantically and visually.

@gziolo
Copy link
Member

gziolo commented May 17, 2023

With 6f2d112, there might be slight changes necessary in the webpack config and the PHP file that register the view script. The built file is going to be located in build/block-library/blocks/image/interactivity.min.js.

@artemiomorales artemiomorales force-pushed the experiment/image-with-interactivity-api branch from 7b76791 to 6f53c0c Compare May 19, 2023 10:14
packages/block-library/src/image/interactivity.js Outdated Show resolved Hide resolved
packages/block-library/src/image/block.json Outdated Show resolved Hide resolved
event.preventDefault();
}

if ( escapeKeyPressed || isTabKeyPressed ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes from the original set of recommendations you posted in the initial PR.

Either of these:
a. Tabbing beyond the last link in the lightbox or shift-tabbing before the first link should close the lightbox
b. Tabbing should cycle within the actionable links within the lightbox until user clicks a close link.

Since we only have one actionable item inside the lightbox, the close button, it made sense to me to go with option A rather than create a focus trap.

Also, rather than tabbing to some unspecified next link on the page, I thought it made sense to put focus on the enable lightbox button when the lightbox closes, that way folks don't lose their place.

( event.type === 'click' &&
event.pointerType === '' )
) {
context.core.image.lastFocusedElement.focus();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to focus the last element when the lightbox is hidden after scrolling or when clicking outside?

@SantosGuillamot Thanks for looking into this! I previously didn't take this approach because setting the focus when closing the dialog with a mouse click produced a visible focus outline, which would have been distracting for mouse users. However, in the latest push, I moved the image outside of the button element, which fortunately also resolves the UI clutter, so I've been able to remove this conditional 👍

data-wp-context='{ "core": { "image": { "initialized": false, "lightboxEnabled": false, "lastFocusedElement": null } } }'>
<button aria-haspopup="dialog" aria-label="$aria_label" data-wp-on.click="actions.core.image.showLightbox">
$content
</button>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I've updated the implementation so the image is no longer inside of a button, so that should cover the caption scenario.

As for the image being a link, the lightbox functionality will be overriden and its markup won't render if the user configures the image to be a link.

@artemiomorales
Copy link
Contributor Author

I've pushed a batch of changes that covers @joedolson's accessibility feedback. I've marked 'resolved' for the two simplest issues; for the rest, I see them as handled on my end but believe this PR is ready for another review.

Any trigger that opens the image overlay should (semantically) present as a button, and be labelled appropriately to explain what it does. This means we cannot use the image itself as the trigger, because that should (semantically) present as an image, and have a label reflecting what it actually is ('alt text').

This feedback from @andrewhayward has also been addressed, namely the image itself is no longer the trigger — instead the button is overlaid on top of the image. The button also has an aria-label, though does not have a visible label in the UI pending additional explorations and discussion with design folks.

Also, I just realized a few of my comments from several days ago never became public due to a snafu on my end with the Github UI. That's now been updated, so @gziolo and @SantosGuillamot should see responses to their questions from a while back. Sorry for the delay on that!

Important: I've also rebased on trunk and the lightbox is now hidden behind an experimental flag. To activate it, enable Gutenberg > Experiments > Core blocks in the admin.

@SantosGuillamot
Copy link
Contributor

SantosGuillamot commented May 19, 2023

On another note, I was experimenting with the possibility of using a zooming animation for the lightbox, just in case we want to consider it. The idea was to create a similar effect to the UX of the View Transitions API. This is how the experiment looks like at this moment (I did it before the latest changes where @artemiomorales changed the trigger) :

Desktop

Lightbox.Zoom.in.-.Desktop.mp4

Mobile

Lightbox.Zoom.In.-.Mobile.mp4

Explanation

EDIT: I changed the code to use CSS variables which makes the implementation easier, so this video is a bit outdated.

The code of that experiment can be found in this branch, and I also made a video explaining it a bit.

https://www.loom.com/share/b7138ad47a6a42adb7808f3b07a193e4

If at the end we want to follow this approach, I'm happy to help polish the code 🙂

Copy link
Contributor

@SantosGuillamot SantosGuillamot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments so far, although I'd like to do a deeper review later today. Thanks for taking care of it, Artemio 🙂

packages/block-library/src/image/interactivity.js Outdated Show resolved Hide resolved
packages/block-library/src/image/interactivity.js Outdated Show resolved Hide resolved
packages/block-library/src/image/index.php Outdated Show resolved Hide resolved
packages/block-library/src/image/index.php Outdated Show resolved Hide resolved
packages/block-library/src/image/block.json Outdated Show resolved Hide resolved
packages/block-library/src/image/index.php Outdated Show resolved Hide resolved
@SantosGuillamot SantosGuillamot enabled auto-merge (squash) May 24, 2023 21:05
@SantosGuillamot SantosGuillamot merged commit ac13b94 into WordPress:trunk May 24, 2023
@github-actions github-actions bot added this to the Gutenberg 15.9 milestone May 24, 2023

if ( ! empty( $experiments['gutenberg-interactivity-api-core-blocks'] ) && 'none' === $link_destination && $lightbox ) {

$aria_label = 'Open image lightbox';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for not making this text translatable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely should be translated 👍

return
<<<HTML
<div class="wp-lightbox-container"
data-wp-island
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these data attributes are self explanatory, but this "island" part is not, I think this could be improved to be easier to read?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is special and I run into an issue with it as I had to learn it enables Interactivity API runtime for the block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point.

Island is a term that refers to a part of the site that is hydrated and therefore becomes interactive.

The idea of an "islands architecture" was first coined by Etsy's frontend architect Katie Sylor-Miller in 2019, and expanded on in this post by Jason Miller (Preact's creator). Since then, multiple JavaScript frameworks have adopted it: (1, 2, 3...).

Maybe we can change it to data-wp-interactive. Would that be easier to read, or do you have any other suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think data-wp-interactive would be clearer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Carolina 🙂

I opened this issue to discuss it.

For context, the full version of the Interactivity API is still in that repo, but we'll move everything to the Gutenberg repo after the WP 6.3 feature freeze, including the codebase and discussions.

@luisherranz luisherranz deleted the experiment/image-with-interactivity-api branch May 25, 2023 09:16
@cbravobernal cbravobernal changed the title Experiment: Add lightbox to Image block using directives (reopened) Experiment: Add lightbox to Image block using directives May 26, 2023
@cbravobernal cbravobernal changed the title Experiment: Add lightbox to Image block using directives Image: Add lightbox using directives. May 26, 2023
@ramonjd ramonjd added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label May 29, 2023
@ramonjd ramonjd added Needs PHP backport Needs PHP backport to Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels May 31, 2023
@cbravobernal cbravobernal removed the Needs PHP backport Needs PHP backport to Core label Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Feature] Interactivity API API to add frontend interactivity to blocks. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.