WordPress.org

Make WordPress Core

Opened 3 weeks ago

Last modified 3 weeks ago

#49601 reopened defect (bug)

layout width bugfix for img_caption_shortcode()

Reported by: joelhardi Owned by:
Milestone: Priority: normal
Severity: normal Version: trunk
Component: Media Keywords: has-patch dev-feedback
Focuses: Cc:

Description

img_caption_shortcode() in wp-includes/media.php is hardcoding an inline style="width:" attribute on the outer <figure> or <div> element that contains the image and image caption, which it sets to the pixel width of the image. This is so the image caption is the same width as the image.

The problem is that on mobile (narrow width) layouts this hardcoded figure/div element width will cause the entire container element to expand to this width, which makes the entire main content column overflow the window/viewport, so the page content overflows the screen and can't be read. Google search console also flags this issue. Here's what it looks like:

Normally in CSS, themes have something simple like img { max-width: 100%; } to prevent images from overflowing the container element. But since WordPress is hardcoding the element width with an inline style, this takes precedence and clobbers whatever in in CSS. So it is not possible for a theme to fix this issue.

The fix is simple, just use max-width instead of width. The caption text still stretches to whatever the width of the image is, but the element no longer overflows the page. Patch is attached but all it is doing is using max-width instead of width`.

Attachments (4)

49601.diff (410 bytes) - added by joelhardi 3 weeks ago.
bad.png (136.5 KB) - added by joelhardi 3 weeks ago.
screenshot showing what the layout bug looks like
good.png (128.8 KB) - added by joelhardi 3 weeks ago.
screenshot showing what the fix looks like
49601-2.diff (1.2 KB) - added by joelhardi 3 weeks ago.
new patch that removes hardcoded width on container <figure> and puts max-width on caption element, see my comment

Download all attachments as: .zip

Change History (8)

@joelhardi
3 weeks ago

@joelhardi
3 weeks ago

screenshot showing what the layout bug looks like

@joelhardi
3 weeks ago

screenshot showing what the fix looks like

#1 @ocean90
3 weeks ago

  • Keywords dev-feedback removed
  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed
  • Version trunk deleted

Hello @joelhardi, thanks for the ticket.

In 4.9 the inline style was changed to use max-width via [41724] for #33981. But it had to be reverted in [42837] due to incompatibility with existing themes, see #43123.

You can find a possible workaround in this comment.

#2 follow-up: @joelhardi
3 weeks ago

  • Keywords dev-feedback added
  • Resolution duplicate deleted
  • Status changed from closed to reopened
  • Version set to trunk

Hi @ocean90 thanks for the feedback. I read those ticket threads, what a painful history for a silly problem!

I understand the issue (WordPress inline style overriding max-width values set in themes) and the reason for the revert. Sure, it's no problem for me to add a filter like that.

BUT ... I think it's a bad behavior for WordPress to break mobile layouts by default. Especially now that Google has gone to mobile-first crawling and this will get flagged as an issue.

So I have another suggestion (new patch), please let me know what you think. It seems a lot better to me and should have no (or at least fewer) impacts on existing themes. It would definitely not create the conflict that was identified in #43123.

What it does is remove the width inline style on the container figure/div element, and instead put a max-width on the figcaption/p element. Which is probably better anyway, since this is the element that should stretch to the same width as the image (not the figure container). So the output HTML looks like this:

<figure class="wp-caption alignnone">     <!-- or alignleft or alignright -->
    <a><img></a>
    <figcaption style="max-width: 500px" class="wp-caption-text">Caption</figcaption>
</figure>

And in the example @smerriman uses in #43123 where he has got .alignleft, .alignright {max-width:50%;} in this theme, that will still work (the outer container <figure> element shrinks to 50% container width). I tested it out.

The only negative theme impact would now be if a theme has got a max-width set on .wp-caption-text. I don't know if the core dev team has any idea about whether or how often that might be happening. But even in that case the theme developer could just update their theme by adding for example .wp-caption { max-width: 50%; } to their theme.

This seems like a much better overall solution.

@joelhardi
3 weeks ago

new patch that removes hardcoded width on container <figure> and puts max-width on caption element, see my comment

#3 in reply to: ↑ 2 @smerriman
3 weeks ago

Replying to joelhardi:

BUT ... I think it's a bad behavior for WordPress to break mobile layouts by default. Especially now that Google has gone to mobile-first crawling and this will get flagged as an issue.

It's the responsibility of the theme to ensure images work on mobile - if your theme didn't have the line:

img {max-width:100%;}

it would also break.

Just use:

img, .wp-caption {max-width:100%;}

and there are no issues. This is what all themes do, so I see no need to change some fundamental logic in core.

#4 @joelhardi
3 weeks ago

I would have suggested removing inline styles completely but I know that would break people's layouts. (It seems like a hack for core to be hardcoding styles inline, it should be left to each theme's CSS to apply styles, and this width inline style seems unnecessary. If the only point was to make the caption the same width as the image, CSS alone can do that without hardcoding any pixel values anywhere.)

So, goal of the patch is just to incrementally improve core even if this technical debt can't be eliminated completely. Using max-width is better than hardcoding width, and moving this to the caption element is better than the container (it addresses your issue in #43123).

This way the core behavior is improved. With the patch it's actually no longer necessary to put .wp-caption {max-width:100%;} into themes as a workaround for the hardcoded width.

Note: See TracTickets for help on using tickets.