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

Add lightbox effect doesn't work with gallery shortcode #2850

Closed
rsmith4321 opened this issue Jul 19, 2019 · 34 comments · Fixed by #3097
Closed

Add lightbox effect doesn't work with gallery shortcode #2850

rsmith4321 opened this issue Jul 19, 2019 · 34 comments · Fixed by #3097
Labels
Bug Something isn't working Embeds Sanitizers
Milestone

Comments

@rsmith4321
Copy link

rsmith4321 commented Jul 19, 2019

I have a gallery shortcode inserted with the shortcode block like [gallery ..... link="file"] so each image should be linked to its source image. And without any amp settings selected it works correctly. However, if in the AMP settings in the shortcode block editor sidebar I have selected to "Add lightbox effect", then the the links to the source images disappear and the lightbox doesn’t work. It’s is the same as if link=”file” is not in the shortcode.

Originally https://wordpress.org/support/topic/amp-carousel-doesnt-have-alt-tags/

@swissspidy swissspidy added the Support Help with setup, implementation, or "How do I?" questions. label Jul 21, 2019
@rsmith4321
Copy link
Author

rsmith4321 commented Jul 29, 2019

This isn't a "Help Request" I'm reporting a bug. Has anyone tested a standard wordpress gallery shortcode with the "add lightbox effect" option selected but the "display as carousel" option not selected?

The lightbox doesn't work. Can someone please test this, unless it something specific to my site but I don't think it is? Thanks!

@westonruter westonruter added Bug Something isn't working and removed Support Help with setup, implementation, or "How do I?" questions. labels Jul 30, 2019
@westonruter
Copy link
Member

I can confirm this is a bug. If carousel is not enabled, then no lightbox is applied.

@westonruter
Copy link
Member

I believe the problem is rooted in this code:

