Fix lightbox on file-linked galleries

[schlessera]

Add the lightbox attribute to gallery shortcode images when they are being linked to by file and the AMP lightobx setting is enabled.

There was no test suite yet for the gallery embed handler, so I added a test suite to make sure this works as expected.

Fixes #2850

[schlessera]

Right now, the test suite has 2 failures: shortcode_with_lightbox and shortcode_with_lightbox_linking_to_file, so both of the cases where the lightbox switch is on but the carousel switch is off.

It looks like these cases are not yet handled by the gallery embed handler, as it spits out non-AMP HTML (probably the unmodified original shortcode handler output).

Also, if you don't specifiy the AMP carousel switch value at all, it seems to default to using a carousel nevertheless. Is that the intended behavior?

[westonruter]

Also, if you don't specifiy the AMP carousel switch value at all, it seems to default to using a carousel nevertheless. Is that the intended behavior?

There's some backwards-compatibility considerations here. When in Reader mode, the intention is to always show the carousel by default since that is how galleries worked in Reader mode. In Standard/Transitional, the behavior changes to default to the regular gallery output and to instead opt-in to the carousel. Is this what you're seeing?

[schlessera]

I haven't tried this on the frontend, this is purely from running the sanitizer within tests and setting its flags.

The gallery embed code does not contain any reference of the mode, so I doubt that is the case here, unless one of the two scenarios (reader vs standard/transitional) hasn't been implemented yet.

For the non-AMP output, you can see the result here: https://travis-ci.org/ampproject/amp-wp/jobs/577377621#L1208-L1224

So, here's what we need to decide upon:

• Should a gallery use <amp-carousel> if the carousel switch is off? If this is conditional, what are those conditions?
• Is it a bug that the new tests have uncovered that the gallery spits out non-AMP images when the carousel is off and lightbox is on?

[westonruter]

• Should a gallery use <amp-carousel> if the carousel switch is off? If this is conditional, what are those conditions?

In Reader mode, a gallery uses an amp-carousel by default. In Standard/Transitional modes, a gallery should only use amp-carousel if it is toggled on.

I just tried adding a Gallery block and I'm not seeing the carousel toggle:

But in the Reader-mode template, it is shown as a carousel:

If I switch to Transitional mode, then I do see a toggle but it is disabled by default:

And on the frontend:

And when enabling the toggle, then I see a carousel on the frontend:

Perhaps the confusion is that there is actually a gallery sanitizer as well: AMP_Gallery_Block_Sanitizer. It has a carousel_required argument which is set to true if in Reader mode:

amp-wp/includes/amp-helper-functions.php

Lines 842 to 844 in 9337046

	'AMP_Gallery_Block_Sanitizer' => [ // Note: Gallery block sanitizer must come after image sanitizers since itś logic is using the already sanitized images.
	'carousel_required' => ! is_array( $theme_support_args ), // For back-compat.
	],

Yes, the gallery handling is somewhat convoluted!

[westonruter]

• Is it a bug that the new tests have uncovered that the gallery spits out non-AMP images when the carousel is off and lightbox is on?

No, it's not a bug because the AMP_Gallery_Embed runs during template generation. During post-processing, the AMP_Img_Sanitizer then ensures any remaining img elements get converted to amp-img. So during template generation (e.g. in embed handlers) it's fine to output img elements.

[kienstra]

Approved

Hi @schlessera and @westonruter,

Sorry for the delay. This looks good, and it's really nice how it adds a test class AMP_Gallery_Embed_Test.

Before

Even when a gallery shortcode had 'Add lightbox effect' selected:

...it still didn't have the lightbox effect on the front-end:

After

The lightbox effect works on the front-end, <amp-img> tags in the carousel have on="tap:amp-lightbox-gallery.activate"

[kienstra]

Wow, great test file with this @dataProvider

[schlessera]

Thanks for all the context. Based on the above, I assume that the current tests should then compare to what should be expected. They can certainly be extended, but I'd suggest doing so in subsequent tickets to get this one moving.