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

johnwatkins0 commented 4 years ago

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

Activate the Twenty Twenty theme on a site and turn on the AMP plugin (I'm using the latest develop branch)
Create a new post and add an image. Do not turn on "Add lightbox effect" for the image.
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.
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 commented 4 years ago

See also https://github.com/ampproject/amp-wp/issues/3579#issuecomment-545567310. So the answer to that question apparently is yes: we still do need the lightbox toggle.

westonruter commented 3 years ago

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 commented 3 years ago

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 https://github.com/ampproject/amphtml/issues/26452.

westonruter commented 3 years ago

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 https://github.com/ampproject/amp-wp/issues/4989#issuecomment-689677233.

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.