Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#20448 closed task (blessed) (fixed)

Update Twenty Ten and Twenty Eleven to use 3.4 features

Reported by: nacin's profile nacin Owned by: koopersmith's profile koopersmith
Milestone: 3.4 Priority: high
Severity: major Version: 3.4
Component: Bundled Theme Keywords: has-patch
Focuses: Cc:

Description (last modified by nacin)

Twenty Eleven can be given three distinct updates for 3.4:

  • Flexible heights for headers (when a post thumbnail header is not in use)
  • Adding Twenty Eleven's theme options to customize
  • Providing Text Domain and Domain Path headers

Additionally, a theme just doesn't need a $locale.php file — indeed, core almost never needs one either anymore. I suggest we remove the reference, something I had already done for Twenty Twelve when it was in trunk.

Attachments (24)

20448.diff (3.9 KB) - added by nacin 12 years ago.
20448.twentyeleven.diff (1.4 KB) - added by Otto42 12 years ago.
Patch for twentyeleven to support postMessage updating of title, description, and header text-color
20448.twentyeleven.2.diff (2.8 KB) - added by Otto42 12 years ago.
Add color scheme and partially working link-color options to customizer
20448.twentyeleven.3.diff (3.7 KB) - added by Otto42 12 years ago.
Add layouts too
20448.textdomain.diff (1.4 KB) - added by SergeyBiryukov 12 years ago.
20448.2.diff (1.4 KB) - added by ryan 12 years ago.
default option filters
20448-ut.diff (2.7 KB) - added by ryan 12 years ago.
unit tests for default filters
20448.twentyeleven.4.diff (4.9 KB) - added by lancewillett 12 years ago.
Update for Twenty Eleven Theme Customizer integration
20448.rtl.patch (625 bytes) - added by ocean90 12 years ago.
20448.3.diff (1.8 KB) - added by nacin 12 years ago.
20448.4.diff (1.2 KB) - added by kobenland 12 years ago.
Specify default-color argument when calling add_custom_background() in Twenty Ten and Twenty Eleven. :)
20448.5.diff (2.6 KB) - added by nacin 12 years ago.
20448.6.diff (2.8 KB) - added by ryan 12 years ago.
With conditional from beyond
20448-default-bg.diff (1.6 KB) - added by ryan 12 years ago.
20448-remove-bg-image.diff (516 bytes) - added by mfields 12 years ago.
20448-remove-bg-image.2.diff (538 bytes) - added by mfields 12 years ago.
20448.7.diff (2.0 KB) - added by ryan 12 years ago.
Combined
20448.8.diff (1.5 KB) - added by nacin 12 years ago.
20448.9.diff (2.0 KB) - added by nacin 12 years ago.
20448.10.diff (1.3 KB) - added by nacin 12 years ago.
20448.11.diff (1.8 KB) - added by nacin 12 years ago.
20448.12.diff (1.6 KB) - added by koopersmith 12 years ago.
20448.custom-background.diff (1.3 KB) - added by duck_ 12 years ago.
20448.13.diff (632 bytes) - added by nacin 12 years ago.

Download all attachments as: .zip

Change History (97)

#1 @nacin
12 years ago

In [20470]:

Flexible heights for Twenty Eleven's custom headers. see #20448.

#2 @nacin
12 years ago

In [20471]:

Twenty Eleven: Remove $locale.php file support. see #20448.

@nacin
12 years ago

#3 @nacin
12 years ago

  • Keywords has-patch added
  • Owner set to koopersmith
  • Status changed from new to reviewing

#4 @nacin
12 years ago

In [20473]:

Twenty Ten: Remove $locale.php file support. see #20448.

#5 @nacin
12 years ago

In [20474]:

Flexible heights for Twenty Ten's custom headers. see #20448.

#6 @kovshenin
12 years ago

  • Cc kovshenin@… added

#7 follow-up: @ocean90
12 years ago

  • Component changed from Themes to Bundled Theme
  • Summary changed from Update Twenty Eleven to use 3.4 features to Update Twenty (Ten|Eleven) to use 3.4 features

#8 @Mamaduka
12 years ago

  • Cc georgemamadashvili@… added

#9 @wdfee
12 years ago

  • Cc wdfee added

#10 @nacin
12 years ago

  • Priority changed from normal to high
  • Severity changed from normal to major
  • Summary changed from Update Twenty (Ten|Eleven) to use 3.4 features to Update Twenty Ten and Twenty Eleven to use 3.4 features

twentyeleven-customize.diff:ticket:19910 from Otto42 adds postMessage support for Twenty Eleven.

#11 @koopersmith
12 years ago

In [20649]:

Theme Customizer: Pass the WP_Customize instance to all actions fired inside the class. Plugins/themes should not refer to the $wp_customize global. see #19910, #20448.

@Otto42
12 years ago

Patch for twentyeleven to support postMessage updating of title, description, and header text-color

#12 @Otto42
12 years ago

20448.twentyeleven.diff is an updated version of the previous patch (in #19910) to take into account [20649] and to add a somewhat smarter way of doing the JS includes for the postMessage handling.

@Otto42
12 years ago

Add color scheme and partially working link-color options to customizer

#13 follow-up: @Otto42
12 years ago

New patch adds color scheme and link color choices to the customizer.

The link color stuff is partially broken because twentyeleven stores the link color in the database with the # in front of it, while the customizer's color control expects it to not have that. I wrote a function to add the # to make the previewer work, but the control appears wonky when you first load the page.

Uploading the patch anyway in case somebody can see a better solution than myself. I think we'll probably need to make the color control capable of handling the data value with or without the # on the color to get the widest use out of it.

#14 in reply to: ↑ 13 @nacin
12 years ago

Replying to Otto42:

New patch adds color scheme and link color choices to the customizer.

I take it you might have missed 20448.diff :-) Looks good. Obviously far more comprehensive than my initial attempt. I ran into the same issue with #, and I think koopersmith said he was going to work on that.

@Otto42
12 years ago

Add layouts too

#15 follow-up: @Otto42
12 years ago

Yeah, I did miss that. :)

Patch 3 adds layouts using much the same method nacin's patch used, but with the addition of calling twentyeleven_layouts() to get the list of layouts instead of hardcoding them.

#16 @kovshenin
12 years ago

Just a note that header_textcolor is also the checkbox that should show/hide the header text. Hidden when "blank" or empty string. Also see #20600

Last edited 12 years ago by kovshenin (previous) (diff)

#17 @nacin
12 years ago

  • Version set to 3.4

#18 @andyadams
12 years ago

  • Cc aadams@… added

#19 follow-up: @andyadams
12 years ago

I've hit a bug where the default "full page refresh" preview of an option doesn't work unless I already have values saved in the DB for the theme.

So, to replicate:

  • Delete the twentyeleven_theme_options value from your database.
  • Open the customizer for TwentyEleven and toggle the dark/light color scheme, and it won't update.
  • Click "Save" on the customizer pane to save the current options.
  • Try toggling the color scheme again, and it will work.

It looks like the customizer is expecting a value to be in the DB? This doesn't have to do with saving defaults in the database, does it?!?

#20 @Otto42
12 years ago

Hahahahaha! #blamenacin

#21 @Otto42
12 years ago

andyadams: Patch that should correct this problem is now in #19910. However, I'm not sure that it's the final version of the patch that will be used. But I can confirm that the problem exists.

#22 @chipbennett
12 years ago

  • Cc chip@… added

#23 in reply to: ↑ 15 @kobenland
12 years ago

Replying to Otto42:

Patch 3 adds layouts using much the same method nacin's patch used, but with the addition of calling twentyeleven_layouts() to get the list of layouts instead of hardcoding them.

Would it make sense to move the JavaScript into its own file, and enqueue it via the 'wp_enqueue_scripts' hook? Works well for me.

#24 follow-up: @nacin
12 years ago

  • Description modified (diff)

#15858 — we also should add Text Domain and Domain Path headers until a subsequent phase of i18n ideally renders those unnecessary.

#25 in reply to: ↑ 19 @kovshenin
12 years ago

Replying to andyadams:

It looks like the customizer is expecting a value to be in the DB? This doesn't have to do with saving defaults in the database, does it?!?

It seems that Customize is using a pre_option_* filter to short-circuit string options, but the option_* filter when it comes to arrays. Since the array is never stored in the database, the option_* filters are never reached because get_option will return the default without passing it through the filter. By simply replacing return $default with $value = $default in get_option I'm able to get the override of non-existing options to work, but I have a feeling this can break something else.

#26 follow-up: @Otto42
12 years ago

kovshenin: See #19910 for the patch I made to fix that issue.

#27 in reply to: ↑ 26 @kovshenin
12 years ago

Otto42: Aye, that's brilliant! Sorry I missed that before writing my comment above. Thanks!

#29 @ryan
12 years ago

Per bug scrub, let's add a default option filter to all of our option functions and add unit tests.

@ryan
12 years ago

default option filters

@ryan
12 years ago

unit tests for default filters

#30 @ryan
12 years ago

In [20783]:

Add filters for the default to get_option() and get_site_option(). Provide default overrides in the customizer. Props Otto42. see #20448

#32 @Otto42
12 years ago

Note on [20783]: The placement of the filter means that the _preview_filter function will run twice when the option exists, once on the default_option value, and once on the real option value. This may lead to unintended consequences in the future, although I don't think it breaks anything right away.

#33 @ryan
12 years ago

In [20784]:

Call filters for default option values only as needed to reduce number of filter calls. Props Ott042. see #20448

@lancewillett
12 years ago

Update for Twenty Eleven Theme Customizer integration

#34 @lancewillett
12 years ago

20448.twentyeleven.4.diff, for Twenty Eleven:

  • Moves Customizer integration code to theme options PHP file, and minor WP code standards cleanup (spaces, alignment)
  • Adds comment blocks to new functions
  • Moves JS to external file

Note from testing: the header_textcolor postMessage callback to change the text color via .css() doesn't work after a custom color is set.

It only works the first time you override the default value. I think it's something with the !important set in functions.php for the header color (see http://core.trac.wordpress.org/browser/trunk/wp-content/themes/twentyeleven/functions.php#L221).

#35 @WraithKenny
12 years ago

  • Cc Ken@… added

#36 @koopersmith
12 years ago

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

In [20916]:

Twenty Eleven theme customizer integration. props lancewillett, Otto42. fixes #20448, see #19910.

#37 @ocean90
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Already pinged koop, but re-opened so it doesn't get lost.

Issue with the link color: http://cl.ly/Gtg6 (notice the two #)

#38 @koopersmith
12 years ago

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

In [20936]:

Theme Customizer: Ensure that JS color controls always use real color values, even if the server-side value is a hex value without a hash. fixes #20448, see #19910.

Adds WP_Customize_Setting->sanitize_js_callback and 'customize_sanitize_js_$settingID' filter, to filter values before they're passed to JS using WP_Customize_Setting->js_value().

Adds support for regular hex colors to the color picker.

Changes color methods:

  • sanitize_hex_color() accepts 3 and 6 digit hex colors (with hashes) and the empty string.
  • sanitize_hex_color_no_hash() accepts 3 and 6 digit hex colors (without hashes) and the empty string.
  • maybe_hash_hex_color() ensures that a hex color has a hash, and otherwise leaves the value untouched.

@ocean90
12 years ago

#4 @koopersmith
12 years ago

In [20938]:

Theme Customizer: Color picker RTL fixes. props ocean90, see #20448.

#5 in reply to: ↑ 24 @SergeyBiryukov
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to nacin:

#15858 — we also should add Text Domain and Domain Path headers until a subsequent phase of i18n ideally renders those unnecessary.

20448.textdomain.diff

#6 @pavelevap
12 years ago

  • Cc pavelevap@… added

Also theme tags strings should be removed from .pot files (and GlotPress), for example: http://translate.wordpress.org/projects/wp/dev/twentyeleven/cs/default?filters[status]=either&filters[original_id]=19426&filters[translation_id]=1115234

Or should I create another ticket?

@nacin
12 years ago

#7 follow-up: @nacin
12 years ago

In [20945]:

WP_Theme: If no 'Domain Path' header is specified, default to looking in the /languages directory. see #20103, see #20448.

#5 @nacin
12 years ago

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

In [20946]:

Provide a Text Domain header for Twenty Ten and Twenty Eleven. WP_Theme will then look to translate the style.css headers (name, author, description, etc.) on themes.php, even if the theme isn't activated. fixes #20448.

#6 @nacin
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Twenty Eleven and Twenty Ten should additionally specify a default-color argument when calling add_custom_background().

Since the behavior of default colors and images for backgrounds are not well supported (UI-wise) prior to 3.3, we should not bother to maintain compatibility (see #20768) by defining the associated constants.

@kobenland
12 years ago

Specify default-color argument when calling add_custom_background() in Twenty Ten and Twenty Eleven. :)

@nacin
12 years ago

#7 follow-up: @nacin
12 years ago

20448.5.diff handles the default colors for both Twenty Ten and Twenty Eleven, including both of Twenty Eleven's color schemes.

It also changes the default custom background callback to only operate on saved values, rather than default values. This prevents an unsaved default value from overriding a manually modified style.css file. body.custom-background should only be reflected when there's actually a custom background in play.

With regards to 20448.4.diff, #fff is the CSS reset background color, not what we specify later in each stylesheet.

#8 in reply to: ↑ 7 @kobenland
12 years ago

Replying to nacin:

With regards to 20448.4.diff, #fff is the CSS reset background color, not what we specify later in each stylesheet.

Right, I thought it was more about having the argument visible in functions.php, which is why I tried to use what I considered the least obtrusive color. Didn't even think of the dark color scheme :(

@ryan
12 years ago

With conditional from beyond

#9 @ryan
12 years ago

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

In [20973]:

Custom background fixes:

  • Specify default background colors for the bundled themes.
  • Change the default custom background callback to only operate on saved values, rather than default values.
  • Prevent an unsaved default value from overriding a manually modified style.css file.

Props nacin, kobenland
fixes #20448

#10 @nacin
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

koopersmith and I agreed in IRC to avoid the conditional from beyond. A patch just never made it here.

It should be a simple if ( get_theme_mod( 'background_image' ) || get_theme_mod( 'background_color' ) )

But we should double-check the IRC logs to see if we missed something in our thought process.

#11 @ryan
12 years ago

Currently themes that don't hard code the default image in css don't get the default background image applied like they did prior to 20973. If they switch to hard coding, they lose the ability to "Remove Background Image".

#12 @ryan
12 years ago

20448-remove-bg-image.2.diff will allow removing the background in the current version of Adventure Journal, which has switched to hard coding the default background image in the CSS. It will not fix displaying the default image for the 3.3 era AJ, which did not hard code.

@ryan
12 years ago

Combined

#13 follow-up: @ryan
12 years ago

Combining the patches gets us closer. It works for default and no bg image whether the bg image is in css or not.

#14 @ryan
12 years ago

If a theme does this:

background: #693808 url( "images/mp-background-tile.jpg" );

Then if the user removes both the background and the color via the custom header admin screen, the default bg image will still show.

#15 follow-up: @ryan
12 years ago

If the theme registers a default background color, a "Default" link shows next to the color picker. If the color has been emptied, this link no longer shows. The show_clear logic needs to be cleaned up for when a default color is registered but the current color is empty.

#16 @mfields
12 years ago

  • Cc michael@… added

#17 @mfields
12 years ago

Just tested 20448.7.diff and it works beautifully!

@nacin
12 years ago

#19 follow-up: @nacin
12 years ago

Okay, let's bury the idea that, if un-saved defaults are in place, there should be no <style> block printed. This hurts themes that used BACKGROUND_IMAGE without specifying a fallback in the stylesheet, and they did that because otherwise removal wouldn't work.

20448.8.diff reverts wp-includes for [20973] and then makes some adjustments:

  • <style> will appear if there is a default image registered. This is the same as 3.3.
  • If only a default color is registered, it still assumes it is in the stylesheet, and no <style> will appear. This is a change from 3.3.
  • <style> will continue to appear as before if there is a custom background color or image. This is the same as 3.3.
  • This then allows for a default background image with background-image: none, overriding style.css. This is new.

The goals of [20973] were to make default-image and default-color not suck, thus encouraging their use, since they're basically new in 3.4.* But at the same time we don't want to encourage people to stop putting these things in CSS as well. We simply want them to register it with WordPress so we know.

(*) I consider them new because:

  • They are now first-class arguments, rather than second-class constants. People will notice them.
  • The constants did not work well, as they could not be removed, were not part of the preview, and were not represented in the UI.
  • The default themes are now leveraging them.

This does not fix comment 15.

#20 @ryan
12 years ago

In [21001]:

Fix removing the default background image for themes that hard-code the default in css. Honor the default background image for themes that do not provide a fallback in css.

  • <style> will appear if there is a default image registered. This is the same as 3.3.
  • If only a default color is registered, it still assumes it is in the stylesheet, and no <style> will appear. This is a change from 3.3.
  • <style> will continue to appear as before if there is a custom background color or image. This is the same as 3.3.
  • This then allows for a default background image with background-image: none, overriding style.css. This is new.

Props nacin
see #20448

@nacin
12 years ago

@nacin
12 years ago

#21 @nacin
12 years ago

20448.10.diff addresses this:

If the theme registers a default background color, a "Default" link shows next to the color picker. If the color has been emptied, this link no longer shows. The show_clear logic needs to be cleaned up for when a default color is registered but the current color is empty.

Purely JS, as the field cannot be blank in PHP (the default would get filled in).

#22 @nacin
12 years ago

In [21002]:

Add some comments to _custom_background_cb() to explain the logic. see #20448.

#23 @ryan
12 years ago

20448.10.diff. Tested with old and new Adventure Journal.

#24 follow-up: @nacin
12 years ago

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

In [21004]:

Custom background color: Show the 'Default' link when the input is empty and there is a default color registered. fixes #20448. see #20734.

#25 @nacin
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

We missed the customizer in this. Patch attached. Using a setting may not have been the most appropriate way to pass a value to the JS, but it does the job.

@nacin
12 years ago

#26 follow-up: @koopersmith
12 years ago

Patch passes the value more cleanly.

#27 @nacin
12 years ago

20448.12.diff works. Good for commit.

#28 @ryan
12 years ago

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

In [21013]:

Don't resurrect a removed default image when changing colors in the Customizer. Props koopersmith, nacin. fixes #20448

#29 @duck_
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

There are still some issues with body.custom-background since get_background_color() will always return true for themes with a registered default. So, the check should be:

if ( get_theme_mod( 'background_color' ) || get_theme_mod( 'background_image' ) || get_theme_support( 'custom-background', 'default-image' ) ) ...

Problem reported by Frumph: http://lists.automattic.com/pipermail/wp-testers/2012-June/014487.html

There also seem to be some issues when looking at _custom_background_cb():

  • $background includes the default-image, so the default-image check on line 1112 is redundant
  • $background includes the default-image, so the "there is not a $background, but there is a default" branch will never be reached

It seems like switching $background back to just the saved image would fix this:

$background = get_theme_mod( 'background_image' );

#30 @nacin
12 years ago

The logic for custom backgrounds is correct.

  • If a custom background image is set, get_background_image() will return the theme mod.
  • If a custom background image is not set and there is no default value, get_background_image() will evaluate to false.
  • If a custom background image is set but empty, this means the image has been "removed", and the empty value would not be overridden by a default. This means get_background_image() (and $background) would evaluate to false.
  • In the case of a set-but-empty image ("removed"), where there is a default image, we print out background-image: none.
  • Thus, the custom-background body class shows up whenever there is either a background image, or a default image.
  • The report from Frumph suggests that all customizations were removed, but to remove all customizations, you need to "Reset" rather than "Remove."

Custom colors do have one bug, and I think it was just a transposition issue:

  • If there is no custom color saved, then get_background_color() will return the default. However, this situation should not output background-color: (default) in the CSS, as this can override a modified stylesheet. (So can images, but that is less of a concern.)
  • It does not output background-color, but it *does* output the custom-background body class. This is incorrect.
  • Wrong: get_background_color() || get_theme_mod( 'background_image' ). Correct: get_theme_mod( 'background_color' ) || get_background_image().
  • Since we subsequently also check for get_theme_support( 'custom-background', 'default-image' ), there is no functional difference between get_theme_mod('background_image') or get_background_image() here. But you can now see why it was a transposition issue — the wrong one was moved to be a theme_mod check.

@nacin
12 years ago

#31 @nacin
12 years ago

20448.13.diff corrects the logic for adding the custom-background body class.

#32 @ryan
12 years ago

Looks good. Tested with Twenty Eleven, Twenty Ten, and old and new Adventure Journal.

#34 @nacin
12 years ago

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

In [21054]:

Do not specify background-image: none when a user removes a custom background
on a theme that has a default background image.

The onus is on the theme to omit the default background-image from style.css,
to allow the user to remove the default background image. Or, the theme can
specify a background-image for the body selector, as long as they then zero
it out for body.custom-background, like so:

body {
	background-image: url( ... );
}
body.custom-background {
	background-image: none;
}

This allows the theme to be compatible with the custom background feature
but also gracefully degrade if the background feature is disabled.

This is the same behavior as 3.3; setting a default image has simply been
made more prominent in 3.4. Reverts [21013], also parts of [21001].
see #20448 for change and discussion history.

see #20132, which will now be marked as invalid.

Also, per previous changes in #20448, the custom-background class should not
be shown when only a default color is in use.

fixes #20448.

#35 @kobenland
12 years ago

With the changes above, the default background-color does not get applied after initial activation of a Theme. The user has to navigate to the custom-backround admin screen, to explicitly save the (proposed) color, so it gets picked up by get_theme_mod( 'background_color' ).

Are Themes supposed to set a background color in style.css on top of specifying 'default-color' when adding theme support?

Thanks,
Konstantin

Note: See TracTickets for help on using tickets.