Twenty Fifteen: hidden header text control doesn't update with color scheme

[cainm]

Steps to recreate in the Customizer:

1. Hide header text with 'Display Header Text' option in the 'Site title & tagline' section
2. In the 'Colors' section, change the color scheme to one with a different header text color (i.e. Default > Pink)
3. Unhide header text - the color control for Header Text Color will still be the old scheme's

The issue has to do with the way core updates the header_textcolor to 'blank' when display_header_text is set: https://core.trac.wordpress.org/browser/trunk/src/wp-admin/js/customize-controls.js#L1274

[downstairsdev]

I'm testing this in Chrome and it appears to be working correctly.

.site-title a is #333 in the default color scheme
.site-title a is #fff in the "pink" color scheme

1) I start with "default" color scheme, .site-title a is #333
2) I unchecked "Display Header Text", text disappears
3) I switched to "Pink" color scheme
4) I checked "Display Head Text", text reappears, .site-title a is #fff

Also appears to work with saves in between.

Is there a chance this was fixed in a different patch?

What browser are you using @cainm?

[japh]

I'm also seeing this issue. Checked in Chrome and Safari on MacBook Pro running Yosemite.

[cainm]

I just verified that the issue exists in Chrome and Safari, but the issue doesn't seem to occur in Firefox on a MacBook Pro running Yosemite.

[cainm]

On my local dev, with SCRIPT_DEBUG set, I couldn't reproduce the issue in Chrome, Firefox, or Safari. @japh, can you double-check?

[cainm]

Nevermind. It seems unrelated to SCRIPT_DEBUG. I have no idea what's going on, clearly. Sometimes, I'm running into the issue, sometimes I'm not.

[iamtakashi]

To reproduce the issue I've seen is.

1. Pick a color scheme. Let's say Dark.
2. Change header text color to red.
3. Save.
4. Hide the header text.
5. Save.
6. Pick a different color scheme. Let say Yellow.
7. Save.
8. Display the header text. (It's still red but that's fine and should be.) but...
9. Click default for the header text color control.

This gives you the default color of the last saved color scheme (Dark). You'd expect to get the default color of the currently selected scheme.

[downstairsdev]

Ok, odd. Following @iamtakashi steps:

I was able to reproduce this in Safari OSX 7.1.
I was able to reproduce this in FF 32.0.3
I was unable to reproduce this in Chrome 38.0.2125.104.

But now that I can reproduce I'll take a look at the code. Thanks!

[cainm]

Update header_textcolor defaults even when control is hidden

[cainm]

29980.diff updates the default values of the header_textcolor control even when it's hidden by display_header_text, but I still think this ticket needs more testing for other scenarios where the values don't behave as expected.

[downstairsdev]

@cainm Tested your patch in Safari and appears to resolve the issue.

I'm wondering if we should get rid of the "if ( 'blank' !== wp.customize( 'header_textcolor' ).get() ) {" conditional altogether though.

If you've selected "white" as the title color at some point, hid the title, switched the palette back to default at a later point and then showed the title, wouldn't you want it to match the color scheme? All the other colors update to match the scheme when it changes regardless of whether a custom color was set- I'm not sure this one should be different just because it was hidden at the time of the switch.

Edit: Easier said than done. Looks like without that conditional anytime the color scheme is changed the header will display, even if it is supposed to be hidden. Haven't exactly been able to trace down why that happens.

[cainm]

Replying to downstairsdev:

Edit: Easier said than done. Looks like without that conditional anytime the color scheme is changed the header will display, even if it is supposed to be hidden. Haven't exactly been able to trace down why that happens.

Yeah, I found out the same thing when working on this originally. Core treats display_header_text as an additional control on the header_textcolor setting - not on a separate setting - and as thus, when the Header Text is hidden,header_textcolor is set to 'blank', and then the 'blank' value is used in most Custom Header text color functionality throughout core and themes (including display_header_text()).

Skipping the conditional in this file overwrites the 'blank' value, and tricks display_header_text().

[cainm]

Replying to downstairsdev:

If you've selected "white" as the title color at some point, hid the title, switched the palette back to default at a later point and then showed the title, wouldn't you want it to match the color scheme? All the other colors update to match the scheme when it changes regardless of whether a custom color was set- I'm not sure this one should be different just because it was hidden at the time of the switch.

This is the use-case I originally created the ticket for, not the default color setting - I was just having a hard time articulating it.

[iamtakashi]

Sorry guys, it looks like I ended up kinda hijacked this thread with a different issue. I can confirm 29980.diff fixes the issue I mentioned.

[iamtakashi]

Now I finally get what the original issue was.

If you wanted to have a red header text, you kinda want to keep the color even you change color scheme?

Now with 29980.diff, you can get the default color of the header text back that matches with the new color scheme you picked. So that's even better.

[cainm]

Replying to iamtakashi:

If you wanted to have a red header text, you kinda want to keep the color even you change color scheme?

Not quite. So the general UX (take the show/hide of the Header Text out of the equation) is that if you select a bunch of custom colors for the different controls and then change color scheme, all of those controls rewrite to the new color scheme.

In the case of header_textcolor, if you select a custom color for the Header Text color (let's say red), and then you hide the text and change scheme, the Header Text color should be updated to the new scheme color, but instead when you unhide, it's still red.

[iamtakashi]

Replying to cainm:

Not quite. So the general UX (take the show/hide of the Header Text out of the equation) is that if you select a bunch of custom colors for the different controls and then change color scheme, all of those controls rewrite to the new color scheme.

In the case of header_textcolor, if you select a custom color for the Header Text color (let's say red), and then you hide the text and change scheme, the Header Text color should be updated to the new scheme color, but instead when you unhide, it's still red.

OK, I see the inconsistency there. Let's keep work on it.

[cainm]

Update header_textcolor defaults and adjust header_textcolor if changed while hidden

[cainm]

29980.2.diff builds on top of 29980.diff with a flag ('changedWhileHidden') and listener on the display_header_text control that updates header_textcolor if the color scheme is changed while header_textcolor is hidden.

I think that there is probably a better way to do this, but I'm having a hard time figuring it out.

[downstairsdev]

@cainm You got closer to figuring it out than I did.

I did see an issue with this patch:

1. Start with default color scheme and title visible
2. Hide title text
3. Switch color scheme
4. Show title
5. Attempt to hide title text again (it fails to hide)

I made a small fix to resolve that issue in 29980.3.diff.

I agree this doesn't feel particularly elegant. I tried and approach using data attributes rather than passing "changedWhileHidden" around, but that seemed even worse.

[kwight]

29980.3.diff works for me (OS X 10.9.5, latest Chrome).

[cainm]

Replying to downstairsdev:

I made a small fix to resolve that issue in 29980.3.diff.

Good catch. Works for me.

[cainm]

Both the Custom Head and Custom Background control switching broke with [30014], so this patch no longer works.

[cainm]

Related: #30125

[downstairsdev]

I think ideally these should be two separate controls. The checkbox "display_header_text" (which saves boolean) and the colorpicker "header_textcolor" (which stores the hex value).

[iamtakashi]

We'll need to wait #30125 first and let's figure out how to tackle this.

[iamtakashi]

If we are merging the Header and Sidebar Text color controls as suggested in #30164, I think we don't need to worry about this anymore.

[iandstewart]

In 30230:

Twenty Fifteen: Simplify the header, sidebar, background controls and make customization faster for users to do. This has the added benefit of fixing our bug where hidden header text wasn't being updated on color scheme switch. Nice.

Props celloexpressions, iamtakashi, fixes #30164 and #29980.