| CARVIEW |
#33981 closed enhancement (fixed)
Default Captions Should Use max-width
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| 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 10 years ago.
- 33981.patch (410 bytes) - added by desrosj 9 years ago.
- 33981.2.patch (1.1 KB) - added by joemcgill 8 years ago.
- Update unit tests
- 33981.3.patch (3.5 KB) - added by desrosj 8 years ago.
- 33981.diff (4.1 KB) - added by joemcgill 8 years ago.
- 33981.2.diff (4.2 KB) - added by joemcgill 8 years ago.
Download all attachments as: .zip
Change History (30)
#1
@joedolson
10 years ago
- Focuses accessibility removed
#2
@swissspidy
10 years ago
- Keywords needs-patch added
@aaronrutley
10 years ago
- Attachment 33981-ar.patch added
#3
@aaronrutley
10 years ago
- Keywords has-patch added; needs-patch removed
This ticket was mentioned in Slack in #core-media by desrosj. View the logs.
9 years ago
@desrosj
9 years ago
- Attachment 33981.patch added
This ticket was mentioned in Slack in #core-media by desrosj. View the logs.
8 years ago
#8
@joemcgill
8 years ago
- Owner set to joemcgill
- Status changed from new to assigned
#9
@joemcgill
8 years 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 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
@desrosj
8 years ago
- Attachment 33981.3.patch added
#14
@desrosj
8 years 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 testtest_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$contentis returned unaltered when width is< 1. - Added
test_img_caption_shortcode_empty_caption()to ensure thatnullis returned when$atts['caption']is empty and no$contentis specified. - Added
test_img_caption_shortcode_empty_caption_and_content()to ensure that$contentis 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.
8 years ago
@joemcgill
8 years ago
- Attachment 33981.diff added
#16
@joemcgill
8 years 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
8 years ago
- Attachment 33981.2.diff added
#17
@joemcgill
8 years 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
8 years ago
@joemcgill 33981.2.diff looks good to me.
#19
@joemcgill
8 years ago
- Resolution set to fixed
- Status changed from assigned to closed
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
#21
@jakeqz
8 years ago
It's widely supported and more mobile friendly
An actual use case demonstrating why this change was needed would be helpful.
could changing
widthtomax-widthcause problems...?
Indeed it could – see #43123.
#22
@joemcgill
8 years ago
#23
@joemcgill
8 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].
Merges [42837] to the 4.9 branch.
Refreshed the patch. Minor change, but looks good to me. Tested with all the default themes and no issues.