Add caption support to the Gallery block in <amp-carousel> and fix displaying Gallery block as carousel in WP 5.3 #3285

kienstra · 2019-09-17T22:54:22Z

Update: this has changed a lot, please see more recent comments

For images in a gallery that have a caption, this adds caption support when 'Display as carousel' is selected:

Before

After

Closes #2855

(Please do npm run build:css for the CSS here)

For images in a gallery that have a caption, display them when in an <amp-carousel>.

…ple.

kienstra · 2019-09-18T02:13:56Z

There are probably more styling considerations with this. Sometimes the caption overlaps the <amp-img>.

Simply placing the caption on top of the <amp-img>, with a scrim behind it, could be an option. But the scrim doesn't look good with certain themes.

… an example." This reverts commit 0b0b2fd.

kienstra · 2019-09-20T20:57:05Z

Couldn't Get Styling To Work With Core Themes
Maybe a CSS specialist could help

I couldn't get the captions to look good on all of the Core themes, and having them look good on all themes would probably be even harder.

Maybe a CSS specialist could help here.

Also, I tried placing the caption above the <amp-img> and adding a scrim with a black gradient. But sometimes it was too wide:

Here are the displays with the current state of the PR (without the scrim):

Twenty Fifteen

Twenty Sixteen

Twenty Seventeen

Twenty Nineteen

Steps To Reproduce

Create a new post
Add a gallery block
Upload 600 x 400 images
Add captions to some of the images
Select 'Display as carousel'
Save the post, and go to the front-end

Of course, galleries could have any combination of image sizes, but the screenshots above all use 600 x 400 images to simplify this.

kienstra · 2019-09-23T19:12:53Z

Before

The plugin output the <amp-carousel> like:

<amp-carousel type="slides" ...>
        <amp-img ...>
</amp-carousel>

After (with captions)

With this PR, the plugin outputs something like:

<amp-carousel type="slides" ...>
        <div class="slide">
                <amp-img ...>
                <div class="amp-wp-gallery-caption">
                        This is a caption
                </div>
        </div>
</amp-carousel>

(Though the AMP runtime adds a lot of tags, like <div class="i-amphtml-slide-item i-amphtml-slide-item-show">)

It looks like amp-carousels with captions have to have something like a <div class="slide"> wrapper for each slide.

But on the Core themes like Twenty Fifteen and Twenty Nineteen, I couldn't find a way to style the caption so it appears in the same place, or at least looks good.

And ensuring that captions look good on any theme would probably be much harder.

I'm not sure if there's a solution for this 😄

kienstra · 2019-09-23T21:36:55Z

Also, please do npm run build:css to compile the CSS locally.

austintude · 2019-09-28T02:47:07Z

I've been unable, time wise, to look into this more deeply but the prospect of not only adding but modifying these is of great importance to several of my clients.
In an ideal world, I'd be able to change: font-size, margin, padding, alignment, and other layout typical styling options availble with most text based content.
While the multi-level-wrapping-text is clickable and expands up after being clicked, it is so low on the screen that this function is NOT obvious to the end user/site-visitor. /
Thank you for working on this. Keep up the good work.
Daniel

westonruter · 2019-10-01T23:02:32Z

@kienstra I suppose this will then need UX support.

kienstra · 2019-10-01T23:35:55Z

Hi @westonruter,
Yeah, that makes sense.

If it's alright, we've asked Bart Makos to work on this, as he's much better with CSS than me.

Though I'm not sure there is a good solution for captions, where they'll look good on any theme.

austintude · 2019-10-02T02:08:36Z

This is starting to sound fun. I love the word CSS!!!

…

On Tue, Oct 1, 2019, 6:36 PM Ryan Kienstra ***@***.***> wrote: Hi @westonruter <https://github.com/westonruter>, Yeah, that makes sense. If it's alright, we've asked Bart Makos to work on this, as he's much better with CSS than me. Though I'm not sure there is a good solution <#3285 (comment)> for captions, where they'll look good on any theme. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3285?email_source=notifications&email_token=AHC4FRO6VO4EQY5SB6C4Y3LQMPNGHA5CNFSM4IXV6JCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEADCQBA#issuecomment-537274372>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHC4FRMWJSHRWY3KOWIH4NLQMPNGHANCNFSM4IXV6JCA> .

