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

Address issue where <ul> is converted to an <amp-carousel> #1529

Merged
merged 4 commits into from Oct 24, 2018

Conversation

@kienstra
Copy link
Contributor

@kienstra kienstra commented Oct 24, 2018

Thanks to @christianc1 for the detailed report of this in #1528.

Steps To Reproduce

  1. In AMP Settings > Template Mode, select 'Classic'
  2. Create a post using Gutenberg
  3. Add a Custom HTML block with:
<ul><li><a href="#">click me</a></li></ul>
  1. Save and go to the AMP URL for the post
  2. Expected: this markup shouldn't be changed on the AMP URL
  3. Actual: this is wrapped in an <amp-carousel>, though it's not a gallery block:

classic-mode-carousel

Background

AMP_Gallery_Block_Sanitizer iterates through the <ul> tags that have child nodes.

As @christianc1 mentioned, if this is in Classic mode, the sanitizer will have the argument of array( 'carousel_required' => true ).

This causes $is_amp_carousel to be true, even for something like <ul><li></li></ul>.

Approach

Assuming that the AMP_Gallery_Block_Sanitizer is only intended for the Gallery block, restricting it to that block would probably make sense.

It looks like the Gallery block should always have the class of wp-block-gallery. Its stylesheet only uses that class, and the image selector begins with ul.wp-block-gallery.

So this restricts the Gallery block sanitizer to only ul.wp-block-gallery, and it simply exits if the class isn't present:

ul-click-me

Closes #1528.

For example:
<ul><li><a href="#">click me</a></li></ul>
...is converted to an <amp-carousel>
So check that the class of wp-block-gallery is present
before the conversion.
@kienstra kienstra changed the title Address issue where <ul> with an <a> is converted to an <amp-carousel> Address issue where <ul> is converted to an <amp-carousel> Oct 24, 2018
@kienstra
Copy link
Contributor Author

@kienstra kienstra commented Oct 24, 2018

Pending Unit Test Passing

If it's alright, I'll look at the failed unit test tomorrow. It passes locally.

@westonruter
Copy link
Member

@westonruter westonruter commented Oct 24, 2018

It looks like the failed test may be due to something new introduced in WordPress 5.0.

How does this PR relate to #1527?

@kienstra
Copy link
Contributor Author

@kienstra kienstra commented Oct 24, 2018

Hi @westonruter,
This PR takes a different approach from #1527, though we might still consider #1527.

I think there's something needed to ensure the AMP_Gallery_Block_Sanitizer only operates on Gallery blocks. Like how this PR checks that the <ul> has the class of wp-block-gallery.

@westonruter westonruter added this to the v1.0 milestone Oct 24, 2018
@westonruter westonruter merged commit b5dfe3d into 1.0 Oct 24, 2018
2 checks passed
@swissspidy swissspidy deleted the fix/1528-gallery branch Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants