Carousel: improve gallery parsing #13554

jeherve · 2019-09-26T17:01:57Z

Fixes #13531
Fixes #13428
Fixes #11191
Fixes #17090

Changes proposed in this Pull Request:

This is still a work in progress. I'd like to add Unit Tests to cover each possible use of Carousel, as detailed in the testing instructions below.

Parsing post content to insert carousel metadata is a bit fragile at the moment. Depending on the format of your images, how they are inserted, and what metadata is available for each image, you may run into several issues, highlighted in the issues above.

This PR introduces a different approach to add the Carousel metadata to the classic galleries (it hooks into the do_shortcode_tag filter instead of gallery_style).
It also takes on @mdawaffe's approach from #13547 to improve our use of DOMDocument.
It also fixes PHPCS errors to get things to be more readable as I was going through that file.

Jetpack product discussion

N/A

Does this pull request change what data or activity we track or use?

No

Testing instructions:

In new posts, insert a variety of blocks:
- A classic block with a gallery
- A classic block with a tiled gallery
- A classic block with a single image
- An image block
- A gallery block
- A Tiled Gallery block
- A column block with some text
- A column block with a gallery block in it.
Publish all those posts
When moving your mouse over each image, make sure the cursor only becomes a pointer when the element can be expanded to a Carousel modal.
Carousel should work well for all those blocks.
You should not see any JavaScript error.

jetpackbot · 2019-09-26T17:04:08Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: November 5, 2019.
Scheduled code freeze: October 29, 2019

Generated by 🚫 dangerJS against a1a13e8

mdawaffe · 2019-09-26T19:39:38Z

I haven't checked, but I think the problems from #13547 (comment) are the same in this PR as they were in the other.

modules/carousel/jetpack-carousel.php

See #13554 (comment)

