Carousel: improve detection of containers where we should add data #13446

jeherve · 2019-09-06T16:55:21Z

Changes proposed in this Pull Request:

Until now we would rely on 2 str_replace to add Carousel data to div and ul containers in post content.
This worked, but wasn't smart enough to know to ignore specific divs like Columns blocks.

By using DOMDocument, we can go through the post content in a more readable way, to detect gallery blocks as well as regular galleries and images.

Testing instructions:

In a new post, 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 (still testing, but I think this may not work, neither with my branch or on the latest stable).
- An image block (still testing, but I think this may not work, neither with my branch or on the latest stable).
- A gallery block
- A Tiled Gallery block
- A column block with some text
- A column block with a gallery block in it.
Publish your post
When moving your mouse over each block, make sure the cursor only becomes a pointer when the element can be expanded to a Carousel modal.

Proposed changelog entry for your changes:

Carousel: improve detection of containers where we should add the Carousel modal.

matticbot · 2019-09-06T16:55:27Z

Caution: This PR has changes that must be merged to WordPress.com
Hello jeherve! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D32610-code before merging this PR. Thank you!

jetpackbot · 2019-09-06T16:57:25Z

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: October 1, 2019.
Scheduled code freeze: September 24, 2019

Generated by 🚫 dangerJS against 6c293bb

Fixes #13428 Until now we would rely on 2 str_replace to add Carousel data to div and ul containers in post content. This worked, but wasn't smart enough to know to ignore specific divs like Columns blocks. By using DOMDocument, we can go through the post content in a more readable way, to detect gallery blocks as well as regular galleries and images.

matticbot · 2019-09-06T17:05:59Z

jeherve, Your synced wpcom patch D32610-code has been updated.

modules/carousel/jetpack-carousel.php

matticbot · 2019-09-06T17:08:10Z

jeherve, Your synced wpcom patch D32610-code has been updated.

mdawaffe

I don't think it strictly matters for this use case, but we should wrap the DOM usage in:

$old_libxml_disable_entity_loader = libxml_disable_entity_loader( true );
// … DOM use
libxml_disable_entity_loader( $old_libxml_disable_entity_loader );

modules/carousel/jetpack-carousel.php

mdawaffe · 2019-09-06T18:56:32Z

modules/carousel/jetpack-carousel.php

