Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#29980 closed defect (bug) (fixed)

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

Reported by: cainm's profile cainm Owned by: iandstewart's profile iandstewart
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.1
Component: Bundled Theme Keywords: needs-testing
Focuses: Cc:

Description

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

Attachments (3)

29980.diff (1.2 KB) - added by cainm 9 years ago.
Update header_textcolor defaults even when control is hidden
29980.2.diff (2.2 KB) - added by cainm 9 years ago.
Update header_textcolor defaults and adjust header_textcolor if changed while hidden
29980.3.diff (2.2 KB) - added by downstairsdev 9 years ago.

Download all attachments as: .zip

Change History (37)

#1 @cainm
9 years ago

  • Keywords needs-patch added

#2 @iandstewart
9 years ago

  • Milestone changed from Awaiting Review to 4.1

This ticket was mentioned in IRC in #wordpress-dev by iandstewart. View the logs.


9 years ago

#4 follow-up: @downstairsdev
9 years ago

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?

#5 @japh
9 years ago

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

#6 in reply to: ↑ 4 @cainm
9 years ago

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

Version 0, edited 9 years ago by cainm (next)

#7 @cainm
9 years ago

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

#8 @cainm
9 years ago

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.

#9 @iamtakashi
9 years ago

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.

#10 @downstairsdev
9 years ago

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
9 years ago

Update header_textcolor defaults even when control is hidden

#11 @cainm
9 years ago

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.

#12 follow-ups: @downstairsdev
9 years ago

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

Last edited 9 years ago by downstairsdev (previous) (diff)

#13 in reply to: ↑ 12 @cainm
9 years ago

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

#14 in reply to: ↑ 12 @cainm
9 years ago

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.

#15 @iamtakashi
9 years ago

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.

#16 follow-up: @iamtakashi
9 years ago

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.

#17 in reply to: ↑ 16 ; follow-up: @cainm
9 years ago

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.

#18 in reply to: ↑ 17 @iamtakashi
9 years ago

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
9 years ago

Update header_textcolor defaults and adjust header_textcolor if changed while hidden

#19 @cainm
9 years ago

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.

#20 follow-up: @downstairsdev
9 years ago

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

#21 @iandstewart
9 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#22 @kwight
9 years ago

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

Last edited 9 years ago by kwight (previous) (diff)

#23 in reply to: ↑ 20 @cainm
9 years ago

Replying to downstairsdev:

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

Good catch. Works for me.

#24 @cainm
9 years ago

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

#25 @cainm
9 years ago

Related: #30125

#26 @downstairsdev
9 years ago

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

This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.


9 years ago

#28 @iandstewart
9 years ago

  • Keywords needs-patch added; has-patch removed

#29 @iamtakashi
9 years ago

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

This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.


9 years ago

#31 @iamtakashi
9 years ago

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.

#32 @iamtakashi
9 years ago

  • Keywords needs-patch removed

This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.


9 years ago

#34 @iandstewart
9 years ago

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

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.

Note: See TracTickets for help on using tickets.