Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#43123 closed defect (bug) (fixed)

Default captions should NOT use max-width

Reported by: smerriman's profile smerriman Owned by: joemcgill's profile joemcgill
Milestone: 4.9.5 Priority: normal
Severity: normal Version: 4.9
Component: Media Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

In #33981, the default caption output had the inline style changed from width: xx to max-width: xx, "to improve compatibility with flexible layouts."

I have written a large number of themes that includes this extremely basic style:

.alignleft, .alignright {max-width:50%;}

This ensures floated images, regardless of whether they are with or without a caption, never take up more than 50% of the column width, allowing enough space for text to wrap around them regardless of the screen size.

The change has completely broken this behavior, with the inline style overriding the CSS.

The old output for creating captions has been around for a huge number of years, and never had any issues with "flexible layouts" - just add a max-width:100% in the CSS if you need to.

I would like to request this is changed back to the original behavior.

Attachments (1)

43123.diff (1.7 KB) - added by joemcgill 6 years ago.

Download all attachments as: .zip

Change History (22)

#1 @lrdn
6 years ago

Is there a specific reason you can't use the !important rule on the declaration? I know it is considered bad practice, but so are inline CSS styles which shouldn't be used by the WordPress core anyway.

.alignleft, .alignright {
	max-width: 50% !important;
}

#2 @smerriman
6 years ago

No, that doesn't work at all, since then you lose the original width entirely. A small floated image (less than 50%) with a long caption would now have the caption extending past the edge of the image.

Sure, you can probably work around it by switching things around so the CSS declaration now contains width:50%, so both work in conjunction again; but a) that means now having to treat captioned and non-captioned images differently, b) that still means needing to edit every single theme created in the past.

Captions are such a fundamental part of WordPress, and as I mentioned caused no issues whatever with flexible layouts, that this simply will break too many old themes for no benefit at all.

(If you do some searching you'll find a lot of support requests about other people's themes who have broken too.)

Last edited 6 years ago by smerriman (previous) (diff)

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


6 years ago

#4 @lrdn
6 years ago

Replying to smerriman:

No, that doesn't work at all, since then you lose the original width entirely. A small floated image (less than 50%) with a long caption would now have the caption extending past the edge of the image.

Indeed you're right, I am afraid I didn't test with an image smaller then 50% of the content width. Thinking about it I also don't quite understand the change to using max-width inline instead of width when it is much more useful to be defined in the style sheet of a theme to ensure compatibility with flexible layouts.

Of course, one could go ahead and filter the output of the caption shortcode but this won't help with old themes either and really shouldn't be necessary for something that should ideally be customizable individually.

Last edited 6 years ago by lrdn (previous) (diff)

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


6 years ago

#6 @jakeqz
6 years ago

I've also found this issue and would like to add my voice to those calling for #33981 to be reverted. Just updated to WordPress 4.9 and found images squashed 10px narrower in some themes (which were ultimately based on TwentyTwelve). #33981 was the culprit. Another issue introduced is that a second floated image will (at least in Firefox) be shrunk to fit into the space beside the first, where it should not fit at its intrinsic size.

The change in #33981 breaks themes and has no justification given – no use case example was provided demonstrating why this change was required, only that it was to be "more mobile friendly". However, it is much more common in RD to use max-width: 100% in the CSS to constrain images to the available space, and set specific optimal image widths as inline styles using the width property (NOT max-width!) or via the width attribute – could the issue that #33981 was attempting to address not have been solved like this?

I suggest that a filter is added for $style with the default behaviour reverted to that in 4.8, so that existing themes are not broken and anyone who really needs max-width instead of width can do so be adding a filter, something like this in image_caption_shortcode:

$style = '';
if ( $caption_width ) {
    $style = 'style="width: ' . (int) $caption_width . 'px" ';
}
$style = apply_filters( 'img_caption_shortcode_style', $style, $atts, $content, $caption_width, $html5 );

Currently it is possible to partially revert #33981 by adding a filter, but this is clumsy as it requires adding the filter for every single shortcode and (the example below) assumes no other element in the content will have a max-width style, and it does not cover other uses of img_caption_shortcode such as by the Media Image widget. No other more suitable filter is currently available:

add_filter(
    'do_shortcode_tag', 
    function( $output, $tag, $attr, $m ) {
        if ( $tag === 'wp_caption' || $tag === 'caption' ) {
            $output = str_replace( 'style="max-width:', 'style="width:', $output );
        }
        return $output;
    }, 
    10, 4
);

#7 @jakeqz
6 years ago

See also #43185.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 years ago

#9 @desrosj
6 years ago

#43185 was marked as a duplicate.

#10 @desrosj
6 years ago

#43078 was marked as a duplicate.

#11 @desrosj
6 years ago

Thanks for all of the feedback and reports.

After looking this over, I think we should revert the change but keep the test improvements that were added in r41724. The spirit of the change was well-intentioned, but enough cases have come up in custom themes that show it did not work exactly how we thought it would based on our testing with the default themes.

The change also assumes that a site is responsive, which may not always be the case.

#12 @joemcgill
6 years ago

  • Milestone changed from Awaiting Review to 4.9.5
  • Owner set to joemcgill
  • Status changed from new to assigned

Let's go ahead and revert this to the pre-4.9.0 behavior and but keep the extra test coverage we added.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 years ago

#14 @joemcgill
6 years ago

  • Keywords has-patch has-unit-tests added

43123.diff reverts the shortcode back to its pre-4.9.0 behavior so that an inline width style is set instead of max-width. I've updated the relevant unit tests and left all others intact.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 years ago

@joemcgill
6 years ago

#16 @jakeqz
6 years ago

Let's go ahead and revert this to the pre-4.9.0 behavior

+1. Is it worth a separate ticket to add a filter for the style attribute so that if anyone specifically requires the 4.9.0 behaviour they can implement this filter?

#17 @joemcgill
6 years ago

@jakeqz, I'm not sure an extra filter is necessary here, since someone could filter the output using the do_shortcode_tag filter, like this:

add_filter( 'do_shortcode_tag', function ( $output, $tag ) {
    if ( 'caption' === $tag || 'wp_caption' === $tag ) {
        $output = str_replace( 'width:', 'max-width:', $output );
    }

    return $output;
}, 10, 2 );

#18 @desrosj
6 years ago

  • Keywords commit added

@joemcgill 43123.diff looks good to me!

#19 @joemcgill
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 42837:

Revert max-width styles on caption shortcodes.

This is a partial revert of [41724], so image captions include an
inline width style instead of max-width.

This returns the caption shortcode to the pre-4.9.0 behavior, while
retaining the extra unit test coverage added in [41724].

Fixes #43123. See #33981.

#20 @joemcgill
6 years ago

In 42838:

Revert max-width styles on caption shortcodes.

This is a partial revert of [41724], so image captions include an
inline width style instead of max-width.

This returns the caption shortcode to the pre-4.9.0 behavior, while
retaining the extra unit test coverage added in [41724].

Fixes #43123. See #33981.

Merges [42837] to the 4.9 branch.

This ticket was mentioned in Slack in #forums by ipstenu. View the logs.


6 years ago

Note: See TracTickets for help on using tickets.