Make WordPress Core

Opened 15 years ago

Closed 13 years ago

Last modified 9 years ago

#10734 closed defect (bug) (fixed)

Gallery Shortcode Causes XHTML Validation Failure

Reported by: achmafooma's profile achmafooma Owned by:
Milestone: 3.9 Priority: normal
Severity: normal Version:
Component: Gallery Keywords:
Focuses: Cc:

Description

When placing a gallery in an entry, the gallery shortcode places a style tag in the body. This is prohibited under XHTML (style tags must be in the head) and causes an XHTML validation failure.

Any style tags generated by WordPress need to go into the head, not the body, so all our sites pass XHTML validation.

This was mentioned in a few other bugs, but there didn't seem to be a bug specifically for dealing with this (at least not that I could find). Sorry if it's a dupe :-).

Attachments (5)

gallery_shortcode_style.diff (2.4 KB) - added by achmafooma 15 years ago.
Proposed patch
media.diff (1.6 KB) - added by zamoose 14 years ago.
A new spin on the gallery shortcode.
style.diff (960 bytes) - added by zamoose 14 years ago.
Styling changes for default style.css
platformbefore.jpg (63.7 KB) - added by chipbennett 13 years ago.
Platform Theme with inline style
platformafter.jpg (44.7 KB) - added by chipbennett 13 years ago.
Platform Theme without inline style

Download all attachments as: .zip

Change History (51)

#1 @achmafooma
15 years ago

  • Cc achmafooma added

#2 @scribu
15 years ago

  • Keywords needs-patch added
  • Milestone changed from Unassigned to 2.9

#4 @josephknight
15 years ago

  • Cc josephknight added

Please remove the CSS period,
The theme CSS should be in control of the styling and the markup separate. Adding CSS to the markup only forces us to use "!important" hacks and other similar headaches in our theme's CSS.

Moving the hard CSS to the header is just moving the problem to another area of the markup. Let us style our own themes please. When removing the hard gallery CSS from the media.php, if the gallery doesn't appear correctly in the default theme then edit the default theme instead of forcing styling on us.

To those making the decisions, thanks for reading and thank you for this awesome publishing platform.

@achmafooma
15 years ago

Proposed patch

#6 @achmafooma
15 years ago

Okay... since posting this bug I've become the maintainer of the aforementioned 'Gallery Shortcode Style to Head' plugin (go figure).

I've attached a patch to media.php that fixes this issue. I used the same approach used in the plugin (props to its original author, Matt Martz [sivel], who did all the hard work here).

Please be kind ;-) this is my first attempt ever submitting a patch for WordPress.

All it does is move the style tag into the page head (which requires a couple new functions to read-ahead for the gallery shortcode).

#7 @achmafooma
15 years ago

  • Keywords has-patch added; needs-patch removed

#8 @Viper007Bond
14 years ago

  • Milestone changed from 2.9 to Future Release

Bumping from 2.9.

