![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png){kind=logo}
Opened 6 weeks ago
Last modified 5 weeks ago
#8285 reopened enhancement
Plugin Directory: Lightbox images are too small
| Reported by: | alanjacobmathew | Owned by: | thilinah |
|---|---|---|---|
| Priority: | normal | Milestone: | |
| Component: | Plugin Directory | Keywords: | has-screenshots has-patch |
| Cc: |
Description
The lightbox images/screenshots in the plugin directory are so small, that the purpose of having a lightbox is negated.
It'd be better if the lightbox image is the same size as that of original size.
Attachments (2)
Change History (22)
#2
follow-up:
↓ 4
@
6 weeks ago
- Keywords needs-patch added
I was able to duplicate it on a non-hdpi device. Chrome on Linux via remote desktop. Screen size 1280Wx900H
Ref: #8083 cc @thilinah
This ticket was mentioned in PR #686 on WordPress/wordpress.org by @thilinah.
6 weeks ago
#3
- Keywords has-patch added; needs-patch removed
## Summary
The screenshot gallery lightbox (added in #622) had two production-only bugs:
- Image too small — the enlarged image was capped at the 900px Photon thumbnail instead of showing the full-resolution screenshot.
- Image duplicated — the screenshot rendered twice, stacked vertically.
Both are fixed here. They were related: fixing (1) is what surfaced (2).
## Background
Plugin screenshots are external ps.w.org assets with no real attachment, so each core/image block is given a *synthetic* attachment ID. Core's lightbox renderer treats that ID as a genuine attachment:
So the lightbox had no full-resolution source to enlarge and no real dimensions, falling back to the thumbnail's Photon-shrunk size.
This was not surfaced during sandbox testing because the Photon srcset is gated to production only photon_srcset() bails on non-production environments.
With no srcset on the sandbox, the thumbnail <img> loads the full-resolution ps.w.org original, so the thumbnail's natural size is the full image and the lightbox looked correct. Only on production, where Photon caps the thumbnail at ≤900px, did the bug appear — and the duplicate-image bug below was likewise masked because the enlarged container had no source to render.
#### Fix
- Repair the lightbox metadata (class-screenshots.php)
A new render_block_core/image filter (priority 20, after core's priority-15 lightbox pass) overrides the interactivity state for our screenshot blocks with the real uploadedSrc, targetWidth/targetHeight, and clears lightboxSrcset so the enlarged view loads the lossless original instead of a capped Photon candidate. Core keys this state by a per-render uniqid(), so the filter reads that key back out of the rendered markup. wp_interactivity_state() merges later-call-wins, so the corrected values replace the broken ones. Applied on all environments so sandbox and production behave identically.
- Stop stacking the lightbox's two image containers (gallery-lightbox-enhancements)
The caption polyfill forced core's two center-overlapping .lightbox-image-container siblings to position: relative, dropping them into normal flow and stacking them vertically. This only ever looked correct because the enlarged container was empty (bug 1 above). Once that container held a real image, the screenshot rendered twice.
- CSS: removed the position: relative override so core's absolute center-overlap is restored.
- JS: the caption is now mounted into the top (full-resolution) container so it isn't occluded by the enlarged image.
#### Testing
- Before startign to test, comment out this line: http://github.com/WordPress/wordpress.org/blob/53180a2d8dec3ab9ad4acc5dea5d732e667b9e1c/wordpress.org/public_html/wp-content/plugins/plugin-directory/shortcodes/class-screenshots.php#L588
- Visit a plugin page e.g: https://wordpress.org/plugins/woocommerce/
- The enlarged image is shown on lightbox.
- The image renders once, not stacked/duplicated.
- Captions still pin to the bottom of the picture. (e.g: https://wordpress.org/plugins/wordpress-seo/)
## Closes / refs
#### Screenshots
Before:
After:
This ticket was mentioned in PR #689 on WordPress/wordpress.org by @thilinah.
6 weeks ago
#6
## Summary
The screenshot gallery lightbox (added in #622) had two production-only bugs:
- Image too small — the enlarged image was capped at the 900px Photon thumbnail instead of showing the full-resolution screenshot.
- Image duplicated — the screenshot rendered twice, stacked vertically.
Both are fixed here. They were related: fixing (1) is what surfaced (2).
## Background
Plugin screenshots are external ps.w.org assets with no real attachment, so each core/image block is given a *synthetic* attachment ID. Core's lightbox renderer treats that ID as a genuine attachment:
So the lightbox had no full-resolution source to enlarge and no real dimensions, falling back to the thumbnail's Photon-shrunk size.
This was not surfaced during sandbox testing because the Photon srcset is gated to production only photon_srcset() bails on non-production environments.
With no srcset on the sandbox, the thumbnail <img> loads the full-resolution ps.w.org original, so the thumbnail's natural size is the full image and the lightbox looked correct. Only on production, where Photon caps the thumbnail at ≤900px, did the bug appear — and the duplicate-image bug below was likewise masked because the enlarged container had no source to render.
## Fix
- Repair the lightbox metadata (class-screenshots.php)
A new render_block_core/image filter (priority 20, after core's priority-15 lightbox pass) overrides the interactivity state for our screenshot blocks with the real uploadedSrc, targetWidth/targetHeight, and clears lightboxSrcset so the enlarged view loads the lossless original instead of a capped Photon candidate. Core keys this state by a per-render uniqid(), so the filter reads that key back out of the rendered markup. wp_interactivity_state() merges later-call-wins, so the corrected values replace the broken ones. Applied on all environments so sandbox and production behave identically.
- Stop stacking the lightbox's two image containers (gallery-lightbox-enhancements)
The caption polyfill forced core's two center-overlapping .lightbox-image-container siblings to position: relative, dropping them into normal flow and stacking them vertically. This only ever looked correct because the enlarged container was empty (bug 1 above). Once that container held a real image, the screenshot rendered twice.
- CSS: removed the position: relative override so core's absolute center-overlap is restored.
- JS: the caption is now mounted into the top (full-resolution) container so it isn't occluded by the enlarged image.
## Testing
- Before startign to test, comment out this line: http://github.com/WordPress/wordpress.org/blob/53180a2d8dec3ab9ad4acc5dea5d732e667b9e1c/wordpress.org/public_html/wp-content/plugins/plugin-directory/shortcodes/class-screenshots.php#L588
- Visit a plugin page e.g: https://wordpress.org/plugins/woocommerce/
- The enlarged image is shown on lightbox.
- The image renders once, not stacked/duplicated.
- Captions still pin to the bottom of the picture. (e.g: https://wordpress.org/plugins/wordpress-seo/)
## Closes / refs
## Screenshots
Before:
After:
@thilinah commented on PR #686:
6 weeks ago
#7
Closed this as I used the wrong workflow.
New PR: https://github.com/WordPress/wordpress.org/pull/689
CC: @boro @dd32
6 weeks ago
#8
@thilinah FWIW It doesn't really matter what repo the branch was filed on. Yes, preferably do your own fork, but it doesn't matter if it's a branch on this repo, especially for things that you're going to merge shortly.
@thilinah commented on PR #689:
6 weeks ago
#9
Suggestion from @alanjacobmathew on closed PR
The 'wp-lightbox-caption', better if it is placed outside the image. That will make sure the screenshots can follow the brand-guidelines too if needed. Right now, if the image has a white shade, the caption becomes nearly unreadable. And most screenshot taken of the admin-screen is nearly white(Take the example of Yoast SEO screenshot above)
Thanks for the suggestion @alanjacobmathew. Since the fix for the original issue (small lightbox image) is a bit urgent, I'll differ this to another PR.
@thilinah commented on PR #686:
6 weeks ago
#10
I just created a issue in trac, and saw this conversation. So adding to it
The 'wp-lightbox-caption', better if it is placed outside the image. That will make sure the screenshots can follow the brand-guidelines too if needed. Right now, if the image has a white shade, the caption becomes nearly unreadable. And most screenshot taken of the admin-screen is nearly white(Take the example of Yoast SEO screenshot above)
Thanks @alanjacobmathew, I've responded to this here: https://github.com/WordPress/wordpress.org/pull/689#issuecomment-4656555620
@annebovelett commented on PR #689:
5 weeks ago
#12
Thank you, @thilinah! Where can I find that PR / Issue for the text outside the image? A second point to that is that the text should be left-aligned. Centered it's hard to read for people with dyslexia (I'm one of the 15% of all users this affects as soon as it's more than one line).
Suggestion from @alanjacobmathew on closed PR
The 'wp-lightbox-caption', better if it is placed outside the image. That will make sure the screenshots can follow the brand-guidelines too if needed. Right now, if the image has a white shade, the caption becomes nearly unreadable. And most screenshot taken of the admin-screen is nearly white(Take the example of Yoast SEO screenshot above)
Thanks for the suggestion @alanjacobmathew. Since the fix for the original issue (small lightbox image) is a bit urgent, I'll differ this to another PR.
cc: @stevejonesdev
@thilinah commented on PR #689:
5 weeks ago
#13
A second point to that is that the text should be left-aligned. Centered it's hard to read for people with dyslexia
Thanks for bringing this up @Bovelett. I've updated the text alignment based on locale, to either left or right.
Where can I find that PR / Issue for the text outside the image?
I'll open the new PR after merging this.
5 weeks ago
#14
@thilinah re:
Since the fix for the original issue (small lightbox image) is a bit urgent, I'll differ this to another PR.
It's safe to say that the caption readability is a larger failure than the image size. Small images are annoying, unreadable text is a literal blocker, and a failure of the Web Content Accessibility Guidelines 2.2 standard that WordPress says it will meet.
Here is an example of an image on my plugin, Accessibility Checker:
This issue that @alanjacobmathew reported should be treated as urgent.
5 weeks ago
#16
There's a tcket from @stevejonesdev for the caption issue in meta trac here.
#17
@
5 weeks ago
- Resolution fixed
- Status closed → reopened
@thilinah
Reopening this
Though it works fine for the examples mentioned above, there is an issue, if the source image height is longer that the vh of the screen, the images becomes small, on lightbox view.
Example: Open the 4th image of https://wordpress.org/plugins/easyfonts/
#18
@
5 weeks ago
- Type defect (bug) → enhancement
@alanjacobmathew agreed this is a valid concern.
But I don't think it's urgent, though, since the image loaded in the lightbox for the 4th screenshot is already the full-size one, and users who need to see the whole thing at full resolution can still open it in a new tab.
To explain the constraint: the lightbox fits each image to the viewport without cropping, so it's bounded by both width and height. For a landscape image, width is the limiting dimension and it fills most of the screen.
For a portrait image taller than the viewport, height becomes the limiting dimension, so it scales down to a narrow column and covers less screen area, even though it's as large as it can be without cropping or distortion. There's no way to make a tall portrait image fill a landscape viewport without cropping it or upscaling it, both of which are worse, so this is partly just the geometry of fit-to-viewport sizing.
As a fix, I'd suggest adding a zoom button to the lightbox. Clicking it would either zoom the image in place (toward 1:1) or open the full-size image in a new tab, effectively restoring the older screenshot behavior of viewing the image at full size, while keeping the default lightbox view clean.
That gives users who want details, an explicit way to get it, without changing the fit-to-viewport default that works well for the landscape screenshots.
Happy to take a pass at this if there's agreement on the approach.
#19
@
5 weeks ago
I want to thank @alanjacobmathew for telling us about the problem with the viewport and for giving us an example from the easyfonts plugin.
I looked into this problem. I can see that it is true. When we take a screenshot in portrait mode it gets a lot smaller when the viewport is not very tall even though the picture is already the size.
It makes sense that we do not want the picture to be bigger than the viewport because this helps prevent things from getting messy and helps keep everything looking nice.. I think this can be a problem when we have tall screenshots and we want to see the details clearly.
I think it would be an idea to have a special way to zoom in on the picture like a button that says open full size when we are looking at the picture, in a lightbox. This way we can make sure that everything is easy to use and safe.
If someone wants to try to make this zoom feature work I can help test it on screens and see how it looks.
#20
@
5 weeks ago
Thanks @thilinah for the update 🙂 Honestly, Fancybox 5 solves most of this kind of thing out of the box. But I still think the right direction here is to keep building on the core lightbox, and I will keep thinking about whether we can add a clean zoom follow up for tall screenshots without accidentally rebuilding Fancybox piece by piece.
@alanjacobmathew Can you let us know what browser and screen size you're working with here?
I've checked with Chrome/Safari/Firefox and it appears to be at a decent size to me.
FYI @alexodiy incase you have any thoughts.