Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#26642 closed task (blessed) (fixed)

HTML5 captions

Reported by: looimaster's profile Looimaster Owned by: azaozz's profile azaozz
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.8
Component: Media Keywords: has-patch
Focuses: Cc:

Description

In /wp-includes/media.php in img_caption_shortcode() there is a note saying By default, the caption is 10 pixels greater than the width of the image, to prevent post content from running up against a floated image.

but this makes the caption text misaligned (too long) 10px in all the possible cases:

  • image floating left
  • image floating right
  • image centered
  • image with alignnone

Caption text is longer than the image. Not exactly below the image.

Furthermore, if an image without caption (for example 150px wide) is placed below/above the image with caption (150px converted to 160px) and both are left aligned then it forms a misaligned grid (all images with captions push the text 10px to the right).

When I remove this 10px increment, everything looks exactly like expected. Post content doesn't run onto the image or caption in any case and I tested them all. I'm wondering if this 10px increment should be there? What does it do? Maybe that's a bug?

Attachments (19)

26642.patch (712 bytes) - added by bcworkz 11 years ago.
26642.diff (3.2 KB) - added by obenland 11 years ago.
Introduce HTML5 support for captions, conveniently ignoring the 10px issue.
26642.2.diff (2.7 KB) - added by obenland 11 years ago.
26642.3.diff (3.0 KB) - added by azaozz 11 years ago.
26642.4.diff (2.4 KB) - added by jond3r 11 years ago.
Introduce a new theme support 'no-inline-style-captions'
26642.5.diff (1.1 KB) - added by jond3r 11 years ago.
A unit test for 26642.4.diff
26642.5.1.diff (1.0 KB) - added by jond3r 11 years ago.
Improved unit test, not using regexp.
captions-wrapping.png (80.5 KB) - added by azaozz 11 years ago.
Default style.png (59.2 KB) - added by jond3r 11 years ago.
No style.png (59.4 KB) - added by jond3r 11 years ago.
26642.6.diff (4.9 KB) - added by jond3r 11 years ago.
Updated patch with unit test
No add 10px.png (57.8 KB) - added by jond3r 11 years ago.
26642.7.diff (5.0 KB) - added by jond3r 11 years ago.
Code and test for the option "don't add 10px"
26642.8.diff (3.6 KB) - added by obenland 11 years ago.
caption-wrap-table.png (91.1 KB) - added by azaozz 11 years ago.
26642.9.diff (3.8 KB) - added by obenland 11 years ago.
26642.10.diff (4.9 KB) - added by azaozz 11 years ago.
26642.11.diff (457 bytes) - added by obenland 10 years ago.
caption-pure-css-for-local-testing.zip (1.3 KB) - added by afercia 10 years ago.

Download all attachments as: .zip

Change History (59)

#1 @SergeyBiryukov
11 years ago

  • Component changed from General to Media

Related: #14380, #17044.

@bcworkz
11 years ago

#2 @bcworkz
11 years ago

  • Cc bcworkz added
  • Keywords has-patch 2nd-opinion added
  • Type changed from defect (bug) to enhancement

I agree the extra 10 pixels added to the caption content container was a poor decision. Such issues as post content running into caption text is supposed to be dealt with by CSS and themes. But it's been this way for at least 6 years and was a conscious decision, so we can't call it a bug. I'm changing it to enhancement request, as I would like to see the 10 px removed from the caption on principle, even though filtering it to one's desire is super easy.

The issue though is most decent themes have compensated for this, so removing the 10 px will break many of the themes (depending on how they compensated in the first place) that were most carefully crafted to ensure the content lined up. It's a no win enhancement. But conceptually, the separation of content and style dictates this spacing is in the wrong place and should be removed. Thus, I'm providing a patch even though I'd be surprised if this gets far.

#3 @helen
11 years ago

Isn't this a duplicate of #14380?

#4 @ocean90
11 years ago

  • Keywords has-patch 2nd-opinion removed
  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

bcworkz, we can't remove the default value for backward compatibility reasons.

Looimaster, you can use the filter (added in [25444]) to remove the 10px (see below) and add your own styles to make it pretty in your theme.

add_filter( 'img_caption_shortcode_width', '__return_zero' )

