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

Prevent image with caption from overflowing its container in Classic mode #1728

Merged
merged 2 commits into from Dec 12, 2018

Conversation

Projects
None yet
2 participants
@kienstra
Copy link
Collaborator

kienstra commented Dec 12, 2018

Thanks to @lumay for raising this in:
https://wordpress.org/support/topic/amp-img-with-caption-center-problem/

Steps To Reproduce

  1. Select 'Classic' mode
  2. Activate the Classic Editor plugin, assuming you're running WordPress 5.0
  3. Create a post
  4. Add an image with a width of at least 1000px, and select Full Size
  5. Add a caption to the image
  6. Update the post and go to its AMP URL
  7. Expected: the image stays within its container
  8. Actual: it overflows its container:

image-caption-overflows-container

Background

This only looks to exist in the Classic editor, or when using the Classic block, or when adding a [caption] to a Shortcode block.

An image with a caption in the Classic editor creates a figure.wp-caption, which often has an inline width style value, like <figure style="width: 1199px" ...>

This can cause it to overflow the container.

There used to be logic in AMP_Style_Sanitizer::filter_style() that converted 'width' to 'max-width'.

Now that it's removed, this PR applies a simple style rule as a workaround.

This is in a stylesheet because it only applies to Classic mode.

This issue doesn't seem to exist in Paired or Native mode, where themes can add their own max-width styling.

Before

captions-before

After

captions-after

Fix an issue where <figure> can overflow its container
figure.wp-caption often has an inline width style value,
like <figure style="width: 1199px" ...>
This can cause it to overflow the container.
There used to be logic in AMP_Style_Sanitizer::filter_style()
that converted 'width' to 'max-width'.
Now that it's removed,
apply a style rule as a workaround.
This is in a stylesheet because this only applies to Classic mode.
This issue doesn't seem to exist in Paired or Native mode.
@kienstra

This comment has been minimized.

Copy link
Collaborator Author

kienstra commented Dec 12, 2018

Request For Review

Hi @westonruter and @amedina,
Could you please review this?

Instead of adding a style rule, a change like this would be possible here in AMP_Img_Sanitizer:: add_auto_width_to_figure(). But what I came up with was more complex:

/*
 * In Classic mode, prevents an image with a caption from overflowing its container.
 * In v0.7.2, AMP_Style_Sanitizer::filter_style() converted inline styling for width to max-width.
 * For example, it converted <figure style="width: 1000px" ...> to <figure style="max-width: 1000px" ...>.
 * This wouldn't be appropriate in Paired or Native mode, as themes can set this max-width in their stylesheets.
 */
if (
	! current_theme_supports( 'amp' )
	&&
	false === strpos( $class, 'wp-block-image' ) // This fix doesn't apply to the Image block.
	&&
	false !== strpos( $class, 'wp-caption' )
	&&
	$figure->hasAttribute( 'style' )
	&&
	preg_match( '/(^|[;\s])width:/', $figure->getAttribute( 'style' ) )
) {
	$figure->setAttribute( 'style', 'max-width: 100%;' . $figure->getAttribute( 'style' ) );
	return;
}

Thanks!

@kienstra kienstra requested review from westonruter and amedina Dec 12, 2018

@westonruter westonruter added this to the v1.0.1 milestone Dec 12, 2018

@westonruter
Copy link
Member

westonruter left a comment

The changes here make sense to me.

Remove figure element from selector, as it could be a div
The shortcode callback for [caption] can output
either a figure or a div,
depending on whether there is HTML5 support:
https://github.com/WordPress/wordpress-develop/blob/7ec7fadfe751795871caf4ca6ca34e92f1450925/src/wp-includes/media.php#L1626
@kienstra

This comment has been minimized.

Copy link
Collaborator Author

kienstra commented Dec 12, 2018

Thanks to @lumay's feedback, the original commit didn't completely fix this.

b9e0463 is needed to apply to cases where there's no theme support for HTML5.

In that case, the caption shortcode uses a div, not a <figure>.

So this removes figure from the selector.

Before

images-that-have-captions-before

After

images-that-have-captions-after

@westonruter westonruter merged commit 8a882ae into 1.0 Dec 12, 2018

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.