WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#38993 closed defect (bug) (fixed)

Twenty Seventeen: Clearing header text color doesn't update color in preview

Reported by: ocean90 Owned by: davidakennedy
Milestone: 4.7 Priority: low
Severity: normal Version: 4.7
Component: Bundled Theme Keywords: has-patch needs-testing commit dev-reviewed
Focuses: Cc:

Description

Steps to reproduce:

  • Clear existing theme mods (wp eval "remove_theme_mods();")
  • Go to the customizer and the colors section
  • Choose a color
  • Click "Save & Publish"
  • Reload customizer (CMD/CTRL+R)
  • Go to the customizer and the colors section
  • Click the "Clear" button
  • Bug: The preview still shows the previous color

Attachments (3)

38993-clear-color.gif (569.7 KB) - added by ocean90 10 months ago.
38993.diff (802 bytes) - added by sstoqnov 10 months ago.
38993.2.patch (2.0 KB) - added by davidakennedy 10 months ago.
Check if the text color has been removed and use default colors in theme stylesheet.

Download all attachments as: .zip

Change History (15)

This ticket was mentioned in Slack in #core by helen. View the logs.


10 months ago

@sstoqnov
10 months ago

#2 @sstoqnov
10 months ago

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

As @ocean90 mentioned in Slack discussion, the color doesn't remove because the styles are still in header.
Since we are not able to remove that styles I made a check if there is a color, and if not revert to original colors.
I am not sure that this is the best way to add the text color, when there is custom header image.

And I think that there is another problem with color picker.

  • Go to the customizer and the colors section
  • Choose a color
  • Click "Save & Publish"
  • Reload customizer (CMD/CTRL+R)
  • Go to the customizer and the colors section
  • Click the "Clear" button
  • Refresh the page without saving
  • The text colors are back to default

@davidakennedy
10 months ago

Check if the text color has been removed and use default colors in theme stylesheet.

#3 @davidakennedy
10 months ago

In 38993.2.patch, I bounce off the good work from @sstoqnov but do it a bit differently. I just remove the styles from the <head> since the theme stylesheet can take care of things. This avoids the extra logic in the Customizer preview JS.

@laurelfulford Want to test this one out and provide feedback?

#4 @laurelfulford
10 months ago

This looks good to me, @davidakennedy!

I tested it with a variety of combinations - with and without both video and header image (and with both); on subpages and on the front page, and with the dark and custom colour schemes. The header colour consistently returned to the default, whatever that was based on the other settings (white, dark grey, or custom).

#5 @davidakennedy
10 months ago

  • Keywords commit added

This ticket was mentioned in Slack in #core by davidakennedy. View the logs.


10 months ago

#7 follow-up: @westonruter
10 months ago

@davidakennedy

  • What about the situation when the header text color is added? This patch removes the #twentyseventeen-custom-header-styles stylesheet entirely. When the header text is added shouldn't the style element be restored?
  • Likewise, when is_customize_preview(), the twentyseventeen_header_style() should output an empty style element when short-circuiting, even when there are no styles. This is so that the the style can be restored.
  • The JS logic assumes that default-text-color is going to be an empty string. What if a child theme sets the default color?

#8 in reply to: ↑ 7 @davidakennedy
10 months ago

Replying to westonruter:

Good questions @westonruter!

  • What about the situation when the header text color is added? This patch removes the #twentyseventeen-custom-header-styles stylesheet entirely. When the header text is added shouldn't the style element be restored?

That's covered here:

$( '.site-branding, .site-branding a, .site-description, .site-description a' ).css({
	color: to
});

This code only comes into play if to is empty. It's easier to remove the styles than to figure out what the default text color should be, override it, etc. – the color is different based on whether a header image or video is in place.

  • Likewise, when is_customize_preview(), the twentyseventeen_header_style() should output an empty style element when short-circuiting, even when there are no styles. This is so that the the style can be restored.

The styles are added in the other parts of the code... this use case only covers the preview when you press the "Clear" button.

  • The JS logic assumes that default-text-color is going to be an empty string. What if a child theme sets the default color?

If Twenty Seventeen or a child theme where to add a default text color, everything should work fine – unless I'm missing something. I tested this by adding a default text color to Twenty Seventeen. The added logic never is run because the button now applies the "Default" color instead of clearing it.

If I'm missing something, let me know.

#9 @westonruter
10 months ago

  • Keywords dev-feedback added

@davidakennedy thanks for sitting down to explain it with me. It looks good to me.

+1 needs dev-reviewed

#10 @helen
10 months ago

  • Keywords dev-reviewed added; dev-feedback removed

#11 @davidakennedy
10 months ago

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

In 39447:

Twenty Seventeen: Ensure header text color updates in Customizer preview when cleared

Checks if the text color has been cleared and removes the style block if it has been. Uses color styles in theme stylesheet.

Props sstoqnov, davidakennedy.

Fixes #38993.

#12 @helen
10 months ago

In 39448:

Twenty Seventeen: Ensure header text color updates in Customizer preview when cleared

Checks if the text color has been cleared and removes the style block if it has been. Uses color styles in theme stylesheet.

Props sstoqnov, davidakennedy.

Fixes #38993 for the 4.7 branch.

Note: See TracTickets for help on using tickets.