#5 @SergeyBiryukov
11 years ago

#27301 was marked as a duplicate.

#6 @sonic1243
11 years ago

I can understand this is difficult after many years of work-arounds, but it's got to go sometime ? I'm mainly concerned with the presence of this problem in the editor, I'm really impressed with the way editor-styles work, but the extra 10px makes it look like I've screwed something up:

https://core.trac.wordpress.org/raw-attachment/ticket/27301/10px-problem-editor-style.jpg

#7 @Looimaster
11 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

ocean90:
WordPress 3.8 introduced MP6 that broke admin panel designs of a lot of plugins and themes. Many needed an update of styles. Many still don't work with this new skin.

WordPress 3.9-alpha switched from TinyMCE 3.x to 4.x, breaking 95% of TinyMCE plugins overnight.

So, that is allowed and fixing this little issue is not allowed?

An issue that is troubling many developers has been identified and has a patch now. I think it should be fixed right now because a lot of people are puzzled why this happens and it produces invalid look of images unless a theme adds some workarounds in style.css.

sonic1243: Agree, it has to change someday or more people will be entering bug tracker and reporting the same issue.

#8 @SergeyBiryukov
11 years ago

  • Milestone set to Awaiting Review

#9 follow-up: @nacin
11 years ago

  • Keywords close added

Causing breakage in the admin is a far cry from breaking the theme's design.

#10 in reply to: ↑ 9 @sonic1243
11 years ago

Replying to nacin:

Causing breakage in the admin is a far cry from breaking the theme's design.

My point was simply that we have a work-around for the public site, but there is no work-around for the editor without editing the core files directly?
This bug/error belongs in the history books ... Code is poetry...

@obenland
11 years ago

Introduce HTML5 support for captions, conveniently ignoring the 10px issue.

This ticket was mentioned in IRC in #wordpress-dev by obenland. View the logs.


11 years ago

@obenland
11 years ago

@azaozz
11 years ago

#12 @azaozz
11 years ago

In 26642.3.diff: tweaked 26642.2.diff a bit to avoid JS type juggling in wpeditimage (we got burned by that :) ).

#13 @SergeyBiryukov
11 years ago

  • Keywords close removed

Introduce HTML5 support for captions, conveniently ignoring the 10px issue.

Related: #27292

#14 @jond3r
11 years ago

Hello, jumping in from #27292 to this hijacked ticket ;). Seems like obenland and I was thinking about captions and HTML5 about the same time. My two cents on various topics:

