WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#23722 closed defect (bug) (fixed)

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

Reported by: rachelbaker Owned by: 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 2 years ago.
23722.1.diff (1.5 KB) - added by obenland 2 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 2 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 @lancewillett2 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

comment:2 @lancewillett2 years ago

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

@SergeyBiryukov2 years ago

comment:3 @SergeyBiryukov2 years ago

  • Keywords has-patch added; needs-patch removed

Introduced in [23493].

@obenland2 years ago

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

comment:4 @lancewillett2 years ago

  • Keywords commit added

.1 patch looks good in testing.

comment:5 @lancewillett2 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.

comment:6 follow-up: @SergeyBiryukov2 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?

comment:7 @SergeyBiryukov2 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.

comment:8 follow-up: @SergeyBiryukov2 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.

comment:9 in reply to: ↑ 8 @SergeyBiryukov2 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

comment:10 follow-up: @kovshenin2 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.

comment:11 in reply to: ↑ 6 @obenland2 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.

comment:12 in reply to: ↑ 10 @SergeyBiryukov2 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.