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

Editor: incorrect poster dimensions being returned #12354

Closed
swissspidy opened this issue Sep 21, 2022 · 6 comments · Fixed by #12418
Closed

Editor: incorrect poster dimensions being returned #12354

swissspidy opened this issue Sep 21, 2022 · 6 comments · Fixed by #12418
Assignees
Labels
Group: WordPress Changes related to WordPress or Gutenberg integration PHP Pull requests that update PHP code Type: Bug Something isn't working

Comments

@swissspidy
Copy link
Collaborator

Bug Description

Lucky found this obscure bug where the checklist in the editor shows a warning about incorrect poster dimensions, even though the poster definitely has the right size.

I tracked this down to some unexpected behavior in WordPress that boils down to honoring $content_width.

This only happens in the admin and not when accessing the REST API in an other way.

Here's how it works:

  1. In the editor we preload REST API responses, such as the one for the current story (which is handled by Stories_Controller)
  2. \Google\Web_Stories\REST_API\Stories_Controller::get_story_poster() calls wp_get_attachment_image_src()
  3. wp_get_attachment_image_src() in turn calls image_downsize
  4. image_downsize in turn calls image_constrain_size_for_editor
  5. image_constrain_size_for_editor then reduces the width and height because we're in the admin (is_admin() is true because of the REST API preloading)

To work around this, I think we have to call image_get_intermediate_size() to get the actual size ourselves, and if that returns false (because the size does not exist), get the original size from wp_get_attachment_metadata().

But there might be other options.

Notes:

  • This same issue theoretically exists in \Google\Web_Stories\Model\Story::load_from_post()
  • We could just use the Story model in Stories_Controller, so we can delete the redundant get_story_poster method there

Aside:

I find it unexpected that wp_get_attachment_image_src() honors $content_width like this. Maybe worth opening a Trac ticket?

Expected Behaviour

There should be no incorrect warning.

Steps to Reproduce

  1. Activate Twenty Twenty theme
  2. Create a new story and add a few pages
  3. Add a poster image by uploading a new image that needs to be cropped & crop it
  4. Save the story
  5. Reload the page
  6. Notice the checklist warning about incorrect poster dimensions

Screenshots

Screenshot 2022-09-21 at 17 41 01

Additional Context

  • Plugin Version:
  • WordPress Version:
  • Operating System:
  • Browser:
@swissspidy swissspidy added Type: Bug Something isn't working PHP Pull requests that update PHP code Group: WordPress Changes related to WordPress or Gutenberg integration Pod: WP labels Sep 21, 2022
@spacedmonkey
Copy link
Contributor

We have a filter in image_constrain_size_for_editor called editor_max_image_size. I wonder if this can be used the expected results.

Maybe worth opening a Trac ticket?

I think it is a good call.

@swissspidy
Copy link
Collaborator Author

Hmm I guess we could do something like this with that filter:

add_filter( 'editor_max_image_size', $constrain_dimensions );
$poster_src = wp_get_attachment_image_src( $thumbnail_id, Image_Sizes::POSTER_PORTRAIT_IMAGE_SIZE );
remove_filter( 'editor_max_image_size', $constrain_dimensions );

That seems to work

@spacedmonkey
Copy link
Contributor

I am trying to think of a solution that would work with BC in mind. I was thinking about adding a filter before the preload and remove after preload is finished. Which might fix other issues in the future.

@swissspidy
Copy link
Collaborator Author

So basically doing that at a higher level (before the preload) vs. within the controller?

@spacedmonkey
Copy link
Contributor

So basically doing that at a higher level (before the preload) vs. within the controller?

Yeah, I would have done it in edit-story.php.

@swissspidy
Copy link
Collaborator Author

I suppose that could work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Group: WordPress Changes related to WordPress or Gutenberg integration PHP Pull requests that update PHP code Type: Bug Something isn't working
Projects
None yet
2 participants