Make WordPress Core

Opened 5 years ago

Last modified 2 years ago

#49601 reopened defect (bug)

layout width bugfix for img_caption_shortcode()

Reported by: joelhardi's profile joelhardi Owned by:
Milestone: Priority: normal
Severity: normal Version: 5.4
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 5 years ago.
bad.png (136.5 KB) - added by joelhardi 5 years ago.
screenshot showing what the layout bug looks like
good.png (128.8 KB) - added by joelhardi 5 years ago.
screenshot showing what the fix looks like
49601-2.diff (1.2 KB) - added by joelhardi 5 years 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 (9)

@joelhardi
5 years ago

@joelhardi
5 years ago

screenshot showing what the layout bug looks like

@joelhardi
5 years ago

screenshot showing what the fix looks like

#1 @ocean90
5 years 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
5 years 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
5 years 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
5 years 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
5 years 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.

#5 @andrewklimek
2 years ago

Came here to suggest the same fix and found this old ticket. Too bad it's been ignored.

Note: See TracTickets for help on using tickets.