Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#23722 closed defect (bug) (fixed)

Twenty Thirteen: toggling Display Header Text in Theme Customizer not working

Reported by: rachelbaker's profile rachelbaker Owned by: lancewillett's profile lancewillett
Milestone: 3.6 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords:
Focuses: Cc:

Description

Selecting or deselecting the "Display Header Text" checkbox does not result in expected behavior of hiding or displaying the title and description in the theme's header.

The theme customizer control for this setting appears to be for setting a color for the title and description text, no displaying or hiding the text.

Attachments (3)

23722.patch (771 bytes) - added by SergeyBiryukov 11 years ago.
23722.1.diff (1.5 KB) - added by obenland 11 years ago.
Accounts for header image not being set and for cases where header text is hidden to begin with
23733.empty-header-color.png (159.7 KB) - added by SergeyBiryukov 11 years ago.

Download all attachments as: .zip

Change History (15)

#1 @lancewillett
11 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 3.6
  • Severity changed from minor to normal
  • Summary changed from Twenty Thirteen: Theme Customizer for Site Title & Description is inconsistent to Twenty Thirteen: toggling Display Header Text in Theme Customizer not working

#2 @lancewillett
11 years ago

Thanks for the report, good eye. Looks like we missed this.

#3 @SergeyBiryukov
11 years ago

  • Keywords has-patch added; needs-patch removed

Introduced in [23493].

@obenland
11 years ago

Accounts for header image not being set and for cases where header text is hidden to begin with

#4 @lancewillett
11 years ago

  • Keywords commit added

.1 patch looks good in testing.

#5 @lancewillett
11 years ago

  • Owner set to lancewillett
  • Resolution set to fixed
  • Status changed from new to closed

In 23670:

Twenty Thirteen: fix toggling header text display in Customizer. Props obenland, fixes #23722.

#6 follow-up: @SergeyBiryukov
11 years ago

In style.css, min-height is set for .site-header hgroup:
http://core.trac.wordpress.org/browser/trunk/wp-content/themes/twentythirteen/style.css?rev=23680#L802

[23670] sets it for #masthead hgroup. Shouldn't the selector be the same for consistency?

#7 @SergeyBiryukov
11 years ago

  • Keywords has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Found another issue:

  1. Go to Appearance > Header, disable header text.
  2. Go to Customizer, enable header text.
  3. Go to Appearance > Header again. You'll see the header with a wrong color (see the screenshot).

Code inspection shows that get_header_textcolor() returns an empty value.

#8 follow-up: @SergeyBiryukov
11 years ago

Twenty Twelve also has an empty color in the style attribute (as in the screenshot above), but it still shows the correct color due to the declaration in twentytwelve_admin_header_style():
http://core.trac.wordpress.org/browser/tags/3.5.1/wp-content/themes/twentytwelve/inc/custom-header.php#L109

Twenty Eleven doesn't display the header on Appearance > Header screen at all after those steps, so there might be an underlying core issue.

#9 in reply to: ↑ 8 @SergeyBiryukov
11 years ago

Replying to SergeyBiryukov:

Twenty Eleven doesn't display the header on Appearance > Header screen at all after those steps

It specifically checks that get_header_textcolor() is not empty:
http://core.trac.wordpress.org/browser/tags/3.5.1/wp-content/themes/twentyeleven/functions.php#L313

#10 follow-up: @kovshenin
11 years ago

I can reproduce this broken color issue with both Twenty Eleven and Twenty Thirteen and I think it's not related to the theme at all. The trick here is that both fields (text-color and display-header-text) are actually one single setting. In wp-admin/custom-header.php, we check the value for a color, and if a color is not found, we set it to blank, so setting an empty text-color results in blank, which is fine, but do note that we never set the header_textcolor mod to an empty string.

In the Customizer, on the other hand, both sanitize_hex_color_no_hash and sanitize_hex_color are allowed to return an empty string, which is then passed on to the header_textcolor theme mod.

I feel that the broken part is in wp-admin/js/customize-controls.js when juggling the two controls -- the setting isn't reset to its default color when the checkbox is checked and the value is blank. I can't think of a good way to fix this yet.

#11 in reply to: ↑ 6 @obenland
11 years ago

Replying to SergeyBiryukov:

In style.css, min-height is set for .site-header hgroup:
[23670] sets it for #masthead hgroup. Shouldn't the selector be the same for consistency?

I chose to use #masthead in the jQuery selector because I thought it might be faster to look for an ID rather than a class. In the end it will always be an inline style added to the hgroup element, so changing the jQuery selector will have no effect on the priority of the added CSS rule.

#12 in reply to: ↑ 10 @SergeyBiryukov
11 years ago

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

Replying to kovshenin:

I can reproduce this broken color issue with both Twenty Eleven and Twenty Thirteen and I think it's not related to the theme at all.

Thanks for debugging, I've created a new ticket for this issue: #23761.

Note: See TracTickets for help on using tickets.