Appearance -> Header UI: "Display Text" Needs Distinct theme_mod

[chipbennett]

The current UI on the Appearance -> Header admin screen is misleading with respect to the "Display Text" form field. This field does not have a distinct theme_mod, but rather correlates to HEADER_TEXTCOLOR. From ​`wp-admin\custom-header.php`:

<td>
        <p>
	<?php $hidetext = get_theme_mod( 'header_textcolor', HEADER_TEXTCOLOR ); ?>
        <label><input type="radio" value="1" name="hidetext" id="hidetext"<?php checked( ( 'blank' == $hidetext || empty( $hidetext ) )  ? true : false ); ?> /> <?php _e( 'No' ); ?></label>
        <label><input type="radio" value="0" name="hidetext" id="showtext"<?php checked( ( 'blank' == $hidetext || empty( $hidetext ) ) ? false : true ); ?> /> <?php _e( 'Yes' ); ?></label>
        </p>
</td>

The issue is with this line:

<?php $hidetext = get_theme_mod( 'header_textcolor', HEADER_TEXTCOLOR ); ?>

Conditionally displaying text based on the theme_mod for header_textcolor being defined or not is unintuitive. There is no practical reason that the theme_mod to display/not display header text should be tied to the theme_mod specifying the header text color. Yet, currently, in order to support the "Display Text" field, header text output must be wrapped in a if ( 'blank' != get_header_textcolor() ) conditional.

I think the simplest solution would be to create a distinct theme_mod for display_text, with a corresponding display_header_text() conditional.

(See also ticket #17605)

[chipbennett]

I'll be happy to try to write a patch for this, but it has some deep fingers in wp-admin\custom-header.php. If the idea has some consensus, I'll try to tackle it.

[scribu]

I agree that using a 'blank' color to disable the header text is hackish.

But maybe we can just create the display_header_text() conditional that hides the hack.

[chipbennett]

That would at least be a good first step, if nothing else!

Should I just create the conditional, so that it available to Themes, or should it be propagated throughout the Admin where applicable?

[scribu]

It should be used wherever it makes sense in the admin too.

[scribu]

But more importantly, it should be used in Twentyeleven.

[chipbennett]

Okay, I'll generate a patch for the core files, then one for Twenty Eleven. Is it okay if I do iterative patches, and get feedback for each step? This will be the most I've modified any core files, so I want to be sure I do it correctly.

[scribu]

It's ok to iterate on patches.

[chipbennett]

Add conditional function display_header_text() to themes.php

[chipbennett]

Okay, here's my first iteration: adding the conditional function.

[scribu]

The function description is too verbose. I would replace it with just

Conditional check to determine if the user has enabled the Display Text option.

The function implementation is also too verbose. It should be a single line:

return 'blank' != get_header_textcolor();

[chipbennett]

Less verbose phpDoc and functional code, as per Scribu's recommendation

[chipbennett]

Replying to scribu:

The function description is too verbose. I would replace it with just

Conditional check to determine if the user has enabled the Display Text option.

The function implementation is also too verbose. It should be a single line:

return 'blank' != get_header_textcolor();

Made less-verbose, as per your suggestion.

[chipbennett]

Propagate use of display_header_text() in \wp-admin\custom-header.php

[chipbennett]

And here's the patch for \wp-admin\custom-header.php, using the new display_header_text() conditional function.

[chipbennett]

Just a note that, while doing some testing of the new code, I'm still noticing an issue that results from the conflation of header_textcolor and display_header_text. If "Display Text" is set to false, and then changed to true, the header_textcolor displays as #blank, which not only is semantically incorrect (the value is expected to be a valid, six-digit HEX color value), but also poor UX (the previous header_textcolor apparently isn't retained when "Display Text" is set to false).

So, even after putting an intuitive display_header_text() around the header_textcolor hack, we've still not solved all of the non-intuitive UX issues.

[scribu]

I can't reproduce that on current trunk.

[chipbennett]

Replying to scribu:

I can't reproduce that on current trunk.

While trying to reproduce the observation, I discovered something else: If "display header text" is set to false, and then the "Restore Original Header Text" button is clicked, the original header text settings are restored, but the UI does not change (javascript issue?). That is: the Text Color text field remains hidden, until the page is refershed.

Steps to reproduce:

1) Ensure that "display header text" is set to false
2) Select "restore original header text"

Is this related enough to keep in this ticket, or should I submit a different ticket?

[scribu]

When I click the "Restore Original header text" button, a page refresh ensues and everything works as expected.

There doesn't seem to be any JS involved. Tested on Twentyeleven.

[chipbennett]

Interesting.

In the most recent nightly, it appears that the "Display Header Text" radio buttons have been removed entirely. Now, the "Text Color" text field label reads, If you want to hide header text, add #blank as text color.

So, I guess that works, too. (Though, I don't think the instruction to use #blank either semantic or intuitive.)

[chipbennett]

Oops; disregard previous comment. Newly mapped domain name, and forgot to enable javascript for the domain in Chrome. :/

[chipbennett]

@scribu

Okay, after a bit of testing with Twenty Eleven, I think I understand why you can't recreate the issues I'm describing: Twenty Eleven defines NO_HEADER_TEXT as true`'.

The wonky issues thus only appear for Themes that define NO_HEADER_TEXT as false - that is, that display the Header Text color field by default. Since Twenty Eleven doesn't display this field by default, the issue doesn't appear. But for Themes that do display this field, it appears not to be displayed when clicking "Restore Original Header Text".

[scribu]

Could you give an example of such a theme that I can install from the comfort of my wp-admin? (I'm lazy :P)

[chipbennett]

I would say to install Oenology, but all of my header-text fixes are in the soon-to-be-released Version 2.5; so it's not quite yet available in the repo. Maybe I can finish it up this weekend and get it submitted, so you can one-click install it. :)

Otherwise, ​you can get the development version from GitHub.

[chipbennett]

Scribu, is there anything still holding up this one?

[scribu]

You should make a single patch that combines themes.php.display_header_text.rev2.diff and custom-header.php.display_header_text.diff

Also, {1-2 words}.{ticket number}.diff is a suficiently descriptive patch name.

[scribu]

Related: #20249

[nacin]

In [20240]:

Introduce display_header_text(). props chipbennett for initial patch. see #18887.

[nacin]

In [20241]:

Use display_header_text() in custom-header.php. Rework the UI to be a 'Show header text' checkbox (rather than radio buttons). Remove lame 'blank' hack for no-JS -- checkboxes work without JS. Move 'Select a Color' to a link like it is for custom background. Nearby code cleanup. see #18887.

[nacin]

get_header_textcolor() is always going to need to return 'blank' for compatibility reasons. I tweaked it so there is now a new function, display_header_text(), and I leveraged that function in core.

However, the 'blank' checks are still in Twenty Eleven. I don't see a reason to go from:

if ( 'blank' == $text_color ) :

To something like:

if ( ( function_exists( 'display_header_text' ) && display_header_text() ) || 'blank' == $text_color ) :

[nacin]

In [20243]:

Properly intercept the main form being submitted when checking for the display-header-text checkbox. see #18887.

[nacin]

Closing as fixed on the new API. Not sure what we can do about the theme mod, but that can be for another ticket in a future release.

[kovshenin]

Related #20600

[nacin]

In [20772]:

display_header_text() should only return true if the theme mod value is not 'blank'. An empty string is valid -- there is no requirement for it to be truthy. see #18887. see #20600.