Also using query_posts() likely break this patch as that modifies $posts after the fact. Probably best to just always load some CSS (external stylesheet, disabled via function that the theme author can call to tell WP that they have the CSS in their theme's CSS or something).

Another plugin also uses classes instead of an inline width to control column width. Seems much cleaner.

#9 @zamoose
14 years ago

WPEngineer suggested something similar:
http://wpengineer.com/a-solution-for-the-wordpress-gallery/

I'm attaching the .diff I propose that uses his methodology for source:trunk/wp-includes/media.php

It seems like the cleanest of all approaches as it modifies very little but takes all the hardcoded styling explicitly out of the body.

@zamoose
14 years ago

A new spin on the gallery shortcode.

@zamoose
14 years ago

Styling changes for default style.css

#11 @zamoose
14 years ago

Downside of WPE's approach:

  • Themes that don't support the CSS styling will revert galleries to just a single column of images

Upsides:

  • Removes another XHTML hiccup
  • Doesn't mess with any of the queries
  • Saves on one very quick calculation -- hey, every cycle counts! *grin*

#12 @zamoose
14 years ago

  • Cc zamoose added

#13 @archon810
14 years ago

  • Cc admin@… added

I'd like to add to this discussion that this inline style breaks plugins that override the excerpt function, specifically http://wordpress.org/extend/plugins/advanced-excerpt.

Advanced Excerpt is quite useful but if a gallery is inserted, the style tags are stripped and the CSS is displayed in clear text.

A partial workaround is to modify wp-includes/kses.php and add 'style' to the list, then check it in Advanced Excerpt's configuration, so that it doesn't strip the <style> tags.

Another workaround is to use the plugin mentioned earlier in this ticket.

I am quite shocked, frankly, that the gallery does something this irresponsible.

#14 @jenspr
14 years ago

  • Cc jenspr added

#15 @johnbillion
13 years ago

  • Keywords has-patch removed
  • Resolution set to fixed
  • Status changed from new to closed

Fixed with the introduction of the 'use_default_gallery_style' filter in r16865.

#16 @hakre
13 years ago

Related: #15103

#17 follow-up: @chipbennett
13 years ago

Does r16865 change the output location of the generated <style> content, so that it outputs in the document head, rather than in the body? If it doesn't, and it is still being output inline, does that changeset actually fix the ticket issue?

(I'd love to hear that I'm misunderstanding the changeset. This issue is standards non-compliant, and is a frequent annoyance for the Theme Review Team.)

#18 in reply to: ↑ 17 @hakre
13 years ago

Replying to chipbennett:

Does r16865 change the output location of the generated <style> content, so that it outputs in the document head, rather than in the body?

No, not at all as the code shows.

You need to bring it together with some additional code to "fix" the issue:

add_filter( 'use_default_gallery_style', '__return_false' );

and provide the needed CSS on your own.

See WP 3.1 Default Theme functions.php

#19 follow-up: @chipbennett
13 years ago

Then I don't think this is an acceptable solution.

The default WordPress behavior should be standards-compliant, and hacks should be added via filter - not the other way around.

#20 in reply to: ↑ 19 ; follow-up: @nacin
13 years ago

  • Milestone Future Release deleted
  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to chipbennett:

Then I don't think this is an acceptable solution.

The default WordPress behavior should be standards-compliant, and hacks should be added via filter - not the other way around.

We don't have much of a choice here. Shortcodes aren't processed until after we're generating the body. A theme can work around this. Indeed, the default WordPress behavior for both Twenty Ten and Twenty Eleven is compliant.

Eventually, the standard will catch up and allow us to set a scope for <style> in <body>.

#21 @nacin
13 years ago

  • Resolution set to maybelater
  • Status changed from reopened to closed

#22 in reply to: ↑ 20 @chipbennett
13 years ago

Replying to nacin:

The default WordPress behavior should be standards-compliant, and hacks should be added via filter - not the other way around.

We don't have much of a choice here. Shortcodes aren't processed until after we're generating the body. A theme can work around this. Indeed, the default WordPress behavior for both Twenty Ten and Twenty Eleven is compliant.

Eventually, the standard will catch up and allow us to set a scope for <style> in <body>.

But, this is entirely non-essential code. WordPress core doesn't need to define gallery style, any more than WordPress needs to define style for any other markup.

Removing it won't "break" Themes; though Theme developers would need to be made aware that they'll need to incorporate the style definition into the Theme.

#23 follow-up: @dd32
13 years ago

Removing it won't "break" Themes;

So.. Removing styling that Themes are relying upon isn't breaking the theme?

#24 in reply to: ↑ 23 @chipbennett
13 years ago

Replying to dd32:

Removing it won't "break" Themes;

So.. Removing styling that Themes are relying upon isn't breaking the theme?

No. It won't.

Come now; let's be reasonable. The styles that are being applied are: some margin, image border, width, and float. Removing any or all of that won't break a Theme.

The only one that might be even marginally significant is the float:left. And even if it isn't there, the Theme doesn't break; from a rendered display perspective, gallery images simply might behave as block-level, instead of inline.

#25 follow-up: @Viper007Bond
13 years ago

What's more important -- making sure galleries look presentable for millions of users or conforming to a technical standard? I think you have your priorities out of order personally.

Don't get me wrong -- we should totally obey standards, but not at the cost of usability.

#26 in reply to: ↑ 25 ; follow-up: @chipbennett
13 years ago

Replying to Viper007Bond:

What's more important -- making sure galleries look presentable for millions of users or conforming to a technical standard? I think you have your priorities out of order personally.

Don't get me wrong -- we should totally obey standards, but not at the cost of usability.

It's not only about the standard. Inline CSS is a pain in the rear for Theme developers to override - and it is the Theme developers who should be defining presentation style for the Gallery markup, just like all the rest of the markup generated by core.

Besides, simply removing the inline style generated by core will not cause galleries to look "unpresentable" for "millions of users". Such an assertion is hyperbole to the extreme.

#27 in reply to: ↑ 26 ; follow-up: @johnbillion
13 years ago

Replying to chipbennett:

It's not only about the standard. Inline CSS is a pain in the rear for Theme developers to override

I don't think one line of code:

add_filter( 'use_default_gallery_style', '__return_false' );

is that hard for any developer to add to a theme.

#28 in reply to: ↑ 27 @chipbennett
13 years ago

Replying to johnbillion:

Replying to chipbennett:

It's not only about the standard. Inline CSS is a pain in the rear for Theme developers to override

I don't think one line of code:

add_filter( 'use_default_gallery_style', '__return_false' );

is that hard for any developer to add to a theme.

And now we're arguing in circles.

No, it's not hard. But it's the polar opposite of what should be done. Theme Developers shouldn't have to undo something that should never have been done in the first place. Theme developers shouldn't have to fix core's invalid markup. Theme developers shouldn't have to implement workarounds to undo arbitrary styling.

Core does not define styling for any other markup. Why does gallery markup get this special dispensation - and to the degree that it is deemed so vitally important that it is allowed to cause core to output invalid markup in order to implement?

Further: the argument that it's easy to fix rather invalidates the hyperbolic assertion that fixing it would cause breakage for millions of users.

Last edited 13 years ago by chipbennett (previous) (diff)

@chipbennett
13 years ago

Platform Theme with inline style

@chipbennett
13 years ago

Platform Theme without inline style

#29 @chipbennett
13 years ago

Just to prove the point, please refer to the before and after screenshots of Platform Theme (chosen simply because it is currently the "Most Popular" Theme). Clearly, there is no "breakage".

Note also: the default Theme, TwentyTen, already handles gallery styling on its own.

#30 @azaozz
13 years ago

I don't understand the point of view. This (long) discussion would have been in place when this was added to core, not few years after that. And yes, the gallery is definitely broken in platformafter.jpg, it is in a long column of one image per row.

Lets move the <style> tag inside the main <div> and add scoped="scoped" to it. That should stop all this. Themes that don't (want to) support HTML 5.0 can disable it.

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

#31 follow-up: @zamoose
13 years ago

@azozz:
The change was controversial at the time. Many people worked out work-arounds almost as soon as the feature was released(1)(2)(3). It was discussed ad nauseum and the core devs pressed ahead with the (broken) implementation. So don't go acting as though those who have come late to the discussion are somehow invalid or would need access to a Delorean in order for their opinions to count.

The point is, the core behavior in this instance is backwards and has been annoying since the day it was introduced. It's never too late to address a historical mistake.

  1. http://justintadlock.com/archives/2008/04/13/cleaner-wordpress-gallery-plugin
  2. http://zeo.unic.net.my/remove-wordpress-gallery-shortcode-embedded-css/
  3. http://wpengineer.com/1802/a-solution-for-the-wordpress-gallery/
Last edited 13 years ago by zamoose (previous) (diff)

#32 @zamoose
13 years ago

Further, this was discussed on WP Hackers at the time. (Ref. http://lists.automattic.com/pipermail/wp-testers/2008-March/007477.html) This has been a problem/issue of disagreement since it first went in core.

#33 in reply to: ↑ 31 @aaroncampbell
13 years ago

Replying to azaozz:

Lets move the <style> tag inside the main <div> and add scoped="scoped" to it. That should stop all this. Themes that don't (want to) support HTML 5.0 can disable it.

+1, although it looks to me like the HTML5 specs say that a style element can be in the body in pretty much any block-level element (parents can be "any element that can contain metadata elements, div, noscript, section, article, aside"). I like the idea of implementing "scoped" but I don't think it's entirely necessary. The truth is that this was a limitation of the old specs. The browsers saw this (is there any browser the current implementation doesn't work right in?) and now the specs are being updated to reflect it.

Replying to zamoose:

The change was controversial at the time. Many people worked out work-arounds almost as soon as the feature was released(1)(2)(3). It was discussed ad nauseum and the core devs pressed ahead with the (broken) implementation. So don't go acting as though those who have come late to the discussion are somehow invalid or would need access to a Delorean in order for their opinions to count.

If you voiced your opinion and argued your case at the time, then you did everything you could do. It's not that your opinion didn't count, it's that you were outvoted (or overruled if that's how you care to look at it I suppose) and you're upset. Not a single person in the community (not even any of the core devs) gets their way all the time. It's part of what makes WordPress appealing to millions instead of perfect for a handful of people.

#34 @zamoose
13 years ago

@aaroncampbell:
I'm "upset" inasmuch as azozz, et al., seem to be indicating that latecomers to the debate have no right to speak up.

#35 @archon810
13 years ago

  • Cc admin@… removed

#36 follow-up: @aaroncampbell
13 years ago

Replying to zamoose:

@aaroncampbell:
I'm "upset" inasmuch as azozz, et al., seem to be indicating that latecomers to the debate have no right to speak up.

That's "kind of" the case. It like US politics...you can cast your vote for an official but it needs to be done during the voting process. If you want to remove the official once they're in you need some REALLY serious reasons (impeachment).

Basically, it's not that a late opinion doesn't count, it's that it can't be given the same weight as it would have been given if it were on time. The reason is that the threshold for changing something that has already been released is WAY higher than the threshold for changing something that hasn't (and I think that's the way it SHOULD be).

#37 in reply to: ↑ 36 @chipbennett
13 years ago

Replying to aaroncampbell:

It like US politics...you can cast your vote for an official but it needs to be done during the voting process. If you want to remove the official once they're in you need some REALLY serious reasons (impeachment).

Probably not the most apropos analogy. Presidential impeachment would be more analogous to removing the Hooks API, not something as trivial as a few lines of CSS.

Basically, it's not that a late opinion doesn't count, it's that it can't be given the same weight as it would have been given if it were on time. The reason is that the threshold for changing something that has already been released is WAY higher than the threshold for changing something that hasn't (and I think that's the way it SHOULD be).

