WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 weeks ago

Last modified 2 weeks ago

#33981 closed enhancement (fixed)

Default Captions Should Use max-width

Reported by: Howdy_McGee Owned by: joemcgill
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.3.1
Component: Media Keywords: has-patch has-unit-tests commit
Focuses: ui Cc:

Description

HTML5 Captions should really be using max-width: image-size + px rather than a static width. It's widely supported and more mobile friendly than the default width style.

<figure id="attachment_116" style="max-width: 400px;" class="wp-caption alignright">
	<img class="wp-image-116 size-full" src="image.jpg" alt="redraw" width="400" height="365">
	<figcaption class="wp-caption-text">Phasellus nec sem in justo pellentesque facilisis.</figcaption>
</figure>

Works just as well - Can I Use: max-width

Attachments (6)

33981-ar.patch (418 bytes) - added by aaronrutley 2 years ago.
33981.patch (410 bytes) - added by desrosj 4 months ago.
33981.2.patch (1.1 KB) - added by joemcgill 8 weeks ago.
Update unit tests
33981.3.patch (3.5 KB) - added by desrosj 3 weeks ago.
33981.diff (4.1 KB) - added by joemcgill 3 weeks ago.
33981.2.diff (4.2 KB) - added by joemcgill 2 weeks ago.

Download all attachments as: .zip

Change History (26)

#1 @joedolson
2 years ago

  • Focuses accessibility removed

#2 @swissspidy
2 years ago

  • Keywords needs-patch added

#3 @aaronrutley
2 years ago

  • Keywords has-patch added; needs-patch removed

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


4 months ago

@desrosj
4 months ago

#5 @desrosj
4 months ago

  • Milestone changed from Awaiting Review to 4.9

Refreshed the patch. Minor change, but looks good to me. Tested with all the default themes and no issues.

#6 @desrosj
4 months ago

  • Keywords commit added

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


8 weeks ago

#8 @joemcgill
8 weeks ago

  • Owner set to joemcgill
  • Status changed from new to assigned

@joemcgill
8 weeks ago

Update unit tests

#9 @joemcgill
8 weeks ago

  • Keywords has-unit-tests 2nd-opinion added; commit removed

@desrosj – Looks like we need to update the unit tests for this too. The only test I can see that currently covers this style in the HTML is the curiously named test_img_caption_shortcode_with_old_format() in 'tests/phpunit/tests/media.php'. It was passing because it uses a loose preg_match_all() where max-width: 20 and width: 20 would both match /width: 20/.

33981.2.patch Updates that test, but I'm wondering if our coverage for img_caption_shortcode() is adequate.

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


8 weeks ago

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


6 weeks ago

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


5 weeks ago

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


4 weeks ago

@desrosj
3 weeks ago

#14 @desrosj
3 weeks ago

In 33981.3.patch, I made a handful of test adjustments:

  • test_img_caption_shortcode_with_bad_attr() was checking that two identical strings were equal and not checking the result of the function. It was also not passing a content parameter. I renamed this test test_img_caption_shortcode_with_empty_params_but_content().
  • Added test_img_caption_shortcode_short_circuit_filter() to test that the function properly returns the results of the short circuit filter.
  • Added test_img_caption_shortcode_empty_width() to ensure that $content is returned unaltered when width is < 1.
  • Added test_img_caption_shortcode_empty_caption() to ensure that null is returned when $atts['caption'] is empty and no $content is specified.
  • Added test_img_caption_shortcode_empty_caption_and_content() to ensure that $content is returned unaltered when $atts['caption'] is empty.

Makes the tests for that function slightly better, but we probably could still add some more scenarios.

One thing I thought of when diving in, could changing width to max-width cause problems with the img_caption_shortcode_width filter where someone needs the caption to be guaranteed a certain width? I think it is possible, but I don't know if that should hold this small change back.

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


3 weeks ago

@joemcgill
3 weeks ago

#16 @joemcgill
3 weeks ago

  • Keywords commit added; 2nd-opinion removed

@desrosj, 33981.3.patch looks good to me. It may be a bit cleaner to use a data provider for the new shortcode tests, but that's certainly not a blocker. While testing this I noticed an additional test that was failing in tests/phpunit/tests/shortcode.php, which I've updated in 33981.diff.

@joemcgill
2 weeks ago

#17 @joemcgill
2 weeks ago

33981.2.diff is the same as 33981.diff, but removes the use of an anonymous callback function in a filter in the Tests_Media::test_img_caption_shortcode_short_circuit_filter() unit test to avoid failures on PHP 5.3 and below.

#18 @desrosj
2 weeks ago

@joemcgill 33981.2.diff looks good to me.

#19 @joemcgill
2 weeks ago

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

In 41724:

Media: Use max-width for default captions.

This alters the HTML output of the image caption shortcode to use
max-width instead of width to improve compatibility with
flexible layouts.

Props aaronrutley, desrosj.
Fixes #33981.

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


2 weeks ago

Note: See TracTickets for help on using tickets.