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

Validation problem on Gallery related to link settings #1659

Closed
pinarol opened this issue Dec 6, 2019 · 8 comments
Closed

Validation problem on Gallery related to link settings #1659

pinarol opened this issue Dec 6, 2019 · 8 comments
Assignees

Comments

@pinarol
Copy link
Contributor

pinarol commented Dec 6, 2019

Describe the bug

This is not not reproducible on atomic sites, but only other paid plan sites. This looks similar to the problem we currently have on Video block where Jetpack changes the output html in a way that not valid on Core. So maybe it won't have a solution on our side but definitely needs investigation.

To Reproduce

  • Change link settings on web(should be a premium site)
  • Open post on mobile
  • It'll show invalid block placeholder on mobile

Expected behavior
No validation errors.

Screenshots

IMG_3075

Smartphone (please complete the following information):

  • Device: iPhone XS
  • OS: iOS
  • Version 13
@pinarol pinarol changed the title Validation problem on Gallery related to links Validation problem on Gallery related to link settings Dec 6, 2019
@mkevins
Copy link
Contributor

mkevins commented Dec 9, 2019

I was able to reproduce this using a premium site.

Steps I took:

  • On web, in a new post, I created 3 galleries, each with the same single image
  • I changed the "Link To" setting on each gallery to "None", "Media File", and "Attachment Page", respectively
  • I saved the post
  • I copied the html source of the post (from the "Code Editor")
  • I opened the post on mobile (with metro running, I got a red screen for validation failure)
  • I copied the html source on mobile

The result after dismissing the validation red screen is that the gallery 1 and 3 are displayed, while gallery 2 ("Link To" = "Media File") shows "Problem displaying block".The html is different for all three galleries. I after adjusting for whitespace within the html, the differences seem to be the omission of the attribute data-full-url in the content fetched by the mobile app:

diff --git a/valid-gallery-html2 b/invalid-gallery-html2
index e40bad6..a996b78 100644
--- a/valid-gallery-html2
+++ b/invalid-gallery-html2
@@ -3,7 +3,7 @@
 <ul class="blocks-gallery-grid">
 <li class="blocks-gallery-item">
 <figure>
-<img src="https://mattspremiumtestsitetravel.files.wordpress.com/2019/12/mountaindawn.jpg?w=750" alt="" data-id="13" data-full-url="https://mattspremiumtestsitetravel.files.wordpress.com/2019/12/mountaindawn.jpg" class="wp-image-13"/>
+<img src="https://mattspremiumtestsitetravel.files.wordpress.com/2019/12/mountaindawn.jpg?w=750" alt="" data-id="13" class="wp-image-13"/>
 </figure>
 </li>
 </ul>
@@ -16,7 +16,7 @@
 <li class="blocks-gallery-item">
 <figure>
 <a href="https://mattspremiumtestsitetravel.files.wordpress.com/2019/12/mountaindawn.jpg">
-<img src="https://mattspremiumtestsitetravel.files.wordpress.com/2019/12/mountaindawn.jpg?w=750" alt="" data-id="13" data-full-url="https://mattspremiumtestsitetravel.files.wordpress.com/2019/12/mountaindawn.jpg" class="wp-image-13"/>
+<img src="https://mattspremiumtestsitetravel.files.wordpress.com/2019/12/mountaindawn.jpg?w=750" alt="" data-id="13" class="wp-image-13" />
 </a>
 </figure>
 </li>
@@ -29,7 +29,7 @@
 <ul class="blocks-gallery-grid">
 <li class="blocks-gallery-item">
 <figure>
-<img src="https://mattspremiumtestsitetravel.files.wordpress.com/2019/12/mountaindawn.jpg?w=750" alt="" data-id="13" data-full-url="https://mattspremiumtestsitetravel.files.wordpress.com/2019/12/mountaindawn.jpg" class="wp-image-13"/>
+<img src="https://mattspremiumtestsitetravel.files.wordpress.com/2019/12/mountaindawn.jpg?w=750" alt="" data-id="13" class="wp-image-13"/>
 </figure>
 </li>
 </ul>

@mkevins
Copy link
Contributor

mkevins commented Dec 9, 2019

Investigating this further, I have reproduced this on a free site as well:

HTML diff
diff --git a/free-site-valid-html b/free-site-invalid-html
index 554c655..47b73dc 100644
--- a/free-site-valid-html
+++ b/free-site-invalid-html
@@ -3,7 +3,7 @@
 <ul class="blocks-gallery-grid">
 <li class="blocks-gallery-item">
 <figure>
-<img src="https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg?w=683" alt="" data-id="243" data-full-url="https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg" class="wp-image-243"/>
+<img src="https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg?w=683" alt="" data-id="243" class="wp-image-243"/>
 </figure>
 </li>
 </ul>
@@ -16,7 +16,7 @@
 <li class="blocks-gallery-item">
 <figure>
 <a href="https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg">
-<img src="https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg?w=683" alt="" data-id="243" data-full-url="https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg" class="wp-image-243"/>
+<img src="https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg?w=683" alt="" data-id="243" class="wp-image-243" />
 </a>
 </figure>
 </li>
@@ -29,7 +29,7 @@
 <ul class="blocks-gallery-grid">
 <li class="blocks-gallery-item">
 <figure>