And if that's going to be the case, then the powers-that-be need to be considerably less capricious regarding the addition of such code, and considerably more thoughtful of the unintended consequences thereof.

I don't argue that removal would now bear consequences; rather, I'm arguing that the benefit of removing the code would far outweigh the detriment of removing it, and that the detriment of leaving it in far outweighs the benefit of leaving it in.

I don't argue that the threshold for removing code once added should be higher than the threshold for adding that code to begin with; rather, I'm questioning what I perceive to be a complete unwillingness even to consider removing the code.

At what threshold would this code-removal be considered? Already, the default Theme (TwentyTen) accounts for the fix. I assume that the next default Theme (TwentyEleven) will also account for the fix. Several of the high-profile commercial Theme developers already account for the fix. Many other third-party Theme developers also account for the fix - and moreso, with more education of the issue.

#38 follow-up: @azaozz
13 years ago

I still don't understand the reasons why few people here would like to change/revert this. Couple of years ago a decision was made to use the de facto standard (supported by all browsers) instead of the "written" standards (HTML 4 and XHTML 1).

Recently the de facto standard was accepted in the "written" standard (HTML 5). In that terms proposing to support the soon to be outdated XHTML 1 is indeed hard to understand.

Furthermore this ticket seems to contribute very little in terms of fixing bugs or adding new features. If somebody wants to advocate compliance with (soon to be outdated) standards, there are better places for it. For example wp-hackers, #wordpress-dev, etc. Trac is for code, bugs, patches and for discussions about code, bugs and patches.

@aaroncampbell as far as I understand, scoped="scoped" tells the browser to continue and apply the styles only to this branch of the dom instead of stopping and recalculating the styles, so it speeds up the rendering with few milliseconds.

#39 in reply to: ↑ 38 @aaroncampbell
13 years ago

Replying to azaozz:

@aaroncampbell as far as I understand, scoped="scoped" tells the browser to continue and apply the styles only to this branch of the dom instead of stopping and recalculating the styles, so it speeds up the rendering with few milliseconds.

That's how I understand it as well. I agree we should use it, I was just pointing out that it isn't necessary to comply with the new standards.

#40 @cramdesign
12 years ago

I am amazed that this is even in question. Inline styling should definitely be removed. WordPress by default should only output standards compliant code. I really hope that 3.4 addresses media handling and the gallery shortcode. It is way past time. I am so grateful for Chip's voice of reason in all of this.

For those worried that it would break older themes, we have to move ahead at some point. Standards compliance really is the way forward. I remember a lot of similar arguments about css vs table layout just a few years ago.

#41 @cramdesign
12 years ago

  • Cc matt@… added

#42 @tar.gz
12 years ago

  • Cc code@… added

#43 @tar.gz
11 years ago

A little addition to this: the current gallery code doesn't work well for fluid/responsive layouts, because it injects a line break with <br style="clear: both"> after every third item (or whatever column number has been chosen). This is pretty much impossible to override with CSS alone.

A clean solution would be to get rid of the <br/> and set the line returns by using the :nth-child() CSS selector - then a responsive theme can override it easily with it's own CSS.

The downside is that :nth-child is supported only from IE9 onwards.

#44 @nacin
10 years ago

  • Milestone set to 3.9
  • Resolution changed from maybelater to fixed

Calling this fixed by #26697.

#45 @SergeyBiryukov
9 years ago

#28065 was marked as a duplicate.

#46 @SergeyBiryukov
9 years ago

#31357 was marked as a duplicate.

Note: See TracTickets for help on using tickets.