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

Tiled Gallery: Fix block validation errors caused by inconsistent flex-basis style #18971

Merged
merged 5 commits into from
Mar 12, 2021

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Mar 2, 2021

Fixes #16514
Closes #16543

A change made to the Tiled Gallery last year to fix mosaic and column layouts on AMP pages (#15770) introduced a block validation issue. This was a result of applying a decimal percentage width to columns via an inline flex-basis style. In the actual block content those decimal values were rounded to 12 places whereas the save function allowed 15. This inconsistency meant the two didn't match and caused the error.

This PR is essentially a clone of #16543 with fixes applied so the v3 deprecation runs correctly. I ran into issues rebasing the original PR after the recent Jetpack restructure hence this fresh PR.

Changes proposed in this Pull Request:

  • Restrict the percentage columns widths applied as flex-basis inline style to 5 decimal places
  • Add deprecation to migrate blocks with a flex-basis style already applied

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

No change.

Testing instructions:

  • On a wpcom site, create a new post, insert a layout grid and tiled galleries within its columns
  • Assign one gallery the mosaic block style and another the columns style
  • Save the post and reload the editor, you should receive block validation errors
  • Sync this PR or apply its diff to your sandbox and sandbox your test site
  • Reload the editor again and you should not encounter the block validation error
  • In dev tools check the console logs to ensure the deprecation ran and the block successfully migrated

Screenshot

Screen Shot 2021-03-02 at 11 49 52 pm

Proposed changelog entry for your changes:

  • Bug Fix: Prevent block validation errors for tiled galleries with mosaic or column layouts.

@aaronrobertshaw aaronrobertshaw added [Type] Bug When a feature is broken and / or not performing as intended [Block] Tiled Gallery labels Mar 2, 2021
@aaronrobertshaw aaronrobertshaw requested review from carlosenamdev and a team March 2, 2021 14:27
@aaronrobertshaw aaronrobertshaw self-assigned this Mar 2, 2021
@github-actions github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Mar 2, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2021

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.

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 🤖


If you are an automattician, once your PR is ready for review 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.undefined


jetpack plugin:

  • Next scheduled release: April 6, 2021.
  • Scheduled code freeze: March 29, 2021

@github-actions github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Mar 2, 2021
@aaronrobertshaw aaronrobertshaw added [Status] Needs Team Review and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Mar 2, 2021
@glendaviesnz
Copy link
Contributor

This worked for new blocks added after the patch applied, but the block added prior to applying the patch still throws a validation failure after patch applied:

Block validation: Expected attribute 'style' of value 'flex-basis:60.2428555574296%', saw 'flex-basis:60.24285555743%;'

full block content:

`<!-- wp:jetpack/layout-grid {"column1DesktopSpan":6,"column1TabletSpan":4,"column1MobileSpan":4,"column2DesktopSpan":6,"column2TabletSpan":4,"column2MobileSpan":4,"className":"column1-desktop-grid__span-6 column1-desktop-grid__row-1 column2-desktop-grid__span-6 column2-desktop-grid__start-7 column2-desktop-grid__row-1 column1-tablet-grid__span-4 column1-tablet-grid__row-1 column2-tablet-grid__span-4 column2-tablet-grid__start-5 column2-tablet-grid__row-1 column1-mobile-grid__span-4 column1-mobile-grid__row-1 column2-mobile-grid__span-4 column2-mobile-grid__row-2"} -->
<div class="wp-block-jetpack-layout-grid alignfull column1-desktop-grid__span-6 column1-desktop-grid__row-1 column2-desktop-grid__span-6 column2-desktop-grid__start-7 column2-desktop-grid__row-1 column1-tablet-grid__span-4 column1-tablet-grid__row-1 column2-tablet-grid__span-4 column2-tablet-grid__start-5 column2-tablet-grid__row-1 column1-mobile-grid__span-4 column1-mobile-grid__row-1 column2-mobile-grid__span-4 column2-mobile-grid__row-2"><!-- wp:jetpack/layout-grid-column -->
<div class="wp-block-jetpack-layout-grid-column wp-block-jetpack-layout-grid__padding-none"><!-- wp:jetpack/tiled-gallery {"columnWidths":[[60.2428555574296,39.7571444425704]],"ids":[424,422,420]} -->
<div class="wp-block-jetpack-tiled-gallery aligncenter is-style-rectangular"><div class="tiled-gallery__gallery"><div class="tiled-gallery__row"><div class="tiled-gallery__col" style="flex-basis:60.24285555743%;"><figure class="tiled-gallery__item"><img alt="" data-height="194" data-id="424" data-link="https://mysite.com/tree5-1/" data-url="https://mysite.com/2021/01/tree5-1.jpeg" data-width="260" src="https://mysite.com/2021/01/tree5-1.jpeg" /></figure></div><div class="tiled-gallery__col" style="flex-basis:39.75714444257%;"><figure class="tiled-gallery__item"><img alt="" data-height="168" data-id="422" data-link="https://mysite.com/tree1-1/" data-url="https://mysite.com/2021/01/tree1-1.jpeg" data-width="299" src="https://mysite.com/2021/01/tree1-1.jpeg" /></figure><figure class="tiled-gallery__item"><img alt="" data-height="168" data-id="420" data-link="https://mysite.com/tree6-1/" data-url="https://mysite.com/2021/01/tree6-1.jpeg" data-width="299" src="https://mysite.com/2021/01/tree6-1.jpeg" /></figure></div></div></div></div>
<!-- /wp:jetpack/tiled-gallery --></div>
<!-- /wp:jetpack/layout-grid-column -->

<!-- wp:jetpack/layout-grid-column -->
<div class="wp-block-jetpack-layout-grid-column wp-block-jetpack-layout-grid__padding-none"></div>
<!-- /wp:jetpack/layout-grid-column --></div>
<!-- /wp:jetpack/layout-grid -->

@aaronrobertshaw
Copy link
Contributor Author

This worked for new blocks added after the patch applied, but the block added prior to applying the patch still throws a validation failure after patch applied:

Block validation: Expected attribute 'style' of value 'flex-basis:60.2428555574296%', saw 'flex-basis:60.24285555743%;'

I think that particular message is a bit of a read herring. Looking at the other validation errors they show the new deprecation rounding the flex basis value so the style matches. It looks like I had a data attribute in the deprecation's gallery image save function that shouldn't have been there. Now I've removed that the block's migrate successfully again for me.

Could have sworn I needed that attribute there in previous tests. Would definitely appreciate it if you could give this one another test. Thanks! 🙂

glendaviesnz
glendaviesnz previously approved these changes Mar 4, 2021
Copy link
Contributor

@glendaviesnz glendaviesnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tested well for me. Added a tiled added a mosaic and columns tiled gallery in a layout grid and both showed invalid when reloaded. Applied patch and deprecations were applied and resaving and reloading, or adding new tiled galleries didn't result in any more validation errors.

@glendaviesnz glendaviesnz added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Team Review labels Mar 4, 2021
carlosenamdev
carlosenamdev previously approved these changes Mar 5, 2021
Copy link
Contributor

@carlosenamdev carlosenamdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Aaron for fixing this, I Approve

@anomiex
Copy link
Contributor

anomiex commented Mar 10, 2021

Change itself looks ok. But it needs a master merge for the "Changelogger use" and "Changelogger validity" checks, and a changelogger change file so the former will pass.

@anomiex anomiex added [Status] Ready to Merge Go ahead, you can push that green button! [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 10, 2021
…sis style

Inconsistent flex-basis style values are causing block validation errors in the tiled gallery. This fixes that issue and adds a deprecation to correct any blocks created between the problem commit was merged and this one.
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello aaronrobertshaw! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D58515-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@aaronrobertshaw
Copy link
Contributor Author

Thanks @anomiex, I've rebased the PR and added the changelog.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests well for me. Nice work!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Mar 11, 2021
@jeherve jeherve added this to the 9.6 milestone Mar 11, 2021
@aaronrobertshaw aaronrobertshaw merged commit 488952f into master Mar 12, 2021
@aaronrobertshaw aaronrobertshaw deleted the fix/tiled-gallery-validation-errors branch March 12, 2021 00:32
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 12, 2021
@github-actions
Copy link
Contributor

Great news! One last step: head over to your WordPress.com diff, D58515-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

@aaronrobertshaw
Copy link
Contributor Author

Needed to rebase diff. Build is failing at present. Will revisit committing the diff Monday.

@aaronrobertshaw
Copy link
Contributor Author

Deployed: rWP222709

jeherve added a commit that referenced this pull request Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Tiled Gallery [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gutenberg: Tiled Gallery on Layout Grid invalid content
6 participants