Add lightbox support to Core Image blocks with links #3460

kienstra · 2019-10-08T02:43:11Z

Summary

As reported in #3450, there's an issue where images that are wrapped in <a> didn't support the lightbox feature. Clicking on them simply caused the browser to go to that link.

So this recognizes when an image is wrapped in <a>, and runs the lighbox logic for it.

Steps to reproduce:

Create a post
Add an Image block
Add any image to it
Click the 'link' icon, and click 'Media File' or 'Attachment Page':

Click 'Add lightbox effect'
Click 'Update,' and go to the front-end
Expected: Clicking that image causes the lightbox to display
Actual: it goes to the URL of the image:

Repeat all of the steps above, and after step 4, set the alignment of the image to 'Align Right':

The expected outcome is the same: clicking the image on the front-end causes the lightbox to display.

Fixes #3450

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

There was an issue where images that are wrapped in <a> didn't support the lightbox feature, as clicking on them simply caused the browser to go to that link. So this recognizes when an image is wrapped in <a>, and it strips that.

kienstra · 2019-10-10T21:09:17Z

Request To Test

Hi @rsmith4321,
Hope you've had a great week.

When you have a chance, could you please test this PR?

It should fix your Issue #3450, and shouldn't require the workaround of removing the Image block's link.

Thanks, Ryan!

westonruter · 2019-10-11T10:19:25Z

includes/sanitizers/class-amp-img-sanitizer.php