+				foreach ( $div_tags as $div_tag ) {
+					if (
+						false === strpos( $div_tag->getAttribute( 'class' ), 'wp-block-' )
+						|| false !== strpos( $div_tag->getAttribute( 'class' ), 'gallery' )


Should this be &&?

As written, we add the attribute to all divs that are not wp-block- divs (including divs with no class at all).

That's the current behaviour, I didn't want to change that. We currently add the attributes to all divs. With this change, it will be added to all divs, except if they include a class including wp-block-. The second parameter is here to add an additional exception: wp-block- is okay if it's a gallery block, like wp-block-jetpack-tiled-gallery.

Maybe there would be a more readable way to do that? Or should we instead move to more of a whitelist where we don't add the data to all the divs, but only to some specific ones?

I'd support keeping the behavior the way it is for the sake of the PR, but open an enhancement issue to explore refactoring it.

Logged in #13499

modules/carousel/jetpack-carousel.php

See https://github.com/Automattic/jetpack/pull/13446/files#r321866447 Co-Authored-By: Michael D Adams <michael.d.adams@gmail.com>

matticbot · 2019-09-09T06:39:32Z

jeherve, Your synced wpcom patch D32610-code has been updated.

matticbot · 2019-09-09T06:44:24Z

jeherve, Your synced wpcom patch D32610-code has been updated.

See https://github.com/Automattic/jetpack/pull/13446/files#r321867238 Co-Authored-By: Michael D Adams <michael.d.adams@gmail.com>

matticbot · 2019-09-09T06:45:19Z

jeherve, Your synced wpcom patch D32610-code has been updated.

See #13446 (review)

matticbot · 2019-09-09T06:50:12Z

jeherve, Your synced wpcom patch D32610-code has been updated.

I've added the libxml_disable_entity_loader check as well 👍

Done!

kraftbj

I did some performance testing locally of the the_content filter before and after with a very large post (105k words, ~750k characters). There didn't seem to be a functional difference between them.

* Changelog: initial set of changes for 7.8 * Changelog: add #13310 * Changelog: add #13103 * Changelog: add #13426 * Changelog: add #13389 * Changelog: add #13449 * Changelog: add #13461 * Changelog: add #13460 * Changelog: add #13441 * Changelog: add #13454 * Changelog: add #13457 * Changelog: add #13425 * Changelog: add #13473 * Changelog: add #13355 * Changelog: add #13451 * Changelog: add #13358 * Changelog: add #13464 * Changelog: add #13416 * Changelog: add #13494 * Changelog: add #13465 * Changelog: add #13424 * Changelog: add #13432 * Changelog: add #13471 * Changelog: add 7.7.2 entry * Changelog: add #13446 * Add more testing elements

…uld add data (#13446) * Carousel: improve detection of containers where we should add data Fixes #13428 Until now we would rely on 2 str_replace to add Carousel data to div and ul containers in post content. This worked, but wasn't smart enough to know to ignore specific divs like Columns blocks. By using DOMDocument, we can go through the post content in a more readable way, to detect gallery blocks as well as regular galleries and images. * Bail if the server does not support DOMDocument * Store and return value from libxml_use_internal_errors See https://github.com/Automattic/jetpack/pull/13446/files#r321866447 Co-Authored-By: Michael D Adams <michael.d.adams@gmail.com> * No need to escape data key See https://github.com/Automattic/jetpack/pull/13446/files#r321867238 Co-Authored-By: Michael D Adams <michael.d.adams@gmail.com> * Wrap DOM usage in libxml_disable_entity_loader See #13446 (review)

jeherve · 2019-09-24T16:06:30Z

Cherry-picked to branch-7.8 in 7e44a01

… 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

jeherve added this to the 7.8 milestone Sep 6, 2019

jeherve requested a review from a team September 6, 2019 16:55

jeherve self-assigned this Sep 6, 2019

matticbot added Touches WP.com Files labels Sep 6, 2019

jeherve force-pushed the fix/carousel-on-columns-13428 branch from 474d368 to ebb2a70 Compare September 6, 2019 17:05

kraftbj previously requested changes Sep 6, 2019

View reviewed changes

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

Bail if the server does not support DOMDocument

fef2a1a

mdawaffe previously requested changes Sep 6, 2019

View reviewed changes

Store and return value from libxml_use_internal_errors

ef5cb8a

See https://github.com/Automattic/jetpack/pull/13446/files#r321866447 Co-Authored-By: Michael D Adams <michael.d.adams@gmail.com>

No need to escape data key

b2a0560

See https://github.com/Automattic/jetpack/pull/13446/files#r321867238 Co-Authored-By: Michael D Adams <michael.d.adams@gmail.com>

jeherve force-pushed the fix/carousel-on-columns-13428 branch from 905aac7 to b2a0560 Compare September 9, 2019 06:45

Wrap DOM usage in libxml_disable_entity_loader

6c293bb

See #13446 (review)

jeherve requested a review from mdawaffe September 9, 2019 06:50

jeherve requested a review from kraftbj September 9, 2019 06:51

jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Sep 19, 2019

jeherve mentioned this pull request Sep 20, 2019

Carousel: only add Carousel data to divs that need it #13499

Closed

kraftbj approved these changes Sep 24, 2019

View reviewed changes

jeherve merged commit a7e494e into master Sep 24, 2019

jeherve deleted the fix/carousel-on-columns-13428 branch September 24, 2019 07:25

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 24, 2019

jeherve added [Status] Needs Testing We need to add this change to the testing call for this month's release [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Sep 24, 2019

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

Changelog: add #13446

316678f

jeherve mentioned this pull request Sep 24, 2019

Carousel: issues modifying markup in some environments #13531

Open

mdawaffe mentioned this pull request Sep 26, 2019

Carousel: Improve DOM Parsing #13547

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Carousel: improve detection of containers where we should add data #13446

Carousel: improve detection of containers where we should add data #13446

jeherve commented Sep 6, 2019 •

edited

Loading

matticbot commented Sep 6, 2019

jetpackbot commented Sep 6, 2019 •

edited

Loading

matticbot commented Sep 6, 2019

matticbot commented Sep 6, 2019

mdawaffe left a comment

mdawaffe Sep 6, 2019

jeherve Sep 9, 2019

kraftbj Sep 19, 2019

jeherve Sep 20, 2019

matticbot commented Sep 9, 2019

matticbot commented Sep 9, 2019

matticbot commented Sep 9, 2019

matticbot commented Sep 9, 2019

kraftbj left a comment

jeherve commented Sep 24, 2019

Carousel: improve detection of containers where we should add data #13446

Carousel: improve detection of containers where we should add data #13446

Conversation

jeherve commented Sep 6, 2019 • edited Loading

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

matticbot commented Sep 6, 2019

jetpackbot commented Sep 6, 2019 • edited Loading

matticbot commented Sep 6, 2019

matticbot commented Sep 6, 2019

mdawaffe left a comment

Choose a reason for hiding this comment

mdawaffe Sep 6, 2019

Choose a reason for hiding this comment

jeherve Sep 9, 2019

Choose a reason for hiding this comment

kraftbj Sep 19, 2019

Choose a reason for hiding this comment

jeherve Sep 20, 2019

Choose a reason for hiding this comment

matticbot commented Sep 9, 2019

matticbot commented Sep 9, 2019

matticbot commented Sep 9, 2019

matticbot commented Sep 9, 2019

kraftbj left a comment

Choose a reason for hiding this comment

jeherve commented Sep 24, 2019

jeherve commented Sep 6, 2019 •

edited

Loading

jetpackbot commented Sep 6, 2019 •

edited

Loading