About adding an inline style width + 10px: As ocean90 pointed out (#comment:4) this can be removed on the front-end using the filter img_caption_shortcode_width. This filter does not work in the visual editor though (?). Uncertain if it is possible to get it working there.

Obenland suggests that by adding a new html5 theme support 'caption', the markup should change to use <figure> and <figcaption> elements and skip the inline width style altogether. Also bypassing the img_caption_shortcode_width filter. One could argue that this special treatment of HTML5-supported captions is not fully logical. Removal of an inline style is unrelated to using HTML5 markup.

Using an inline width style for an image with a caption can be useful. For example, say that you have a small (narrow) image with a fairly long caption. Then using an inline width style that is wider than the image can make things look better, as there will be fewer line breaks in the caption. As the image width might be unknown to the style sheet, using an inline style might be warranted.

The patch in #27292 also utilizes the 'caption' html5 support to get <figure> and <figcaption> elements but does not change the treatment of the inline style.

In case the approach in the current thread is adopted, the unit test provided in #27292 can still be used. (Though it could be tweaked to explicitly check for the presence of a style attribute if desired).

@jond3r
11 years ago

Introduce a new theme support 'no-inline-style-captions'

#15 @jond3r
11 years ago

This is a bit crazy idea I got, but it would solve the original inline style problem without mixing in HTML5 support.

Patch 26642.4 borrows heavily from 26642.3, but instead makes use of a new theme support 'no-inline-style-captions'.

@jond3r
11 years ago

A unit test for 26642.4.diff

@jond3r
11 years ago

Improved unit test, not using regexp.

#16 @obenland
11 years ago

The added 10px of width have been confusing to theme developers for a while now. The introduction of the shortcode_atts_* filter made that a little better (in a hacky way), but hasn't really resolved that.

With the the switch to HTML5 we finally have the opportunity to stop the madness. You can see declaring HTML5 support as a switch of context for the individual feature, like galleries that won't output default styles when the theme added support for HTML5 gallery markup (r27396).

#17 @helen
11 years ago

Given that the inline width is there for legacy reasons, and that introducing yet-another-argument for theme support means maintaining it into the future (and isn't really any more dev-friendly than the existing filter), I think dropping it with the introduction of HTML5 support is a perfectly sane route. It's not any different from what was done with galleries.

#18 @azaozz
11 years ago

Also the current patches here remove the style="width..." from the wrapper element. That would make it impossible to center the caption wrapper (the image can be centered but the caption text would be centered too) and the text won't wrap around under the image when longer. Is that intended for the HTML 5.0 captions?

Last edited 11 years ago by azaozz (previous) (diff)

#19 @jond3r
11 years ago

I think azaozz has a point here. I did some more testing with short and long caption text, using the default styling of the wrapper element compared to no inline styling. In both cases I used the default editor styling of the captioned image. See the attached images. Obviously there are consequences of skipping the inline style, desired or not.

Additionally I found a few more places in wpeditimage/plugin.js where the style="width: +10px" was added (e.g. when editing an image). The attached new patch .6 accounts for this.

@jond3r
11 years ago

@jond3r
11 years ago

@jond3r
11 years ago

Updated patch with unit test

#20 @jond3r
11 years ago

Note that in the above attached Default style.png image my intention was to change the caption text to "default-style-caption" or something similar. You have to imagine that. Sorry for the inconvenience.

#21 follow-up: @jond3r
11 years ago

Sorry for all the spam here, but I just thought about another option that maybe azaozz was looking for: Keep the inline style but don't add 10 px. The image below shows how that looks in the editor.

To accomplish that I introduced a theme support called 'no-add-10px-captions', but that could of course be called something else. I don't want to introduce HTML5 markup here, since I think that belongs to #27292 (though of course I'm biased here).

Update: There is now a new patch in #27292, that removes the by-default added 10 px, as well as introduces HTML5 markup.

Last edited 11 years ago by jond3r (previous) (diff)

@jond3r
11 years ago

@jond3r
11 years ago

Code and test for the option "don't add 10px"

@obenland
11 years ago

#22 @obenland
11 years ago

  • Milestone changed from Awaiting Review to 3.9

Replying to azaozz:

Also the current patches here remove the style="width..." from the wrapper element. That would make it impossible to center the caption wrapper […] Is that intended for the HTML 5.0 captions?

Themes can use display: table; on centered captions. While this doesn't work for old versions of IE, HTML5 in general doesn't work there either.

I added mce styles to make HTML5 captions look as close to its HTML4 predecessors as possible. Example

#23 in reply to: ↑ 21 ; follow-ups: @azaozz
11 years ago

Replying to jond3r:

I just thought about another option that maybe azaozz was looking for: Keep the inline style but don't add 10 px.

Yes, exactly.

Replying to obenland:

Themes can use display: table; on centered captions.

Ah, the "ancient" layout table. Yes, this solves the centered caption problem (as long as themes are OK with being incompatible with some browsers), but the bigger problem of wrapping caption text under the image still remains.

Thinking we should do exactly what this ticket was opened for: remove the extra 10px but keep the inline stile as there's no good way to wrap the text under the image without it.

#24 in reply to: ↑ 23 @obenland
11 years ago

Replying to azaozz:

Thinking we should do exactly what this ticket was opened for: remove the extra 10px but keep the inline stile as there's no good way to wrap the text under the image without it.

We came so close. But you're right, I can't think of a better way than keeping the style attribute around either.

@obenland
11 years ago

#25 @obenland
11 years ago

In 26642.9.diff:

We still use an inline style in the HTML5 version to encourage captions to break. The width is not filtered since the original purpose of the existing img_caption_shortcode_width filter was to give theme authors a way to circumvent the added 10 pixels.

On the editor side we added a flag to signal html5 caption support. Based on that flag the editor now adds those 10px (or not), and gives captions a general 4px padding (or not).

@azaozz
11 years ago

#26 follow-up: @azaozz
11 years ago

In 26642.10.diff, tweak 26642.9.diff a bit and add checks for wpeditimage_html5_captions in few more places.

Thinking we should be bolder in the default style for html5 captions inside the editor. Perhaps remove the background, margin, padding, and container border, etc.

#27 @nacin
10 years ago

  • Summary changed from [caption] is 10px too wide to HTML5 captions
  • Type changed from enhancement to task (blessed)

#28 @nacin
10 years ago

In 27668:

Introduce HTML5 caption support.

When a theme supports HTML5 captions via add_theme_support( 'html5', 'caption' ), figure and figcaption will be used instead of div and p.

There's a bonus. But first, some history: Captions were introduced with an inline style set for the container. This remains, as it is there to force captions to wrap. But this inline style included an extra 10 pixels, which have vexxed theme developers for years. While these pixels were designed to ensure padding around floated images, modern themes handle this with grace. The additional pixels thus feel encumbering.

As the new HTML5 gallery support avoids outputting default gallery styles (again, irking theme developers for years; see #26697), the new HTML5 caption support will also ditch these 10 pixels of unwanted hand-holding.

The 10 pixels are also removed entirely in the visual editor (and more styles may also disappear here; see #26642), giving themes the power necessary to match the frontend styles.

The filter img_caption_shortcode_width added in 3.7 to work around this madness (see #14380) is skipped entirely when the theme supports HTML5 captions.

props obenland, azaozz.
see #26642. also fixes #9066.

#29 in reply to: ↑ 26 @nacin
10 years ago

Leaving this open so we could look into this:

Replying to azaozz:

Thinking we should be bolder in the default style for html5 captions inside the editor. Perhaps remove the background, margin, padding, and container border, etc.

#30 @sonic1243
10 years ago

"Perhaps remove the background, margin, padding, and container border, etc."
Yes please! I'm having to add css to override the default tinymce theme's caption's border/background, also the alignnone/left/right/center classes, and even the font in tables (my unanswered forum post a few days ago here: http://wordpress.org/support/topic/tinymcethemes-css-overriding-my-editor-style?replies=1). Hope I've understood you correctly... perhaps editor styles aren't often taken as seriously as I am taking them?!

#31 @nacin
10 years ago

In 27758:

Add an .html4-captions class to the editor when HTML5 captions are not supported, for theme use.

see #26642.

#32 @lancewillett
10 years ago

In 27763:

Twenty Thirteen and Fourteen: implement HTML5 markup for galleries (see #26697) and captions (see #26642). Props obenland, closes #27502.

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


10 years ago

@obenland
10 years ago

#34 @obenland
10 years ago

  • Keywords has-patch added

Patch removes basic styling and brings captions in sync with how non-captioned images are displayed.
Before and after: https://cloudup.com/cDOLFPmKdqr

#35 in reply to: ↑ 23 @afercia
10 years ago

but the bigger problem of wrapping caption text under the image still remains.

you may want to try with display: table-caption; and caption-side: bottom; applied to the caption, it will work as long as the caption container will have `display: table;

Themes can use the same solution. Added bonus: just switching to caption-side: top; will display the caption on top.

see http://jsfiddle.net/6XZkN/
attached a .zip with a simple html to be tested on a local server

#36 @nacin
10 years ago

  • Owner set to azaozz
  • Status changed from reopened to assigned

Final tweaks need review.

#37 @azaozz
10 years ago

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

In 27949:

TinyMCE: update the default style for html5-captions, props obenland fixes #26642

#38 follow-up: @ScottSmith
10 years ago

When HTML 5 captions are enabled, the editor still uses dd and dt tags, instead of figure and figcaption. What is the reason for this? It seems to me like the tags should be consistent across the editor and front-end.
Sorry to add to this thread, but I couldn't find the answer to this question here.

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


10 years ago

#40 in reply to: ↑ 38 @azaozz
10 years ago

Replying to ScottSmith:

Inside the editor the DL node is used in managing the caption. It has always been different from the tags outputted on the front-end. Having different nodes for html4 and html5 captions would double that code :)

Note: See TracTickets for help on using tickets.