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

Images in Jetpack Slideshow don't respect width setting. #35822

Closed
iamtakashi opened this issue Aug 27, 2019 · 20 comments
Closed

Images in Jetpack Slideshow don't respect width setting. #35822

iamtakashi opened this issue Aug 27, 2019 · 20 comments
Assignees
Labels
[Feature] Post/Page Editor The editor for editing posts and pages. [Type] Bug

Comments

@iamtakashi
Copy link
Contributor

On a self-hosted site, the images in Jetpack Slideshow change its size depending on the width setting (e.g. normal, wide, and full) and that's expected.

localhost_8888_wordpress__p499

However, on WP.com, the size of the image stays the same regardless of the size of the setting. And this results undesirable looking.

gutenbergtestdata wordpress com_2019_06_27_jetpack-slideshow_-1

@kwight kwight added [Type] Bug [Feature] Post/Page Editor The editor for editing posts and pages. labels Aug 28, 2019
@mmtr
Copy link
Member

mmtr commented Aug 28, 2019

For some reason, the Slideshow is also not calculating correctly the images sizes when the browser devtools are opened. This is happening in both WP.com and self-hosted sites.

@mmtr
Copy link
Member

mmtr commented Aug 28, 2019

Seems that the block applies a height that cannot exceeds the 80% of the window height (#).

That explains why I get a different size when the browser devtools are opened, since the window height is lower.

Likely the iframe used in WP.com is making the block to get a wrong window height.

@mmtr
Copy link
Member

mmtr commented Aug 28, 2019

Likely the iframe used in WP.com is making the block to get a wrong window height.

Actually, this is not the cause, since the block applies the right height in the editor. The issue is on the published view. Images are not resized in the frontend when posts are published in WP.com

@mmtr
Copy link
Member

mmtr commented Aug 28, 2019

Ok, so apparently WP.com is applying a more restrictive max-width in the sizes attribute of the image:

Screen Shot 2019-08-28 at 14 59 22 copy

I believe this is performed by the add_srcset_and_sizes function in the wpcom-media.php file. Not sure if this has been caused by the changes done in r195806-wpcom, but it's maybe related to the same issue noted by @Copons in #35578 (comment)

@kwight
Copy link
Contributor

kwight commented Aug 28, 2019

Hm. By the time execution gets to add_srcset_and_sizes, it's already working with an $image argument with a w=640 param; and the $content passed to make_content_images_responsive has image tags with src attributes with 640 width params (but those are not in the raw content in the post, nor in the db post content). So it's getting filtered somewhere between the db post content and make_content_images_responsive.

@kwight
Copy link
Contributor

kwight commented Aug 28, 2019

The 640 value is coming from Jetpack::get_content_width called in the image_add_wh filter, which successfully gets $GLOBALS['content_width'].

@kwight
Copy link
Contributor

kwight commented Aug 28, 2019

Hm, I'm not sure of the best way forward for this. The editor is "fine" because somewhere it's avoiding the image_add_wh filter and loading max-size images. Commenting out the filter gives us the same on the front-end, but removing that filter on the front-end would not be responsible, to say the least.

@iamtakashi
Copy link
Contributor Author

I'm sure this has been considered, but can the value of max-width for the sizes attribute be the full size of the image like it's on self-hosted site?

Limiting the image size to the content width made sense in the old days when the size of images in a content never is larger than the content, but it doesn't make much sense anymore to me in this Gutenberg era.

@kwight
Copy link
Contributor

kwight commented Aug 29, 2019

Proposed solution in D32186-code.

I'm sure this has been considered, but can the value of max-width for the sizes attribute be the full size of the image like it's on self-hosted site?

@iamtakashi Oh! You know, I never confirmed what happened on self-hosted, I assumed there was proper sizing going on for each of wide and full. I'll check. We can easily do that; I just went safer at 1920px in the diff, to avoid people loading 5MB, 6000px images on mobile 🙃

@kwight
Copy link
Contributor

kwight commented Aug 29, 2019

Indeed, without Jetpack and its "site accelerator" (Photon), self-hosted requests the full size image 😱

I really do think we should cap the width at something sane; allowing full images, regardless of size, just feels irresponsible for WP.com (imagine users uploading 30MB images; they could choke their site by adding those images to slideshows). @iamtakashi what would the smallest width be that you consider workable? cc/ @gwwar for any opinions.

@kwight kwight self-assigned this Aug 29, 2019
@mmtr
Copy link
Member

mmtr commented Aug 29, 2019

Indeed, without Jetpack and its "site accelerator" (Photon), self-hosted requests the full size image

Maybe that's a problem on the slideshow block? Blocks have the ability to use any image size when using them, so perhaps we should ensure the slideshow blocks uses the large images instead of the full ones.

@kwight
Copy link
Contributor

kwight commented Aug 29, 2019

The jetpack/slideshow block only works with the full size:

That's only referring to the editor instance, not the front-end render, correct? (Although, having the editor work with maximum large sounds like a good idea too.)

@mmtr
Copy link
Member

mmtr commented Aug 29, 2019

That's only referring to the editor instance, not the front-end render, correct?

Both. That picked url of the full size is the one used when saving the block: https://github.com/Automattic/jetpack/blob/570e900a71af37803e79e3987bf95568d68dc858/extensions/blocks/slideshow/slideshow.js#L150

@iamtakashi
Copy link
Contributor Author

How big is the large size in WP.com? If that's 1024px like self-hosted, it's probably too small to accommodate various scenarios. It's hard to pick one, but 1920px sounds good to me as a start.

@mmtr
Copy link
Member

mmtr commented Aug 29, 2019

I don't think we want to globally increase the content width limit (or even completely remove it), since that feels irresponsible in WP.com as @kwight noted (i.e. posts created with the classic editor will default to the max size allowed, making users to download more MB than needed).

Instead, we can maybe try a narrower approach:

  • We can assume that images inside a block are using a size explicitly set, being large the default one. In WP.com, the large size is 1024px, but can be modified on a per-site basis on WP Admin > Settings > Media.
  • Disable the content width limit only on images inside a block, because we should respect the size explicitly set by the block. Or simply disable the limit if the post has been created with the block editor.
  • Add an image size selector in the slideshow block that defaults to large but allowing the full option, so we can accommodate the scenarios mentioned by @iamtakashi.

@mmtr
Copy link
Member

mmtr commented Aug 29, 2019

Disable the content width limit only on images inside a block, because we should respect the size explicitly set by the block. Or simply disable the limit if the post has been created with the block editor.

See D32189-code.

@mmtr
Copy link
Member

mmtr commented Aug 29, 2019

Add an image size selector in the slideshow block that defaults to large but allowing the full option

WIP in Automattic/jetpack#13362

@kwight
Copy link
Contributor

kwight commented Aug 29, 2019

Fixed in D32189-code.

@kwight kwight closed this as completed Aug 29, 2019
@iamtakashi
Copy link
Contributor Author

Thank you both for working on this so quickly. I appreciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages. [Type] Bug
Projects
None yet
Development

No branches or pull requests

3 participants