… data (#13446)" This reverts commit a7e494e. Following issues with this implementation, let's go back for now and revisit in #13554

* Revert "Carousel: improve detection of containers where we should add data (#13446)" This reverts commit a7e494e. Following issues with this implementation, let's go back for now and revisit in #13554 * Remove entry from changelog

See #13554 (comment)

stale · 2020-01-21T12:18:40Z

This PR has been marked as stale. This happened because:

It has been inactive in the past 3 months.
It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

stale · 2020-06-17T03:50:27Z

This PR has been marked as stale. This happened because:

It has been inactive in the past 3 months.
It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

- Remove in_gallery; it hasn't been used since #5469 - add comments, docblocks, and fix various PHPCS errors. - use the_content to inspect all posts for galleries In the past we also relied on the gallery_style filter, which while it works, only relies on the gallery's opening div tag. Since we're already examining the post content for gallery blocks, let's extend that to check for old-style galleries as well. - stop saveHTML from adding unneeded doctype and html tags - improve DOM Parsing - Wrap input to `DOMDocument->loadHTML()` in fake root tag. - Specify blog's charset in input to `DOMDocument->loadHTML()`. - filter final output of gallery shortcode to add metadata This replaces the 2 other approaches we had tried: - Before this PR, we relied on the gallery_style filter. This was not ideal as the filter contains only the opening part of the div tag. - Earlier in this PR I tried to rely on the_content, but at that point the gallery shortcode is still just a shortcode, so we can't add metadata yet. - Add first set of unit tests

github-actions · 2021-05-20T18:51:53Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

Next scheduled release: June 1, 2021.
Scheduled code freeze: May 24, 2021.

zinigor · 2021-06-01T19:50:53Z

projects/plugins/jetpack/modules/carousel/jetpack-carousel.php

-				$html = str_replace( '<ul class="blocks-gallery-grid', '<ul ' . esc_attr( $data_key ) . "='" . wp_json_encode( $data_values ) . "' class=\"blocks-gallery-grid", $html );
-				$html = str_replace( '<figure class="wp-block-gallery blocks-gallery-grid', '<figure ' . esc_attr( $data_key ) . "='" . wp_json_encode( $data_values ) . "' class=\"wp-block-gallery  blocks-gallery-grid", $html );
+				// Do not go any further if DOMDocument is disabled on the server.
+				if ( ! class_exists( 'DOMDocument' ) ) {


Do you think it makes sense to return before foreach? Maybe even before anything else is checked to save us an autoloader roundtrip.

zinigor · 2021-06-01T19:57:52Z

Great stuff, sorry, couldn't help myself and did a drive-by review. My only other comment about the code is that I like your intention to add more stuff to test with. It would be great to test with some gibberish too, since the HTML can be put through any sort of filter before it gets to the carousel. So that if DOMDocument freaks out, we can fail gracefully.

jeherve · 2022-01-31T17:05:56Z

Closing this for now, since I haven't been able to work on it in the past few months.

jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Carousel A fullscreen modal appearing when clicking on an image in a gallery or tiled gallery. [Status] In Progress [Pri] High labels Sep 26, 2019

jeherve added this to the 7.8 milestone Sep 26, 2019

jeherve requested a review from a team September 26, 2019 17:01

jeherve self-assigned this Sep 26, 2019

matticbot added the Touches WP.com Files label Sep 26, 2019

mdawaffe reviewed Sep 26, 2019

View reviewed changes

modules/carousel/jetpack-carousel.php Outdated Show resolved Hide resolved

jeherve changed the title ~~Update/carousel add metadata~~ Carousel: improve gallery parsing Sep 27, 2019

jeherve added a commit that referenced this pull request Sep 27, 2019

Remove settings encoding now that it's set in the doctype.

09b3d5b

See #13554 (comment)

jeherve added a commit that referenced this pull request Sep 27, 2019

Revert "Carousel: improve detection of containers where we should add…

a7a529d

… data (#13446)" This reverts commit a7e494e. Following issues with this implementation, let's go back for now and revisit in #13554

jeherve mentioned this pull request Sep 27, 2019

Revert Carousel: improve detection of containers #13564

Merged

jeherve modified the milestones: 7.8, 7.9 Sep 27, 2019

jeherve mentioned this pull request Sep 27, 2019

Carousel: Improve DOM Parsing #13547

Closed

jeherve added a commit that referenced this pull request Oct 1, 2019

Remove settings encoding now that it's set in the doctype.

a6deed9

See #13554 (comment)

jeherve force-pushed the update/carousel-add-metadata branch from 98db791 to a1a13e8 Compare October 1, 2019 09:59

jeherve removed this from the 7.9 milestone Oct 23, 2019

jeherve added [Pri] Normal and removed [Pri] High labels Oct 23, 2019

stale bot added the [Status] Stale label Jan 21, 2020

stale bot removed the [Status] Stale label Mar 17, 2020

stale bot added the [Status] Stale label Jun 17, 2020

jeherve force-pushed the update/carousel-add-metadata branch from a1a13e8 to aad61bd Compare May 20, 2021 15:21

Add changelog

aa6e98f

jeherve removed the [Status] Stale label May 20, 2021

github-actions bot added [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Unit Tests labels May 20, 2021

zinigor reviewed Jun 1, 2021

View reviewed changes

jeherve closed this Jan 31, 2022

github-actions bot removed the [Status] In Progress label Jan 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Carousel: improve gallery parsing #13554

Carousel: improve gallery parsing #13554

jeherve commented Sep 26, 2019 •

edited

Loading

jetpackbot commented Sep 26, 2019 •

edited

Loading

mdawaffe commented Sep 26, 2019

stale bot commented Jan 21, 2020

stale bot commented Jun 17, 2020

github-actions bot commented May 20, 2021 •

edited

Loading

zinigor Jun 1, 2021

zinigor commented Jun 1, 2021

jeherve commented Jan 31, 2022

Carousel: improve gallery parsing #13554

Carousel: improve gallery parsing #13554

Conversation

jeherve commented Sep 26, 2019 • edited Loading

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

jetpackbot commented Sep 26, 2019 • edited Loading

mdawaffe commented Sep 26, 2019

stale bot commented Jan 21, 2020

stale bot commented Jun 17, 2020

github-actions bot commented May 20, 2021 • edited Loading

zinigor Jun 1, 2021

Choose a reason for hiding this comment

zinigor commented Jun 1, 2021

jeherve commented Jan 31, 2022

jeherve commented Sep 26, 2019 •

edited

Loading

jetpackbot commented Sep 26, 2019 •

edited

Loading

github-actions bot commented May 20, 2021 •

edited

Loading