public function maybe_override_gallery( $html, $attributes ) {
$is_lightbox = isset( $attributes['amp-lightbox'] ) && true === filter_var( $attributes['amp-lightbox'], FILTER_VALIDATE_BOOLEAN );
if ( isset( $attributes['amp-carousel'] ) && false === filter_var( $attributes['amp-carousel'], FILTER_VALIDATE_BOOLEAN ) ) {
if ( true === $is_lightbox ) {
remove_filter( 'post_gallery', [ $this, 'maybe_override_gallery' ], 10 );
$attributes['link'] = 'none';
$html = '<ul class="amp-lightbox">' . gallery_shortcode( $attributes ) . '</ul>';
add_filter( 'post_gallery', [ $this, 'maybe_override_gallery' ], 10, 2 );
}
return $html;
}

@rsmith4321
Copy link
Author

rsmith4321 commented Jul 30, 2019

Thanks, I was actually looking though that code as well but I don't know enough to fix it myself. It's adding a class="amp-lightbox" to the ul which I don't think does anything. It needs to just add lightbox attribute to the end of each <amp-img> tag, then even better it could use figcaption or alt for description as described here https://amp.dev/documentation/components/amp-lightbox-gallery/?format=websites

@rsmith4321
Copy link
Author

Has anyone had a chance to look at this bug?

@westonruter
Copy link
Member

I don't believe so, but we have it prioritized in the project board we are currently working through: https://github.com/ampproject/amp-wp/projects/13

@kienstra
Copy link
Contributor

kienstra commented Sep 12, 2019

Steps To Test

Hi @csossi,
Sorry for the delay. Here are some steps to test this on the staging site:

  1. Create a new post
  2. Add a Shortcode block
  3. Paste this in the block:
[gallery ids="3079, 2376, 1714"]
  1. Select 'Add lightbox effect'
  2. Save the page, and go to the AMP URL on the front-end
  3. Expected: on clicking an image, the lightbox appears
  4. Go back to the block editor, and paste this in the same Shortcode block:
[gallery ids="3079, 2376, 1714" link="file"]
  1. Save the page, and go to the AMP URL on the front-end
  2. Expected: just like in step 6, clicking on an image causes the lightbox to appear

@csossi
Copy link

csossi commented Sep 12, 2019

  • not seeing Lightbox effect here. on clicking image, user is redirected to page containing the image

@schlessera
Copy link
Collaborator

Hi @csossi,
I was just able to test this on the staging site as well after getting access.
What I'm seeing is that it works as expected, but the site is configured in such a way that you need to switch to the AMP version of the post first. On the non-AMP version, this has no effect.

Can you recheck and confirm that you are testing on the AMP version of the page?

https://cl.ly/d89f5f783617

@csossi
Copy link

csossi commented Sep 13, 2019

Ugh! Right you are @schlessera - wasn't checking the AMP URL - all good here

@csossi
Copy link

csossi commented Sep 13, 2019

Verified in QA

@rsmith4321
Copy link
Author

I just tested the latest version of the amp plugin. I have the lightbox option set on the gallery shortcode but the images still do not open in the lightbox. You can check out an example here https://www.ryansmithphotography.com/blog/2019/09/30/bridal-portraits-on-the-conway-river-and-new-garden-walks-with-the-nikon-d850/

@westonruter
Copy link
Member

@kienstra Would you please double-check this?

@kienstra
Copy link
Contributor

kienstra commented Oct 2, 2019

Sure, Weston!

@kienstra
Copy link
Contributor

kienstra commented Oct 3, 2019

Question About Caching

Hi @rsmith4321,
Thanks for letting us know. I also see the issue on the link you shared, where the gallery shortcode with 'Add lightbox effect' checked doesn't actually open a lightbox on clicking it:

[gallery ids="3762, 3470, 3327, 2877" link="file"]  // Only example IDs, of course.

Is there anything you could do to clear the cache on that URL, if that applies?

It looks like it's using WP Rocket:

<!-- This website is like a Rocket, isn't it? Performance optimized by WP Rocket. Learn more: https://wp-rocket.me - Debug: cached@1570044620 -->

Though not to cast blame. It's showing the latest version of the AMP plugin, so it could still be a bug with the plugin:

<meta name="generator" content="AMP Plugin v1.3.0; mode=standard; experiences=website">

Locally, I couldn't reproduce the issue using the 1.3.0-built tag.

The expected behavior with the latest (v1.3.0) is that when link="file" and 'Lightbox', the <amp-img> tags aren't wrapped in <a> tags:

The fact that they're wrapped in <a> tags suggests that something is wrong. Maybe with caching, maybe with the plugin:

amp-img-wrapped

@rsmith4321
Copy link
Author

The issue seems to be in my custom filter in functions.php. Instead of using attributes in the shortcode itself I had the following filter. However if I remove this and add link="file" directly in the shortcode it works. Do you know how I could correct this filter so it would work?

add_filter( 'shortcode_atts_gallery', function( $atts ) { $atts['size'] = 'large'; $atts['columns'] = 1; $atts['link'] = 'file'; return $atts; } ); add_filter( 'use_default_gallery_style', '__return_false' );

@kienstra
Copy link
Contributor

kienstra commented Oct 3, 2019

Ah, thanks for sharing that filter. I can reproduce the issue with that. I'll look at this and get back to you.

@rsmith4321
Copy link
Author

Great I appreciate it, using a filter is much better for me than editing each gallery shortcode.

@kienstra
Copy link
Contributor

kienstra commented Oct 3, 2019

Question About Workaround

Hi @rsmith4321,
Sure, it sounds like using the filter is easier for your workflow than editing the individual shortcodes.

But as a workaround, would it be possible to remove $atts['link'] = 'file'; from the filter? Unless that filter is also needed on non-AMP URLs.

For AMP, it shouldn't be necessary to add link="file" to the shortcode string, as the plugin overwrites it.

[gallery ids="3762, 3470, 3327, 2877" size="large"]

It looks like your site is using Standard mode, which is great! If there happen to be non-AMP URLs with the gallery shortcode, the link="file" string would be needed.

Thanks, Ryan!

@rsmith4321
Copy link
Author

Your right, without that link="file" it seems to work correctly and the lightbox is great with the slideshow function and captions! One other question, instead of going to each gallery shortcode in the editor and clicking the add lightbox effect checkbox. Would there be any kind of filter I could add to my functions.php that would default to on the add lightbox effect on all gallery shortcodes?

@rsmith4321
Copy link
Author

One other thing I've noticed, with add lightbox effect disabled the original image size will be linked too which is what I want. However with the lightbox effect on it seems to just load the large thumbnail size in the lightbox. Is it possible for the lightbox to display the original image file?

@kienstra
Copy link
Contributor

kienstra commented Oct 3, 2019

One other question, instead of going to each gallery shortcode in the editor and clicking the add lightbox effect checkbox. Would there be any kind of filter I could add to my functions.php that would default to on the add lightbox effect on all gallery shortcodes?

That'd be useful 😄 But unfortunately, I couldn't find a way to do that.

The method that adds the lightbox runs as a filter of post_gallery, and there unfortunately isn't a way that I can find to add the lightbox before it runs.

@kienstra
Copy link
Contributor

kienstra commented Oct 3, 2019

Hi Ryan,

One other thing I've noticed, with add lightbox effect disabled the original image size will be linked too which is what I want. However with the lightbox effect on it seems to just load the large thumbnail size in the lightbox. Is it possible for the lightbox to display the original image file?

Hm, interesting.

Could you please share a link with that issue, and an image where it exists (if it's a different link from the one you shared)?

In the link you shared above, it looks like the images in the lightbox are generally 1500 x 1000.

For example, here's the 2nd image in the gallery:
image-size-now

@kienstra
Copy link
Contributor

kienstra commented Oct 3, 2019

Hi Ryan,
Sorry, I forgot to mention that $atts['link'] = 'file'; would be necessary in that filter if there are galleries that that you don't want to be lightboxes, but you still want to link to the files.

@rsmith4321
Copy link
Author

Yes the 1500x1000px image is the large thumbnail size. The original image size would be a 2048px image. Here is a link to the full size original file, this is what a typical lightbox would link to. This would also be what would be linked to with just link="file" setting with no lightbox. https://www.ryansmithphotography.com/wp-content/uploads/2019/09/bride-standing-on-train-tracks-conway-river-walk-2019.jpg. The large size thumbnail is probably fine anyway but this isn't typical lightbox behavior. Thanks for the help with this.

@westonruter
Copy link
Member

@rsmith4321 Would you please test #3439?

@kienstra
Copy link
Contributor

kienstra commented Oct 3, 2019

Yes the 1500x1000px image is the large thumbnail size. The original image size would be a 2048px image.

Good point, it would make sense to have a bigger image in a lightbox. I'll look at this more. It might be more of an issue with AMP in general.

@kienstra
Copy link
Contributor

kienstra commented Oct 3, 2019

With #3439, this should allow the previous filter, even setting $attr['link'] = 'file';

@kienstra
Copy link
Contributor

kienstra commented Oct 4, 2019

Better Lightbox Images

Hi @rsmith4321,

Here is a link to the full size original file, this is what a typical lightbox would link to.

Yeah, that makes sense to want a bigger image size for the lightbox.

I think this might have to come from the AMP project, though. Here are some slightly related issues:
ampproject/amphtml#21736
ampproject/amphtml#14900

Though unfortunately, I haven't noticed any issue that looks like it's close to being fixed.

I'll let you know if anything else comes to mind.

@rsmith4321
Copy link
Author

Thanks, I'll keep an eye on this to see if it's ever resolved but it's not a huge deal. There is something strange going on because sometimes it does put the original size in the lightbox and sometimes the thumbnail. It's strange I would assume that the html would be generated by php in wordpress selecting the full size image and not something in amp.

I also tested the latest update and it seems to fix the issue with $attr['link'] = 'file';. I've added that back into my functions.php and the lightbox is working, thanks.

@kienstra
Copy link
Contributor

kienstra commented Oct 4, 2019

Hi @rsmith4321,

There is something strange going on because sometimes it does put the original size in the lightbox and sometimes the thumbnail. It's strange I would assume that the html would be generated by php in wordpress selecting the full size image and not something in amp.

Ah, interesting.

Is there an example image you can point to that does that?

It looks like at least some portrait images are 1365 x 2048:

portrait-intrinsic-size

...but maybe that's not the 'full size image' that you referred to.

@rsmith4321
Copy link
Author

Yes that is the original full size. Basically if the file name has image dimensions at the end of the file name such as 1500x1000.jpg, that is a wordpress generated thumbnail. If it doesn't have this it'a an original upload size image. In the wordpress php that would be like full, large, medium, and thumbnail. Why some images in the lightbox show the full size and some the large size I have no idea, they should all be the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Embeds Sanitizers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants