WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#18887 closed enhancement (fixed)

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

Reported by: chipbennett Owned by:
Milestone: 3.4 Priority: normal
Severity: minor Version: 3.2
Component: UI Keywords: has-patch close
Focuses: Cc:

Description

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)

Attachments (3)

themes.php.display_header_text.diff (793 bytes) - added by chipbennett 3 years ago.
Add conditional function display_header_text() to themes.php
themes.php.display_header_text.rev2.diff (549 bytes) - added by chipbennett 3 years ago.
Less verbose phpDoc and functional code, as per Scribu's recommendation
custom-header.php.display_header_text.diff (1.5 KB) - added by chipbennett 3 years ago.
Propagate use of display_header_text() in \wp-admin\custom-header.php

Download all attachments as: .zip

Change History (36)

comment:1 chipbennett3 years ago

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.

comment:2 scribu3 years ago

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.

comment:3 chipbennett3 years ago

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?

comment:4 scribu3 years ago

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

comment:5 scribu3 years ago

But more importantly, it should be used in Twentyeleven.

comment:6 chipbennett3 years ago

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.

comment:7 scribu3 years ago

It's ok to iterate on patches.

chipbennett3 years ago

Add conditional function display_header_text() to themes.php

comment:8 chipbennett3 years ago

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

comment:9 follow-up: scribu3 years ago

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();
Version 0, edited 3 years ago by scribu (next)

chipbennett3 years ago

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

comment:10 in reply to: ↑ 9 chipbennett3 years ago

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.

comment:11 scribu3 years ago

  • Summary changed from Appearnce -> Header UI: "Display Text" Needs Distinct theme_mod to Appearance -> Header UI: "Display Text" Needs Distinct theme_mod

chipbennett3 years ago

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

comment:12 chipbennett3 years ago

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

comment:13 chipbennett3 years ago

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.

comment:14 follow-up: scribu3 years ago

I can't reproduce that on current trunk.

comment:15 in reply to: ↑ 14 chipbennett2 years ago

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?

comment:16 scribu2 years ago

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.

comment:17 chipbennett2 years ago

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.)

comment:18 chipbennett2 years ago

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

comment:19 chipbennett2 years ago

@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".

comment:20 scribu2 years ago

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

comment:21 chipbennett2 years ago

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.

comment:22 chipbennett2 years ago

  • Keywords has-patch needs-testing added

comment:23 chipbennett2 years ago

Scribu, is there anything still holding up this one?

comment:24 scribu2 years ago

  • Severity changed from normal to minor
  • Type changed from defect (bug) to enhancement

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.

comment:25 scribu2 years ago

Related: #20249

comment:26 nacin2 years ago

In [20240]:

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

comment:27 nacin2 years ago

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.

comment:28 nacin2 years ago

  • Keywords close added; needs-testing removed
  • Milestone changed from Awaiting Review to 3.4

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 ) :

comment:29 nacin2 years ago

In [20243]:

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

comment:30 nacin2 years ago

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.

comment:31 nacin2 years ago

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

comment:33 nacin2 years ago

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.

Note: See TracTickets for help on using tickets.