@@ -393,13 +398,18 @@ private function maybe_add_lightbox_attributes( $attributes, $node ) {
 			$attributes['tabindex']          = 0;

 			$this->maybe_add_amp_image_lightbox_node();
+
+			if ( $is_node_wrapped_in_anchor ) {
+				// Remove the <a> that the image is wrapped in, as it can prevent the lightbox from working.


Did you check if this issue has been reported in AMP? Perhaps it is a bug in AMP itself that should be fixed?

Ah, that's a good point. It is a little strange that the plugin has to remove the <a>.

I couldn't find an issue in http://github.com/ampproject/amphtml for this, but I'll keep looking.

I also might open an issue in amphtml for this.

I opened an issue in amphtml: ampproject/amphtml#25021

schlessera · 2019-10-12T05:52:25Z

Code looks good to me. However, we should wait for feedback on ampproject/amphtml#25021 before proceeding.

kienstra · 2019-10-12T17:48:31Z

Code looks good to me. However, we should wait for feedback on ampproject/amphtml#25021 before proceeding.

Nice, thanks!

It sounds like this is the recommended component for lightboxes, not the previous amp-image-ligthbox.

kienstra · 2019-10-15T22:06:36Z

includes/sanitizers/class-amp-img-sanitizer.php

+			if ( $is_node_wrapped_in_anchor ) {
+				// Remove the <a> that the image is wrapped in, as it can prevent the lightbox from working.
+				$node->parentNode->parentNode->replaceChild( $node, $node->parentNode );
+			}


If amphtml later supports wrapping <amp-img lightbox> in <a>, this can be removed.

kienstra · 2019-10-15T22:26:13Z

This PR is now changing the Core Image block from using the <amp-image-lightbox> to the recommended <amp-lightbox-gallery>. It looks like <amp-image-lightbox> will eventually be deprecated.

It looks like this works well so far. The only change I noticed is that all <amp-img lightbox> elements will be visible in the lightbox:

If the amphtml feature is added, the code can probably be removed.

westonruter · 2019-10-16T00:00:02Z

@rsmith4321 Would you test this please? Here's a build for testing: amp.zip

rsmith4321 · 2019-10-16T03:46:16Z

I tested it and it works well. I think it's kind of nice to be able to cycle to other images on the page in the lightbox even though they aren't contained in a gallery. It still has the same issue of the lightbox not opening if it's linked to the media file surrounded by an <a> tag.

…

On Tue, Oct 15, 2019 at 8:00 PM Weston Ruter ***@***.***> wrote: @rsmith4321 <https://github.com/rsmith4321> Would you test this please? Here's a build for testing: amp.zip <https://github.com/ampproject/amp-wp/files/3731846/amp.zip> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3460?email_source=notifications&email_token=AB2Y5H73RBSOFVSMRKYSE6LQOZKQTA5CNFSM4I6MEMZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBKSYAQ#issuecomment-542452738>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB2Y5H2ZNCTRFLB6KGKPR5DQOZKQTANCNFSM4I6MEMZA> .

westonruter · 2019-10-16T16:33:15Z

It still has the same issue of the lightbox not opening if it's linked to the media file surrounded by an tag.

I'm confused. @kienstra I thought this is what the $is_node_wrapped_in_anchor was for?

kienstra · 2019-10-16T16:36:01Z

@westonruter, right, this PR should allow the image to be wrapped in a <a>. Let me look at this more.

westonruter · 2019-10-16T16:37:47Z

If the link is for the media file itself, then I think it is correct to remove the hyperlink in favor of the lightbox alone, since clicking the link would just cause the image to be loaded on a separate page; here a lightbox is a better experience since you don't leave the page context.

If, however, the link is for the attachment page, then in this case I think it is better to retain the link.

kienstra · 2019-10-16T16:45:37Z

If the link is for the media file itself, then I think it is correct to remove the hyperlink in favor of the lightbox alone, since clicking the link would just cause the image to be loaded on a separate page; here a lightbox is a better experience since you don't leave the page context.

If, however, the link is for the attachment page, then in this case I think it is better to retain the link.

Sounds good, I'll work on applying that.

kienstra · 2019-10-16T16:47:15Z

Hi @rsmith4321,

It still has the same issue of the lightbox not opening if it's linked to the media file surrounded by an tag.

Thanks for testing this!

Is that on a public site, by chance, and could you share a link to it?

That might help to reproduce it.

Locally, when linking to the Media File and selecting the lightbox, the lightbox works:

Thanks again, Ryan! You've been really helpful in testing everything.

rsmith4321 · 2019-10-16T19:07:43Z

Yes, on this page https://www.ryansmithphotography.com/myrtle-beach-photo-booths/ the three images at the top have the lightbox selected on but I removed the link to the media files. The images further down the page I switched the lightbox on but they still have the link to the media file and they don't open in a lightbox.

…

On Wed, Oct 16, 2019 at 12:47 PM Ryan Kienstra ***@***.***> wrote: Hi @rsmith4321 <https://github.com/rsmith4321>, It still has the same issue of the lightbox not opening if it's linked to the media file surrounded by an tag. Thanks for testing this! Is that on a live site, by chance, and could you share a link to it? That might help to reproduce it. Locally, when linking to the Media File and selecting the lightbox, the lightbox works: [image: ligthbox-media] <https://user-images.githubusercontent.com/4063887/66940416-87c9cf80-f00a-11e9-91aa-2978c321b1f3.gif> Thanks again, Ryan! You've been really helpful in testing everything. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3460?email_source=notifications&email_token=AB2Y5H5HSPT37JDTMGVZ2PDQO5ARXA5CNFSM4I6MEMZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBNFRQY#issuecomment-542791875>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB2Y5H73S2PBKFUEGIJO5CLQO5ARXANCNFSM4I6MEMZA> .

…page As Weston mentioned, the lightbox is a better experience than simply going to the URL of the media file. But if it's for the attachment page, don't add the lightbox attribute, and don't strip the <a>.

kienstra · 2019-10-17T03:30:24Z

Thanks, I also see the issue with the lightbox not working.

In the link provided, it looks like the lightbox doesn't work when it's for an Image block that has a link and alignment:

Image blocks with alignment are wrapped in an extra <div>. The logic accounts for that extra div, or an extra <a> from the link, but not both.

I'll work on this.

Before, there was handling for links or alignment, but not both. So add a way to get the great-grandparent of the <img>.

This doesn't pass without passing the object to it.

It looks like it won't be implemented for backwards-compatibility reasons.

This caused an error in PHP 5.4 and 5.5.

kienstra · 2019-10-18T00:56:24Z

Request To Test

Hi @rsmith4321,
Thanks for your patience. Could you please test this amp.zip, which should fix the issue with lightboxes on the URL you provided?

This shouldn't be an issue on that URL you shared, but the lightbox will only work if the link is to the 'Media File,' not to the 'Attachment Page':

...though on that URL you provided, it looks like the URLs are to the 'Media File,' so they should be fine.

Thanks, Ryan! You've been really helpful in reporting and testing these issues. Have a great weekend.

includes/sanitizers/class-amp-img-sanitizer.php

kienstra · 2019-10-18T00:59:22Z

includes/sanitizers/class-amp-img-sanitizer.php

-			$classes = preg_split( '/\s+/', $grand_parent->getAttribute( 'class' ) );
-			if ( in_array( 'wp-block-image', $classes, true ) ) {
-				$parent_node = $grand_parent;
-			}


The logic above is mainly copied into does_node_have_block_class()

rsmith4321 · 2019-10-18T20:37:03Z

No problem. Yes you wouldn't want a lightbox if the image is an link to another page and not to the media file so that seems like the correct behavior. I tried the latest build you provided and I'm still seeing the same results on that page.

…

On Thu, Oct 17, 2019 at 8:56 PM Ryan Kienstra ***@***.***> wrote: *Request To Test* Hi @rsmith4321 <https://github.com/rsmith4321>, Thanks for your patience. Could you please test this amp.zip <https://github.com/ampproject/amp-wp/files/3741812/amp.zip>, which should fix the issue with lightboxes on the URL you provided <https://www.ryansmithphotography.com/myrtle-beach-photo-booths/>? This shouldn't be an issue on that URL you shared, but the lightbox will only work if the link is to the 'Media File,' not to the 'Attachment Page': [image: link-to-attachment-page] <https://user-images.githubusercontent.com/4063887/67057580-b2a14a00-f116-11e9-96ae-ac56c92ed708.png> ...though on that URL you provided <https://www.ryansmithphotography.com/myrtle-beach-photo-booths/>, it looks like the URLs are to the 'Media File,' so they should be fine. Thanks, Ryan! You've been really helpful in reporting and testing these issues. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3460?email_source=notifications&email_token=AB2Y5HYYQY6NOE5RTRW6EZ3QPECUBA5CNFSM4I6MEMZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBSB76Y#issuecomment-543432699>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB2Y5HYFO57ARVAW6P4E52LQPECUBANCNFSM4I6MEMZA> .

kienstra · 2019-10-18T20:57:37Z

Thanks, Ryan! Right, it does look like the issue still exists on that URL. I'll look at that.

kienstra · 2019-10-18T21:37:07Z

Maybe attachment_url_to_postid isn't returning the ID, as expected. Though I couldn't reproduce this locally.

westonruter · 2019-10-18T22:03:16Z

includes/sanitizers/class-amp-img-sanitizer.php

+			&&
+			( 'figure' === $parent_node->tagName || 'figure' === $parent_node->parentNode->tagName )
+			&&
+			attachment_url_to_postid( $parent_node->getAttribute( 'href' ) )


From what I recall, attachment_url_to_postid() is not super reliable. I think it would be better to look at the URL to see if it looks like a file instead. For example:

$is_file_url = preg_match( '/\.\w+$/', wp_parse_url( $parent_node->getAttribute( 'href' ), PHP_URL_PATH ) );

It's applied in 783a8d8

Use the line that Weston suggested, instead of attachment_url_to_postid().

kienstra · 2019-10-18T23:02:41Z

Hi @rsmith4321,
Sorry to keep sending you plugin builds to test.

But here's another amp.zip when you have a chance.

Thanks a lot for your help.

rsmith4321 · 2019-10-21T02:22:55Z

Yes the latest build seems to fix the issue for me, you can check out that same page now I still have some of the images linked to the media file and it seems to strip that out for the lightbox.

…

On Fri, Oct 18, 2019 at 7:02 PM Ryan Kienstra ***@***.***> wrote: Hi @rsmith4321 <https://github.com/rsmith4321>, Sorry to keep sending you plugin builds to test. But here's another amp.zip <https://github.com/ampproject/amp-wp/files/3745901/amp.zip> when you have a chance. Thanks a lot for your help. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3460?email_source=notifications&email_token=AB2Y5H5B6K4LK25YS3ZX5HTQPI6BRA5CNFSM4I6MEMZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBWLOHA#issuecomment-543995676>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB2Y5H2UJQLA3DZF275ZBODQPI6BRANCNFSM4I6MEMZA> .

rsmith4321 · 2019-10-21T02:35:09Z

The only thing I notice on a mouse hover the cursor is not changed to a pointer because it's not a link anymore. I would recommend if you can add that so on desktop there is an indication the images can be opened in the lightbox. I'm not sure how to target an image with the data-amp-lightbox="true" because it's not a regular class so I can't just add this to the css.

…

On Sun, Oct 20, 2019 at 10:22 PM Ryan Smith ***@***.***> wrote: Yes the latest build seems to fix the issue for me, you can check out that same page now I still have some of the images linked to the media file and it seems to strip that out for the lightbox. On Fri, Oct 18, 2019 at 7:02 PM Ryan Kienstra ***@***.***> wrote: > Hi @rsmith4321 <https://github.com/rsmith4321>, > Sorry to keep sending you plugin builds to test. > > But here's another amp.zip > <https://github.com/ampproject/amp-wp/files/3745901/amp.zip> when you > have a chance. > > Thanks a lot for your help. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#3460?email_source=notifications&email_token=AB2Y5H5B6K4LK25YS3ZX5HTQPI6BRA5CNFSM4I6MEMZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBWLOHA#issuecomment-543995676>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AB2Y5H2UJQLA3DZF275ZBODQPI6BRANCNFSM4I6MEMZA> > . >

swissspidy · 2019-10-21T10:32:20Z

Glad to hear it's working!

Personally I don't think the AMP plugin should change the cursor as it's the theme's responsibility.

You should be able to use the following CSS in your theme to change the cursor:

[data-amp-lightbox="true"] { cursor: pointer; }

kienstra · 2019-10-21T16:03:04Z

Hi @rsmith4321,
Thanks a lot for testing this so many times. It's good to hear it's working.

The only thing I notice on a mouse hover the cursor is not changed to a
pointer because it's not a link anymore.

Sorry, it would have been ideal if it could have still been wrapped in a link. We ended up having to strip.

The styling above should work, let me know if there's an issue.

kienstra · 2019-10-21T16:07:23Z

Request For Final Review

Hi @westonruter,
Happy Monday!

When you have a chance, could you please review this PR again?

In addition to the testing steps above, this should also work when adding alignment to the image, like alignright, after testing step 4.

Thanks, Weston!

includes/sanitizers/class-amp-img-sanitizer.php

rsmith4321 · 2019-10-21T18:03:51Z

Glad to hear it's working!

Personally I don't think the AMP plugin should change the cursor as it's the theme's responsibility.

You should be able to use the following CSS in your theme to change the cursor:
[data-amp-lightbox="true"] { cursor: pointer; }

Thanks I'll use that. I would agree except you are doing something unusual by stripping the link. Normally a lightbox would open from a link to the image so the cursor would change. So I don't think any theme is going to ever know to add that css.

kienstra · 2019-10-21T18:17:53Z

Thanks I'll use that. I would agree except you are doing something unusual by stripping the link.

Yeah, that's a good point. We tried to get around stripping the link, but the amphtml project couldn't change the behavior, due to backwards-compatibility.

Allow images that are wrapped in <a> to have a lightbox

c4dd0ec

There was an issue where images that are wrapped in <a> didn't support the lightbox feature, as clicking on them simply caused the browser to go to that link. So this recognizes when an image is wrapped in <a>, and it strips that.

googlebot added the cla: yes Signed the Google CLA label Oct 8, 2019

kienstra mentioned this pull request Oct 8, 2019

Add lightbox effect doesn't work for image blocks #3450

Closed

swissspidy requested review from schlessera and westonruter October 10, 2019 22:41

westonruter reviewed Oct 11, 2019

View reviewed changes

schlessera approved these changes Oct 12, 2019

View reviewed changes

Use amp-lightbox-gallery for Core Image block

237cde9

It sounds like this is the recommended component for lightboxes, not the previous amp-image-ligthbox.

kienstra commented Oct 15, 2019

View reviewed changes

Add a comment that code can be removed if an issue is implemented

9ee7d2e

If the amphtml feature is added, the code can probably be removed.

westonruter added this to the v1.3.1 milestone Oct 15, 2019

swissspidy approved these changes Oct 16, 2019

View reviewed changes

Only remove the <a> if it goes to the media file, not the attachment …

9c7fd7b

…page As Weston mentioned, the lightbox is a better experience than simply going to the URL of the media file. But if it's for the attachment page, don't add the lightbox attribute, and don't strip the <a>.

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

kienstra added 2 commits October 17, 2019 17:39

Allow lightboxes in Image blocks with links and alignment

9a08c8a

Before, there was handling for links or alignment, but not both. So add a way to get the great-grandparent of the <img>.

Add a parameter to the invoke() method in a test

88b8efc

This doesn't pass without passing the object to it.

kienstra added 2 commits October 17, 2019 18:00

Remove the link to the amphtml issue, as it's closed

bd1906b

It looks like it won't be implemented for backwards-compatibility reasons.

Access a DOMNodeList item with ->item() instead of [0]

d243326

This caused an error in PHP 5.4 and 5.5.

kienstra commented Oct 18, 2019

View reviewed changes

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

kienstra commented Oct 18, 2019

View reviewed changes

westonruter reviewed Oct 18, 2019

View reviewed changes

Use Weston's method of finding if the link is to a file

783a8d8

Use the line that Weston suggested, instead of attachment_url_to_postid().

kienstra assigned westonruter Oct 21, 2019

westonruter approved these changes Oct 21, 2019

View reviewed changes

includes/sanitizers/class-amp-img-sanitizer.php Show resolved Hide resolved

westonruter merged commit 468b349 into develop Oct 21, 2019

westonruter deleted the update/3450-lightbox-anchor branch October 21, 2019 16:14

swissspidy mentioned this pull request Oct 21, 2019

Disable lightbox toggle for image linking to attachment pages #3579

Closed

Add lightbox support to Core Image blocks with links #3460

Add lightbox support to Core Image blocks with links #3460

Conversation

kienstra commented Oct 8, 2019 • edited by westonruter Loading

Summary

Checklist

kienstra commented Oct 10, 2019

westonruter Oct 11, 2019

Choose a reason for hiding this comment

kienstra Oct 11, 2019

Choose a reason for hiding this comment

kienstra Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

kienstra Oct 11, 2019

Choose a reason for hiding this comment

kienstra Oct 12, 2019

Choose a reason for hiding this comment

schlessera commented Oct 12, 2019

kienstra commented Oct 12, 2019

kienstra Oct 15, 2019

Choose a reason for hiding this comment

kienstra commented Oct 15, 2019 • edited Loading

westonruter commented Oct 16, 2019

rsmith4321 commented Oct 16, 2019 via email

westonruter commented Oct 16, 2019

kienstra commented Oct 16, 2019

westonruter commented Oct 16, 2019

kienstra commented Oct 16, 2019

kienstra commented Oct 16, 2019 • edited Loading

rsmith4321 commented Oct 16, 2019 via email

kienstra commented Oct 17, 2019

kienstra commented Oct 18, 2019 • edited Loading

kienstra Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

rsmith4321 commented Oct 18, 2019 via email

kienstra commented Oct 18, 2019

kienstra commented Oct 18, 2019

westonruter Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

kienstra Oct 18, 2019

Choose a reason for hiding this comment

kienstra Oct 18, 2019

Choose a reason for hiding this comment

kienstra commented Oct 18, 2019

rsmith4321 commented Oct 21, 2019 via email

rsmith4321 commented Oct 21, 2019 via email

swissspidy commented Oct 21, 2019

kienstra commented Oct 21, 2019

kienstra commented Oct 21, 2019

rsmith4321 commented Oct 21, 2019

kienstra commented Oct 21, 2019

kienstra commented Oct 8, 2019 •

edited by westonruter

Loading

kienstra Oct 11, 2019 •

edited

Loading

kienstra commented Oct 15, 2019 •

edited

Loading

kienstra commented Oct 16, 2019 •

edited

Loading

kienstra commented Oct 18, 2019 •

edited

Loading

kienstra Oct 18, 2019 •

edited

Loading

westonruter Oct 18, 2019 •

edited

Loading