Tiled Gallery: Fix block validation issues caused by attribute removals#43345
Conversation
coder-karen
added
Bug
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
github-actions
bot
added
the
[Plugin] Jetpack
Code Coverage SummaryCoverage changed in 3 files.
12 files are newly checked for coverage. Only the first 5 are listed here.
Full summary · PHP report · JS report Coverage check overridden by
I don't care about code coverage for this PR
|
coder-karen
force-pushed
the
fix/tiled-gallery-accessibility-related-validation-issues
branch
2 times, most recently
from
be38926 to
f53ea90
Compare
coder-karen
force-pushed
the
fix/tiled-gallery-accessibility-related-validation-issues
branch
from
1ae79c1 to
f4bc02f
Compare
coder-karen
changed the title
…n appropriate - conditionally
… and previous changes
…ple sites, so that images continue to display on AMP displayed pages when AMP is active
… things work with data attributes
coder-karen
force-pushed
the
fix/tiled-gallery-accessibility-related-validation-issues
branch
from
be5e8ce to
654a3f1
Compare
coder-karen
added
[Status] Needs Review
fgiannar
left a comment
fgiannar
left a comment
There was a problem hiding this comment.
Many thanks for working on this, Karen 👍
I followed the testing intructions for multisites and everything works as described. I only have a question around using custom links in the Tiled Gallery with Carousel enabled:
Looks like we are never redirected to the specified link. We see the image in the Carousel instead. Just wanted to make sure if this is expected behaviour as I thought that by clicking on the image should redirect me to the corresponding custom link.
|
Thanks for testing @fgiannar !
Yes that's expected. Or rather, that's been the default behaviour as long as I'm aware, as it is also the case if you set the link to the attachment page or media file. In that case it will open up the Carousel if Carousel is enabled. That should be improved though - here is a related issue: #42097 |
fgiannar
left a comment
fgiannar
left a comment
There was a problem hiding this comment.
Many thanks for all the additional info, appreciate it!
Tested on a JN multisite and a Simple site and everything works as described 🚢
coder-karen
added
the
I don't care about code coverage for this PR
coder-karen
deleted the
fix/tiled-gallery-accessibility-related-validation-issues
branch
github-actions
bot
removed
the
[Status] Needs Review
While looking at adding some more eslint rules, I found lints in these files that should have broken the build. Turns out these files are completely unused, which is why they didn't do so. Let's delete them. * `tiled-gallery/deprecated/v7` stuff being removed here was added during development of #43345, but all uses were removed before the PR was finished. * `query-account-protection-settings` was added during development of #40925, but then use of it was removed before the PR was finished. * `post-publish-qr-post-panel` stuff is unused since #23390. * `search-dashboard-entry` is unused since #21888. * `section-nav/segmented` was apparently never used, just brought in with a bunch of other stuff way back in #8208.
While looking at adding some more eslint rules, I found lints in these files that should have broken the build. Turns out these files are completely unused, which is why they didn't do so. Let's delete them. * `tiled-gallery/deprecated/v7` stuff being removed here was added during development of #43345, but all uses were removed before the PR was finished. * `query-account-protection-settings` was added during development of #40925, but then use of it was removed before the PR was finished. * `post-publish-qr-post-panel` stuff is unused since #23390. * `search-dashboard-entry` is unused since #21888. * `section-nav/segmented` was apparently never used, just brought in with a bunch of other stuff way back in #8208.
2 participants
Fixes several Tiled Gallery PRs:
#16514
#20756
#42104
#42119
(and others, mentioned in the main Linear issue, to be re-reviewed afterwards).
Related Linear issues: VULCAN-90 (and VULCAN-4, VULCAN-35, VULCAN-27)
Proposed changes:
data-custom-linkattribute which was being removed on Simple sites (same cause as above - see below).data-amp-layoutandtabindexwas being removed from the HTML on Simple sites only (front-end), yet on subsites of multisites, when a Tiled Gallery block was added by a subsite admin, only thetabindexwas being removed. The new code includesdata-amp-layout(needed if using the AMP plugin and viewing the page in AMP transitional mode), but only if the site is not Simple. It also removes thetabindex(and while at it, theroleandaria-label) - only to add them back via PHP on the front-end if Carousel is enabled.data-amp-layout(as well asdata-custom-linkin this case) is due to code that strips outdata-attributes, unless whitelisted. See this Linear link for related links: VULCAN-90tabindexattribute is due to the usage of theunfiltered_htmlcapability on multisites - only superadmins will have that capability. Confirmed in testing by adding the capability back .data-amp-layoutattribute on Simple is that block validation issues in Tiled Galleries added within the Layout block are now not an issue, however layout issues then became visible. These have been fixed with changes in theresize.jsfile and in the editor scss file (which then required some edits to meet currentsass-embeddedrequirements).tabindexfrom thegallery-image/save.jsin the v7 deprecation version, if the Tiled Gallery is being added by a non super-admin on a multisite network (or more realistically, anyone who doesn't haveunfiltered_htmlcapability on a multisite network - or any user role ifDISALLOW_UNFILTERED_HTMLis set to true inwp-config.php).save.jschanges that ultimately had been problematic.Other information:
Jetpack product discussion
Relevant Slack conversation - p1747151379475959-slack-C05PV073SG3
p1749539621385629-slack-C02A41GCS
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
On a self-hosted test site, with this PR applied (locally), or on a WoA or Jurassic Ninja test site with the Jetpack Beta tester plugin, and this PR applied. As a note, the reported issues predominantly affect Simple sites and multisites, so prioritize testing those (see further down):
/wp-admin/admin.php?page=jetpack#writing).tabindexof0,roleofbutton, and anaria-label. In the editor the image markup should not include anaria-label,roleortabindex.tabindex,aria-labelorrole. If link is set to none, you won't be able to click on the images (it won't do anything). For any of the other link options, the link should work. There should still be noaria-label,tabindexorrole- as the image is no longer the clickable element but instead is wrapped in an anchor tag./wp-admin/admin.php?page=jetpack#writing- this should be enabled by default on Simple). On saving, there should be no block validation errors..aria-label,tabindexorroleapplied to images if Carousel is disabled - you may need make a minor change and re-save the post for this to apply though.Multisites:
?specialops=true#), create a multisite, and within that multisite network create a subsite.Simple:
/wp-admin/options-media.php)