Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AMP Gallery Block Sanitizer transforming all <ul>'s to carousels. #1528

Closed
christianc1 opened this issue Oct 23, 2018 · 2 comments

Comments

Projects
3 participants
@christianc1
Copy link
Contributor

commented Oct 23, 2018

In cc53173, logic was added (from the commit message for back-compat), setting the AMP_Gallery_Block_Sanitizer::$args['carousel_required'] to true. This affects all installations that have not explicitly declared some sort of add_theme_support for amp.

An issue arises as the logic in AMP_Gallery_Block_Sanitizer::sanitize allows through all <ul> nodes. Further in the method, there is a check to see if there are child <a> nodes and if there are, pushes them onto an array of images which are then appended to the carousel.

So there are two problems.

  1. As far as I can tell, all <ul>'s will be will be transformed to galleries if one of their items contain a link, unless add_theme_support is declared.
  2. Given the above, any content that is not an image or a link or a descendent of one, is stripped and not rendered to the amp template.

Admittedly I'm a bit fuzzy as to the full context here, but I opened #1527. If someone can fill in some blanks, I can help contribute a better fix if this PR is missing something.

@christianc1

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2018

We were able to hotfix this issue with a version of the following:

        /**
	 * Fixes an issue where AMP transforms unordered lists to amp-carousels.
	 *
	 * @link https://github.com/Automattic/amp-wp/issues/1528
	 * @todo This can be removed when the above issue is resolved.
	 *
	 * @param  array $sanitizers An array of content sanitizers.
	 * @return array             An array of content sanitizers.
	 */
	public static function hotfix_amp_content_sanitizers( $sanitizers ) {
		$sanitizers['AMP_Gallery_Block_Sanitizer'] = array();

		return $sanitizers;
	}
        add_filter( 'amp_content_sanitizers', 'hotfix_amp_content_sanitizers' );

@kienstra kienstra self-assigned this Oct 24, 2018

@kienstra kienstra added this to In progress in v1.0 Oct 24, 2018

@westonruter westonruter added this to the v1.0 milestone Oct 24, 2018

@westonruter

This comment has been minimized.

Copy link
Member

commented Oct 24, 2018

Fixed in #1529.

@kienstra kienstra moved this from In progress to Ready for Merging in v1.0 Oct 30, 2018

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.