Twenty Seventeen: Background colour for header

[laurelfulford]

The header background colour should be updated to work with a default white header font.

Discussion background on: ​https://github.com/WordPress/twentyseventeen/issues/482

[celloexpressions]

Since it's the only comment on the GH issue, repeating here:

Won't the custom color (and its white default) still only be applied when there's a header image (or video) set? The current design could probably be preserved for the no-image case if desired.

[davidakennedy]

One idea I had for this, especially now that the header image is global, is we could do a variable for the default text color and change it based on whether a header image exists or not. So no header image, color is #222 and with a header image, it's #fff. That may be confusing from a UX perspective, but it's a possible solution to think through. I think we could make it work with custom colors too.

[celloexpressions]

I missed this in #38426 - we need to make an active callback for the header text color control that returns has_header_image() || has_header_video(). I don't think we need to make any frontend changes here, only change when the option is visible to be based on when it actually applies.

Adding the active callback above (via $wp_customize->get_control()) would be a good first bug if any new contributors are able to give that a try.

[Idealien]

first-bug patch submitted, but it is not yet approve worthy. Addition or removal of image / video has no impact on the control display within colors section in my testing. However, if same callback is used with return is_front_page() it does have expected behavior.

Suggestions to resolve further are appreciated.

[Idealien]

In Slack bug scrub yesterday @ [3:58 PM] @celloexpressions wrote

@davidakennedy: The patch for 38391 looks correct but isn't quite there according to @idealien. I think the has_header_video() case in the conditional needs to have an && is_front_page(), so that its not shown when there's a video but no image and you're on an inner page and it's always shown if you have an image regardless of the video.

I believe there is something else in logic related to header_textcolor control presentation causing a larger issue for this patch.

Scenario 1
If I put the same twentyseventeen_is_header_textcolor_visible function as active_callback on colorscheme control defined earlier in the customer.php it behaves mostly as expected.

Repeatable steps for issue that the control is not hidden after add + remove of image or video without full page reload:

• [correct] Start with No Image or Video and page refresh, colorscheme is hidden
• [correct] Add Video and colorscheme displays
• [incorrect] Remove Video, colorscheme stays
• [correct] Save and force full page refresh and colorscheme is hidden

Scenario 2
Remove the header_textcolor control and re-add it with snippet below. The control never changes show / hide behavior. BUT...When adding the new control rename it as "header_textcolour" (eh!) and it does work identical to Scenario 1.

<?php
        $wp_customize->get_control( 'header_textcolor' )->active_callback = 'twentyseventeen_is_header_textcolor_visible';
        $wp_customize->remove_control( 'header_textcolor' );
        $wp_customize->add_control( new WP_Customize_Color_Control( $wp_customize, 'header_textcolor', array(
                'label'       => __( 'Header Text Color', 'twentyseventeen' ),
                'description' => __( 'Select Color', 'twentyseventeen' ),
                'section'     => 'colors',
                'active_callback' => 'twentyseventeen_is_header_textcolor_visible',
        ) ) );
?>

Some other logic related to presentation of header_textcolor overlapping impact on the callback defined here? Debugging into that depth of JS is out of my realm for this release, but hopefully the details above can point someone else in a better direction towards right solution.

[davidakennedy]

@Idealien Thanks for the detailed feedback here!

I tested your patch quite a bit yesterday, ran into some of the same issues and didn't have time to comment. Something else is causing problems, and I'm not sure what. Technically, has_custom_header() should be able to be used as an active callback function and work, but it does not. And has the same issues. See: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/theme.php#L1395

I'll try to get some more eyes on this. Thanks again for your efforts!

[helen]

I've been a little confused about this ticket and various terminology being thrown around, so to try to be sure I'm understanding, here's what I've got:

• When there is a header image or video, the header text defaults to white.
• When there is not, the header text defaults to black in the light color scheme, white in the dark color scheme, and ??? in custom.
• When removing a header image or video, the header area does not collapse (#38627) and the header text remains white even if the current color scheme is light with the header text color as default (the bug being described in this ticket).
• Bug: the header text color control does not change to reflect the current default color based on scheme.
• The color scheme and header text color controls should always be shown.

[davidakennedy]

Replying to helen:

I've been a little confused about this ticket and various terminology being thrown around, so to try to be sure I'm understanding, here's what I've got:

• When there is a header image or video, the header text defaults to white.

Yes.

• When there is not, the header text defaults to black in the light color scheme, white in the dark color scheme, and ??? in custom.

Yes.

• When removing a header image or video, the header area does not collapse (#38627) and the header text remains white even if the current color scheme is light with the header text color as default (the bug being described in this ticket).

Yes.

• Bug: the header text color control does not change to reflect the current default color based on scheme.

Yes. And it doesn't change to suit the style.css. A theme can change the header (site title and tagline) text color in the CSS to accommodate visual changes, like Twenty Seventeen does with or without a image/video background in the header. But there is always one default text color, which may not always match up to what the user sees.

• The color scheme and header text color controls should always be shown.

Probably, yes. Although one idea here was to only show the controls if a user had a image or video in the background.

This issue in this ticket could have been more clearly explained, and it's tough because there are a few things. So thanks for diving in and getting to the problems succinctly. :) It comes down to the preview not matching the front end at times. And we haven't hit on a good solution for that.

[davidakennedy]

In testing, it looks like the patch in #38627 (https://core.trac.wordpress.org/attachment/ticket/38627/38627.3.diff) also solves this issue. I'll get it committed soon, but I'd also recommend not setting a default text color.

This means, if a user clicks default text color in the Customizer – no matter which header style they have, it falls back to the CSS in style.css. The preview changes with the header media changes… if they have used a default text color, it works everywhere and if they switch back to the default (blank) from a custom color, it uses the theme’s CSS, and the preview matches.

[celloexpressions]

I have only ever seen the existence of the header text color option justified as being necessary to ensure that the text is visible over the user's image. Every user test I've seen where users run into this option, they tend to think it's there to customize the colors of the theme and are disappointed/confused about there not being more options. Twenty Seventeen has a black gradient over the image, which makes it difficult to achieve good contrast over the image with colors other than white anyway.

Can we remove the header text color option for Twenty Seventeen? If the gradient is made slightly darker, one could reasonably argue that white will work over most images, and where it doesn't, custom CSS can be used. The most recent round of user tests on make/design include a user who was concerned about the opacity of the site tagline here and struggled to eventually determine that CSS would be required for that, but the color option didn't help anyway. If we want to keep it, it should only apply or be visible as an option when there's an image; otherwise it falls outside the intended purpose of the option, causes confusion for users, and diverges from the carefully-designed custom color options/patterns in use for the rest of the theme.

The color scheme controls are always shown, at least the last time I checked. Not sure how that's related here?

[davidakennedy]

Replying to celloexpressions:

That's good feedback. Thanks!

Can we remove the header text color option for Twenty Seventeen?

I think it's better to leave it because without it, a user can't modify it based on the type of header image or video they upload.

If we want to keep it, it should only apply or be visible as an option when there's an image; otherwise it falls outside the intended purpose of the option, causes confusion for users, and diverges from the carefully-designed custom color options/patterns in use for the rest of the theme.

That's what was experimented a bit with on this ticket. I don't think there's harm in having it be present all the time to give user's some flexibility.

The color scheme controls are always shown, at least the last time I checked. Not sure how that's related here?

They are – right.

[davidakennedy]

Removes default header text color and unused variable.

[davidakennedy]

In 39220:

Twenty Seventeen: Make custom header preview match front end in Customizer when changed

• Toggles has-header-image body class in Customizer preview whenever images or videos are added or removed.
• Hides the .custom-header-imagediv in CSS when an image or video haven't been set so preview changes are smoother.
• Also fixes the main issues in #38391 – making the preview match.

Props bradyvercher.

Fixes #38627.
See #38391.

[laurelfulford]

Updates CSS selectors in custom-header.php.

[laurelfulford]

38391.2.patch​ looks like it works well, @davidakennedy!

The only issue I spotted is that the CSS selectors in custom-header.php had gotten out of date compared to style.css, so the custom text colour was being applied to the front page, but not the subpages.

38391.3.patch​ tweaks the code a bit to address that.

[karmatosed]

In 39224:

Twenty Seventeen: Fixes custom header text color issues

Removes default header text color and updates and updates selectors.

Props laurelfulford, davidakennedy, Idealien
Fixes #38391

[celloexpressions]

That's what was experimented a bit with on this ticket. I don't think there's harm in having it be present all the time to give user's some flexibility.

Noting that there are usability problems with having a header text color option as something to provide additional color-based customization unrelated to the header image; see ​https://make.wordpress.org/design/2016/11/04/4-7-beta-1-user-testing-in-the-customizer-and-with-twenty-seventeen/ for an example of how this ends up coming across to users (in the first test I believe).