kienstra · 2019-10-02T02:09:39Z

Haha, I wish I was great at CSS 😄

westonruter · 2019-10-08T19:46:38Z

This may have some inspiration: Automattic/newspack-blocks#143

kienstra · 2019-10-08T20:09:40Z

Nice, thanks for posting that! It could help.

It looks like this is working well with the Core themes Twenty Fifteen through Twenty Nineteen.

Instead of modifying the ->textContent property, create a new text node, and append it to the <span> node.

kienstra · 2019-10-25T01:23:27Z

Thanks to Weston's suggestion, captions look good so far. Though there's still some work left.

kienstra · 2019-10-25T01:25:48Z

includes/sanitizers/class-amp-gallery-block-sanitizer.php

 			foreach ( $images as $image ) {
-				$amp_carousel->appendChild( $image );
+				$div = AMP_DOM_Utils::create_node(


This used to simply append the image to the <amp-carousel>. But now it creates a <div class="slide"> wrapper that includes the image the caption if one exists.

Then, it appends that <div class="slide"> to the <amp-carousel>.

It looks like that's needed in order to display a caption.

It's actually a slide in the <amp-carousel> Also, add a comment, and remove an extra newline.

kienstra · 2019-10-25T01:58:48Z

Gallery Blocks With amp-carousel Sometimes Look Different
Especially square images

Some Gallery blocks look different with this PR. But it looks like they better display the images at the width and height that the AMP_Img_Sanitizer produces.

Before

After

The <amp-img> above looks like a rectangle, as you'd expect from its width=“640” height=“400”.

This is from the new object-fit="cover" for the <amp-img>, which prevents the caption from overflowing the image:

Steps to reproduce

Create a new post
Add a gallery block
Upload few 800 x 800 images
Select 'Display as carousel'
Save the post, and go to the front-end of the AMP URL

kienstra · 2019-10-25T02:14:32Z

Steps to test

Create a new post with a Gallery block
Upload few rectangular and square images to it
Add captions to some, including very long captions
Select 'Display as carousel'
Save the post, and go to the front-end of the AMP URL
Expected: the captions always appear over the image, no matter how long they are:

Ideally, test this with several themes

This is more important than text-align: center.

westonruter

The style of the captions overlaying the gallery looks very good now. Nevertheless, I had try to update the gallery block sanitizer to work with the new markup now coming in WordPress 5.3. See #3285 (comment)

I took an initial stab at adding compatibility, but some tests are broken now and need to be fixed. The changes need to be in WordPress 5.2 without Gutenberg active as well, to ensure the carousel is displaying as expected.

kienstra · 2019-10-25T16:11:12Z

I took an initial stab at adding compatibility, but some tests are broken now and need to be fixed. The changes need to be in WordPress 5.2 without Gutenberg active as well, to ensure the carousel is displaying as expected.

Hi Weston, thanks for making those commits. Ah, I should have tested this with WordPress 5.3.

I'll work on the tests.

assets/css/src/amp-default.css 76:37 ✖ Expected double quotes string-quotes Simply change the ' to ".

claudiulodro

I noticed a couple small bugs in the implementation. In general, though, this works great with and without Gutenberg 6.5+!

claudiulodro · 2019-10-25T17:27:25Z

includes/sanitizers/class-amp-gallery-block-sanitizer.php

+				' or ',
+				[
+					'( parent::figure[ contains( @class, "wp-block-gallery" ) and @data-amp-carousel = "true" ] )',
+					'( contains( @class, "wp-block-gallery" ) and @data-amp-carousel = "true" )',


I believe you've lost some functionality with this change. Previously you could have the carousel "off" and lightbox "on" for the gallery, but this sanitizer will now ignore that setting combination.

claudiulodro · 2019-10-25T17:44:56Z

includes/sanitizers/class-amp-gallery-block-sanitizer.php

+			 * Element
+			 *
+			 * @var DOMElement $node
+			 */

 			$attributes      = AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node );
 			$is_amp_lightbox = isset( $attributes['data-amp-lightbox'] ) && true === filter_var( $attributes['data-amp-lightbox'], FILTER_VALIDATE_BOOLEAN );


Like $is_amp_carousel, $is_amp_lightbox also needs to check the parentNode to see if data-amp-lightbox is set (for Gutenberg 6.5+ compatibility). The simplest way of handling this situation for both $is_amp_carousel and $is_amp_lightbox might be something like this:

$attributes = array_merge( AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node ), AMP_DOM_Utils::get_node_attributes_as_assoc_array( $node->parentNode ) );

Hi @claudiulodro, thanks for pointing that out. I ended up doing something similar to that.

includes/sanitizers/class-amp-gallery-block-sanitizer.php

In that case, I think the sanitizer should still run. This could still use more testing.

Though maybe this isn't applied as intended.

kienstra · 2019-10-25T23:14:43Z

Hi @westonruter,
Thanks for your commits, and for testing this.

In my testing, the Gallery block is looking good with the following combinations:

Environment

With Gutenberg active (6.7.0, latest stable)
Without Gutenberg, running WP Core 5.2.4

Gallery Block Settings

Only lightbox
Only carousel
Lightbox and carousel
With and without captions

Though it looks like all Gallery blocks appear with a lightbox, whether it's selected or not. This issue also seems to appear on the develop branch and the last stable release (1.3.0 tag):

kienstra · 2019-10-25T23:20:31Z

I noticed a couple small bugs in the implementation. In general, though, this works great with and without Gutenberg 6.5+!

Hi @claudiulodro,
Thanks a lot for testing this. If you have time, it'd be great if you could test this again. This should now work when the carousel is off and the lightbox is on.

kienstra · 2019-10-25T23:33:20Z

One thing I missed is that this doesn't add captions to gallery shortcodes with 'Display as carousel' selected, like:

[gallery ids="4572, 3619"]

This PR now only addresses Gallery blocks.

claudiulodro

The latest version looks good. Thanks for making those revisions. 👍

westonruter · 2019-10-28T20:54:37Z

One thing I missed is that this doesn't add captions to gallery shortcodes with 'Display as carousel' selected

@kienstra So the only outstanding thing is classic gallery shortcode instances will continue to be rendered without captions? If that's the case, then I think we can merge this and then follow-up with another issue/PR to do the rest.

kienstra · 2019-10-28T20:59:33Z

Hi @westonruter,

@kienstra So the only outstanding thing is classic gallery shortcode instances will continue to be rendered without captions?

That's right, this PR doesn't affect the Gallery shortcode (or the Classic editor galleries, which use that shortcode).

westonruter · 2019-10-28T21:11:57Z

OK great, so [gallery] shortcode use will appear no different for users, while at the same time this change fixes the Gutenberg gallery block integration to ensure that the Carousel setting actually has an effect:

Non-AMP	AMP Before	AMP After

kienstra · 2019-10-28T21:40:24Z

@kienstra So the only outstanding thing is classic gallery shortcode instances will continue to be rendered without captions? If that's the case, then I think we can merge this and then follow-up with another issue/PR to do the rest.

Hi @westonruter,
Thanks a lot for reviewing this. Would it be alright if I opened an issue for [gallery] shortcode captions, then worked on a PR for that?

The support topic that prompted this PR's issue (#2855) looks to use a Gallery shortcode.

Thanks, Weston.

austintude · 2019-10-28T23:02:49Z

Great stuff y'all!!

…

On Mon, Oct 28, 2019, 4:40 PM Ryan Kienstra ***@***.***> wrote: @kienstra <https://github.com/kienstra> So the only outstanding thing is classic gallery shortcode instances will continue to be rendered without captions? If that's the case, then I think we can merge this and then follow-up with another issue/PR to do the rest. Hi @westonruter <https://github.com/westonruter>, Thanks a lot for reviewing this. Would it be alright if I opened an issue for [gallery] shortcode captions, then worked on a PR for that? The support topic <https://wordpress.org/support/topic/amp-carousel-with-captions/> that prompted this PR's issue (#2855 <#2855>) looks to use a Gallery shortcode. Thanks, Weston. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3285?email_source=notifications&email_token=AHC4FRO7U5TOAP34XCIVEGDQQ5L5BA5CNFSM4IXV6JCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECOPP3I#issuecomment-547157997>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHC4FROMU7YEYJOFROULDFDQQ5L5BANCNFSM4IXV6JCA> .

westonruter · 2019-10-29T00:15:07Z

@kienstra por supuesto. 👍

kienstra · 2019-10-29T00:36:24Z

Gracias!

Add caption support to the Gallery shortcode and block in <amp-carousel>

2fab3b9

For images in a gallery that have a caption, display them when in an <amp-carousel>.

googlebot added the cla: yes Signed the Google CLA label Sep 17, 2019

kienstra changed the title ~~Add caption support to the Gallery shortcode and block in <amp-carousel>~~ [WIP] Add caption support to the Gallery shortcode and block in <amp-carousel> Sep 17, 2019

Add a 'scrim' at the bottom of the carousel, using amp.dev as an exam…

0b0b2fd

…ple.

westonruter mentioned this pull request Sep 18, 2019

Add image captions to the gallery carousel #2855

Closed

westonruter added this to the v1.3.1 milestone Sep 18, 2019

kienstra added 2 commits September 19, 2019 16:11

Merge branch 'develop' into add/gallery-caption

c5f0040

Revert "Add a 'scrim' at the bottom of the carousel, using amp.dev as…

a6fec8c

… an example." This reverts commit 0b0b2fd.

swissspidy modified the milestones: v1.3.1, v1.4 Oct 17, 2019

kienstra added 3 commits October 24, 2019 16:05

Merge branch 'develop' into add/gallery-caption

af5b86d

Taking cues from the Newspack Carousel, improve captions

f034593

It looks like this is working well with the Core themes Twenty Fifteen through Twenty Nineteen.

Attempt to fix failed PHPUnit tests in PHP 5.5

e52a3ea

Instead of modifying the ->textContent property, create a new text node, and append it to the <span> node.

kienstra commented Oct 25, 2019

View reviewed changes

Rename $div to $slide, as it's more descriptive

19d8af7

It's actually a slide in the <amp-carousel> Also, add a comment, and remove an extra newline.

Move position: absolute to the top of the style rules

d4dd0a3

This is more important than text-align: center.

westonruter added 2 commits October 24, 2019 22:05

Fix gallery carousel in the latest Gutenberg (and WP 5.3)

03adbd9

Prevent rendering wp-block-gallery with display:flex

a202971

westonruter requested changes Oct 25, 2019

View reviewed changes

Address CSS linting issue

e8ffb78

assets/css/src/amp-default.css 76:37 ✖ Expected double quotes string-quotes Simply change the ' to ".

claudiulodro suggested changes Oct 25, 2019

View reviewed changes

westonruter reviewed Oct 25, 2019

View reviewed changes

includes/sanitizers/class-amp-gallery-block-sanitizer.php Outdated Show resolved Hide resolved

kienstra added 3 commits October 25, 2019 16:59

Account for galleries that only have a lightbox

7f7f5ca

In that case, I think the sanitizer should still run. This could still use more testing.

Refactor the contains() query with Weston's suggestion

9af22c8

Though maybe this isn't applied as intended.

Add a trailing comma to address a phpcs issue.

a19b89a

kienstra changed the title ~~Add caption support to the Gallery shortcode and block in <amp-carousel>~~ Add caption support to the Gallery block in <amp-carousel> Oct 25, 2019

Merge branch 'develop' into add/gallery-caption

0f0fd39

claudiulodro approved these changes Oct 28, 2019

View reviewed changes

westonruter approved these changes Oct 28, 2019

View reviewed changes

westonruter changed the title ~~Add caption support to the Gallery block in <amp-carousel>~~ Add caption support to the Gallery block in <amp-carousel>; fix displaying Gallery block as carousel in WP 5.3 Oct 28, 2019

westonruter changed the title ~~Add caption support to the Gallery block in <amp-carousel>; fix displaying Gallery block as carousel in WP 5.3~~ Add caption support to the Gallery block in <amp-carousel> and fix displaying Gallery block as carousel in WP 5.3 Oct 28, 2019

westonruter merged commit 7cae80e into develop Oct 28, 2019

westonruter deleted the add/gallery-caption branch October 28, 2019 21:17

This was referenced Oct 29, 2019

Add support for captions to the Gallery shortcode #3658

Closed

Enable captions on Gallery shortcodes #3659

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add caption support to the Gallery block in <amp-carousel> and fix displaying Gallery block as carousel in WP 5.3 #3285

Add caption support to the Gallery block in <amp-carousel> and fix displaying Gallery block as carousel in WP 5.3 #3285

kienstra commented Sep 17, 2019 •

edited

Loading

kienstra commented Sep 18, 2019

kienstra commented Sep 20, 2019 •

edited

Loading

kienstra commented Sep 23, 2019 •

edited

Loading

kienstra commented Sep 23, 2019

austintude commented Sep 28, 2019

westonruter commented Oct 1, 2019

kienstra commented Oct 1, 2019

austintude commented Oct 2, 2019 via email

kienstra commented Oct 2, 2019

westonruter commented Oct 8, 2019

kienstra commented Oct 8, 2019

kienstra commented Oct 25, 2019

kienstra Oct 25, 2019 •

edited

Loading

kienstra commented Oct 25, 2019 •

edited

Loading

kienstra commented Oct 25, 2019 •

edited

Loading

westonruter left a comment

kienstra commented Oct 25, 2019 •

edited

Loading

claudiulodro left a comment

claudiulodro Oct 25, 2019

claudiulodro Oct 25, 2019

kienstra Oct 25, 2019

kienstra commented Oct 25, 2019 •

edited

Loading

kienstra commented Oct 25, 2019

kienstra commented Oct 25, 2019 •

edited

Loading

claudiulodro left a comment

westonruter commented Oct 28, 2019

kienstra commented Oct 28, 2019

westonruter commented Oct 28, 2019

kienstra commented Oct 28, 2019

austintude commented Oct 28, 2019 via email

westonruter commented Oct 29, 2019

kienstra commented Oct 29, 2019

Add caption support to the Gallery block in <amp-carousel> and fix displaying Gallery block as carousel in WP 5.3 #3285

Add caption support to the Gallery block in <amp-carousel> and fix displaying Gallery block as carousel in WP 5.3 #3285

Conversation

kienstra commented Sep 17, 2019 • edited Loading

Update: this has changed a lot, please see more recent comments

Before

After

kienstra commented Sep 18, 2019

kienstra commented Sep 20, 2019 • edited Loading

Twenty Fifteen

Twenty Sixteen

Twenty Seventeen

Twenty Nineteen

kienstra commented Sep 23, 2019 • edited Loading

Before

After (with captions)

kienstra commented Sep 23, 2019

austintude commented Sep 28, 2019

westonruter commented Oct 1, 2019

kienstra commented Oct 1, 2019

austintude commented Oct 2, 2019 via email

kienstra commented Oct 2, 2019

westonruter commented Oct 8, 2019

kienstra commented Oct 8, 2019

kienstra commented Oct 25, 2019

kienstra Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

kienstra commented Oct 25, 2019 • edited Loading

Before

After

Steps to reproduce

kienstra commented Oct 25, 2019 • edited Loading

Steps to test

westonruter left a comment

Choose a reason for hiding this comment

kienstra commented Oct 25, 2019 • edited Loading

claudiulodro left a comment

Choose a reason for hiding this comment

claudiulodro Oct 25, 2019

Choose a reason for hiding this comment

claudiulodro Oct 25, 2019

Choose a reason for hiding this comment

kienstra Oct 25, 2019

Choose a reason for hiding this comment

kienstra commented Oct 25, 2019 • edited Loading

Environment

Gallery Block Settings

kienstra commented Oct 25, 2019

kienstra commented Oct 25, 2019 • edited Loading

claudiulodro left a comment

Choose a reason for hiding this comment

westonruter commented Oct 28, 2019

kienstra commented Oct 28, 2019

westonruter commented Oct 28, 2019

kienstra commented Oct 28, 2019

austintude commented Oct 28, 2019 via email

westonruter commented Oct 29, 2019

kienstra commented Oct 29, 2019

kienstra commented Sep 17, 2019 •

edited

Loading

kienstra commented Sep 20, 2019 •

edited

Loading

kienstra commented Sep 23, 2019 •

edited

Loading

kienstra Oct 25, 2019 •

edited

Loading

kienstra commented Oct 25, 2019 •

edited

Loading

kienstra commented Oct 25, 2019 •

edited

Loading

kienstra commented Oct 25, 2019 •

edited

Loading

kienstra commented Oct 25, 2019 •

edited

Loading

kienstra commented Oct 25, 2019 •

edited

Loading