All images in a post have lightbox when no image is explicitly opted into lightbox

[johnwatkins0]

Bug Description

With this plugin active on a site, any page that (a) contains one or more amp-imges and (b) doesn't have lightboxing explicitly turned on for any amp-img receives auto lightboxing for all images on the page. This document pretty much explains why: https://github.com/ampproject/amphtml/blob/master/spec/auto-lightbox.md

While auto-lightboxing is probably fine for many sites, there are cases where it's not desirable -- e.g. very small images tend not to look great when expanded into a lightbox, and decorative graphical elements generally don't benefit from the lightbox.

Expected Behaviour

Image blocks with "Add lightbox effect" toggled off should never have a lightbox.

Perhaps the plugin could make it possible to opt out of auto lightboxing at the post level. Where auto lightboxing is disabled, however, it should still be possible to enable the lightbox for individual images using the "Add lightbox effect" toggle.

On the project where this came up, we did just that -- we added a post meta field to the editor sidebar, to the effect of "Disable lightbox for this post." When this is on, we add data-amp-auto-lightbox-disable to the element wrapping the post content. When this attribute is present, images with "Add lightbox effect" turned on still have the lightbox. This approach could be difficult to implement from the plugin, though, because the plugin can't easily modify the theme's markup. The attribute could be added to the body tag, maybe.

Steps to reproduce

1. Activate the Twenty Twenty theme on a site and turn on the AMP plugin (I'm using the latest develop branch)
2. Create a new post and add an image. Do not turn on "Add lightbox effect" for the image.
3. Publish the post and view it. Click on the image and note that it has lightboxing, even though lightboxing was not turned on for the image.
4. If you add another image to the post with "Add lightbox effect" toggled on, however, you'll notice that the first image no longer has lightboxing, while the second image does.

Additional context

• WordPress version: 5.4.2
• Plugin version: latest develop
• AMP plugin template mode: Standard
• PHP version: 7.3
• OS: Mac
• Browser: Chrome

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• By default the data-amp-auto-lightbox-disable attribute should be added to the body.
• A PHP filter should be available to disable this disabling.

Implementation brief

See https://gist.github.com/westonruter/b6691407af1648b402b09371a9faf7f0

• Add a new AMP_Auto_Lightbox_Disable_Sanitizer which sets the data-amp-auto-lightbox-disable attribute on the body.
• Inside of amp_get_content_sanitizers(), conditionally add this sanitizer to the $sanitizers if apply_filters( 'amp_auto_lightbox_disabled', true ) returns false.

QA testing instructions

Demo

Changelog entry

[westonruter]

See also #3579 (comment). So the answer to that question apparently is yes: we still do need the lightbox toggle.

[westonruter]

Here's a mini plugin that adds the the data-amp-auto-lightbox-disable attribute to the body: https://gist.github.com/westonruter/b6691407af1648b402b09371a9faf7f0

I think it makes sense to go ahead and update the plugin to do this by default. Maybe there should be a filter to control whether the attribute is added, but it would be added by default.

[westonruter]

Note that even when data-amp-auto-lightbox-disable is added to the body, the amp-auto-lightbox-0.1.js script will continue to be downloaded even though it won't be used for anything. See ampproject/amphtml#26452.

[westonruter]

As opposed to a filter, enabling/disabling the auto-lightbox functionality could be an advanced option on the Settings screen. There may be some other such settings that would be relevant in such a section, such as whether galleries should by default use a carousel and/or lightbox per #4989 (comment).

I'm just wary of adding too many settings. If we find that most users benefit from something being enabled (or disabled) we should just do it and provide an API to override the default behavior. If there are more than 20% of users who would be wanting to override the default, then in that case the UI would make sense.

[dhaval-parekh]

I'm just wary of adding too many settings. If we find that most users benefit from something being enabled (or disabled) we should just do it and provide an API to override the default behavior. If there are more than 20% of users who would be wanting to override the default, then in that case the UI would make sense.

As of now, I have added the filter to disable the auto lightbox. (by default auto lightbox will be enabled). Let me know if we should add a UI for that option.

cc: @westonruter

[westonruter]

As of now, I have added the filter to disable the auto lightbox. (by default auto lightbox will be enabled). Let me know if we should add a UI for that option.

No UI is needed, no. But let's disable the auto-lightbox by default. See https://github.com/ampproject/amp-wp/pull/6936/files#r822046774.

[westonruter]

To test this, try with a theme like Twenty Seventeen. I found that auto-lightbox didn't engage in Twenty Twenty or Twenty Twenty-One because they already use lightboxes.

[pooja-muchandikar]

QA Passed ✅

Before: Image had lightbox effect even if lightbox toggle was off

Screen.Recording.2022-03-23.at.12.18.04.PM.mov

---

After: Image is displayed in lightbox only if lightbox toggle is enabled

Screen.Recording.2022-03-23.at.12.19.38.PM.mov

---