-<img src="https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg?w=683" alt="" data-id="243" data-full-url="https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg" class="wp-image-243"/>
+<img src="https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg?w=683" alt="" data-id="243" class="wp-image-243"/>
 </figure>
 </li>
 </ul>

However, in the case of "Link To" == "None", or "Attachment Page", the block is recovered, and displayed normally. Only the "Media File" gallery block fails to display. One notable difference in the html that renders for the gallery block with "Link To" == "Media File" is that the images are wrapped in an <a> element. The href attribute of this element seems to be what is failing validation.

I was also able to reproduce the validation failure via the web block editor with the following steps:

  • Create a new post on a wpcom site (free, personal, or premium)
  • Add a gallery block with one image
  • Change "Link To" setting to "Media File"
  • Save post and leave (back to the post list)
  • Open the devtools console and re-open the post

Result:

image

Block validation: Expected attribute `href` of value `https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg?w=683`, saw `https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg`.
index.js?m=157487816…45e1b07eaec152198:2 Block validation: Block validation failed for `core/gallery` (
{name: "core/gallery", icon: {…}, attributes: {…}, keywords: Array(2), save: ƒ, …}
).

Content generated by `save` function:

<figure class="wp-block-gallery columns-1 is-cropped"><ul class="blocks-gallery-grid"><li class="blocks-gallery-item"><figure><a href="https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg?w=683"><img src="https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg?w=683" alt="" data-id="243" class="wp-image-243"/></a></figure></li></ul></figure>

Content retrieved from post body:

<figure class="wp-block-gallery columns-1 is-cropped"><ul class="blocks-gallery-grid"><li class="blocks-gallery-item"><figure><a href="https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg"><img src="https://onetwoonetwothisisjustatesthome.files.wordpress.com/2019/12/pexels-photo-1260968.jpeg?w=683" alt="" data-id="243" class="wp-image-243" /></a></figure></li></ul></figure>

Appending ?w=683 to the href attribute and re-saving the post seemed to pass validation after re-opening the post.

@pinarol
Copy link
Contributor Author

pinarol commented Dec 9, 2019

hey @jorgefilipecosta 👋 is this something you heard of? Gallery block becomes invalid if link is set to Media File and post is closed/reopened (happens on .com).

@pinarol pinarol mentioned this issue Dec 9, 2019
1 task
@mkevins
Copy link
Contributor

mkevins commented Dec 10, 2019

the differences seem to be the omission of the attribute data-full-url in the content fetched by the mobile app

I have confirmed that this is also true when saving and re-opening a post on web for the .com sites. The omission of data-full-url attribute itself does not seem to present a validation problem directly, but I believe it's absence from the stored html content leads to image.fullUrl being undefined, and the anchor's href falling back to image.url (which has the extra ?w=xxx parameter): https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/gallery/save.js#L22

@jorgefilipecosta
Copy link

Hi @pinarol, @mkevins, I had never heard of this issue before. I tried to replicate the issue on a normal WordPress test instance with Gutenberg master using a less privileged author user and I was not able to replicate the issue. Maybe it is something related to how WordPress.com works?

@kwight
Copy link

kwight commented Dec 11, 2019

WordPress.com has more strict security requirements around the use of data attributes – some background can be found in D23463-code.

@mkevins
Copy link
Contributor

mkevins commented Dec 12, 2019

Thanks Kirk for the info. I believe this is indeed the root of the issue we're encountering.

@jorgefilipecosta, I had some success with the following (somewhat crude) workaround:

diff --git a/packages/block-library/src/gallery/save.js b/packages/block-library/src/gallery/save.js
index 9981c0285..0bb9eeda8 100644
--- a/packages/block-library/src/gallery/save.js
+++ b/packages/block-library/src/gallery/save.js
@@ -8,6 +8,10 @@ import { RichText } from '@wordpress/block-editor';
  */
 import { defaultColumnsNumber } from './shared';
 
+// workaround to get fullUrl from url when image.fullURL is not defined
+// (strips `?w=size` from url string)
+const stripUrlParameters = url => url.split( '?' )[ 0 ];
+
 export default function save( { attributes } ) {
 	const { images, columns = defaultColumnsNumber( attributes ), imageCrop, caption, linkTo } = attributes;
 
@@ -19,7 +23,7 @@ export default function save( { attributes } ) {
 
 					switch ( linkTo ) {
 						case 'media':
-							href = image.fullUrl || image.url;
+							href = image.fullUrl || stripUrlParameters( image.url );
 							break;
 						case 'attachment':
 							href = image.link;

Which assumes that we can safely fallback to using a parameterless url as the full url when linkTo is set to the 'media' option and image.fullUrl is falsey. In all the sites I've tested on, this seems ok: all the sites I've tested either have image.fullUrl defined (and properly parsed from the attributes) and / or have full image urls with no parameters (e.g. https://example.com/YYYY/MM/FILE_NAME-width.jpg, etc), or use the ?w=width parameter, which can be truncated to obtain the full url of the image. However, I'm not certain if there are cases where this could be problematic. Wdyt?

@pinarol
Copy link
Contributor Author

pinarol commented Dec 13, 2019

This is being addressed on .com(D36576-code) so currently no need to make workarounds on Core side as they might introduce other issues.

@pinarol pinarol closed this as completed Dec 13, 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
Development

No branches or pull requests

4 participants