Skip to content
This repository has been archived by the owner on Jan 24, 2024. It is now read-only.

Set default image behavior to expand on click (lightbox) #529

Closed
2 tasks
richtabor opened this issue Sep 28, 2023 · 11 comments
Closed
2 tasks

Set default image behavior to expand on click (lightbox) #529

richtabor opened this issue Sep 28, 2023 · 11 comments

Comments

@richtabor
Copy link
Member

To showcase the new lightbox functionality in core, I propose we set lightbox (Expand on click) on for images.

  • Update theme.json to reflect lightbox global setting (source)
  • Update patterns that should have lightbox off, like clients
@carolinan
Copy link
Contributor

I would strongly recommend against this because the lightbox has multiple issues.
If the lightbox is improved and is accessible in a later release, the theme can be extended with new patterns.

@richtabor
Copy link
Member Author

I would strongly recommend against this because the lightbox has multiple issues.

Can you give sources? This feature is currently planned for 6.4.

@carolinan
Copy link
Contributor

The feature doesn't have its own label, here are some:

Tracking issue: WordPress/gutenberg#51132

WordPress/gutenberg#54921
WordPress/gutenberg#54869
WordPress/gutenberg#54916
WordPress/gutenberg#54682
WordPress/gutenberg#54635

There are accessibility issues that I have run into and that I know have been discussed, but right now, I cannot find specific issues for them; they may only be mentioned in other discussions.
For example, alt texts are not announced, so the only information a screen reader user gets is "Enlarge image" followed by "close"

@artemiomorales
Copy link

For example, alt texts are not announced, so the only information a screen reader user gets is "Enlarge image" followed by "close"

@carolinan Thanks for catching this. This had been working previously but had regressed in a recent shuffle of the code. Here's a PR fixing it.

I haven't come across other accessibility issues or been made aware of any. As of the last time accessibility reviewers took a look, it also seemed to be working well, so would appreciate links to those discussions if you or others could surface them 🙏

As far as enabling the lightbox by default goes, I personally don't think that's necessary for core and it can instead be showcased via themes, though am happy to hear other opinions and further thoughts on that.

@richtabor
Copy link
Member Author

As far as enabling the lightbox by default goes, I personally don't think that's necessary for core and it can instead be showcased via themes, though am happy to hear other opinions and further thoughts on that.

Agreed! That's why I'm proposing it here in TT4.

@artemiomorales
Copy link

Agreed! That's why I'm proposing it here in TT4.

Got it! Pardon, I missed that this is in the Twenty Twenty Four repo 😆

@luminuu
Copy link
Member

luminuu commented Oct 9, 2023

@carolinan Have you followed the development of the lightbox recently and can you tell us if the A11Y issues have been worked on?

@carolinan
Copy link
Contributor

Only some are worked on https://github.com/WordPress/gutenberg/issues?q=is%3Aopen+is%3Aissue+author%3Aafercia+label%3A%22%5BBlock%5D+Image%22
During a test on Friday, Andrea found more problems with the usability, that does not have issues opened yet. I believe he plans to write the dedicated issues today.

@richtabor
Copy link
Member Author

There is a follow-up PR to lightbox ready for testing, but it's an exploration; not a matter-of-fact. Worth a look for sure though.

@github-project-automation github-project-automation bot moved this to 📁 Triage in TT4 Project Board Oct 12, 2023
@richtabor richtabor moved this from 📁 Triage to 📋 Backlog in TT4 Project Board Oct 12, 2023
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in TT4 Project Board Oct 16, 2023
@richtabor
Copy link
Member Author

Going to close this as not-planned. The lightbox feature needs a bit more attention before we should enable it by default out-of-the-box. This is a good PR #530 to showcase the how though, for future reference.

@MaggieCabrera
Copy link
Collaborator

Let's make a point of including a patch for TT4 on 6.5 including this change

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

5 participants