WordPress / twentytwenty

Twenty Twenty is included in Core as of WordPress 5.3 🎉 To report any issues, please go here: https://core.trac.wordpress.org/newticket
Other
301 stars 140 forks source link

Color: Re selecting the default options does not reset the secondary color #932

Closed carolinan closed 4 years ago

carolinan commented 5 years ago

If I customize the primary color, and then switch back to the default option in the customizer, the secondary color does not reset back to 6d6d6d.

If I customize the background color, and then reset it to default, the secondary color does not reset back to 6d6d6d.

(-If I customize both the background and the primary color, and then reset the primary, it works correctly because I expect the secondary color to still work with the background.)

ianbelanger79 commented 5 years ago

I see the issue @carolinan, but I can't figure out how to fix it before RC3. It may have to wait until after 5.3 is released.

joyously commented 5 years ago

This same thing was in Twenty Nineteen, so look at the code there.

abrightclearweb commented 5 years ago

I've noticed the same issue as @carolinan:

If I customize the primary color, and then switch back to the default option in the customizer, the secondary color does not reset back to 6d6d6d.

But when I try this, the secondary colour does not reset for me:

(-If I customize both the background and the primary color, and then reset the primary, it works correctly because I expect the secondary color to still work with the background.)

The secondary colour stays the same after the switch of the primary colour back to default e.g. #4a7c73, rather than #6d6d6d.

aristath commented 5 years ago

I spent the last few hours working on this one, and it's not an easy fix. The problem here is that this is not really an issue. The default colors are barely compliant, and the script tries to find colors that comply as best as possible. What I tried to do is this:

That has an issue because for some reason the customizer doesn't update the preview pane with the new value (probably because it's a control-less setting)

Next I tried using the theme_mod_* filter in PHP to change the value:

/**
 * Filters the "accent_accessible_colors" theme-mod and returns the defaults when needed.
 *
 * @since 1.0.0
 * @param array $value The theme-mod value.
 * @return array
 */
function twentytwenty_accent_accessible_colors_fallback( $value ) {
    $accent_hue_active = get_theme_mod( 'accent_hue_active', 'default' );
    $background_color  = sanitize_hex_color_no_hash( get_theme_mod( 'background_color' ) );
    $value             = (array) $value;

    if ( 'default' === $accent_hue_active && 'f5efe0' === $background_color ) {
        $value['content'] = array(
            'text'      => '#000000',
            'accent'    => '#cd2653',
            'secondary' => '#6d6d6d',
            'borders'   => '#dcd7ca',
        );
    }
    return $value;
}
add_filter( 'theme_mod_accent_accessible_colors', 'twentytwenty_accent_accessible_colors_fallback' );

That works, but it would also mean that we'd have to refresh the preview pane to apply the changes, and add some listeners in JS to trigger a refresh in the preview frame when the background-color and accent_hue_active have the values we want.

Overall I made 3 alternative implementations and none of them works the way they should. I managed to fix this by combining multiple methods, but the end result was ugly code and it felt too hacky. Honestly I'm not even sure this one should be fixed. I mean the issue here is that the auto-calculated colors are not the same as the original colors. So.... why change the calculated values to comply with the arbitrary hardcoded colors instead of changing the arbitrarily selected colors to be more like the auto-calc'd ones?

joedolson commented 5 years ago

I'm happy with changing the hard-coded colors to match the auto-calculated colors if those colors are accessible. I agree that the problem, at root, is that the defaults should follow the same rules that the calculation does - but both should result in an accessible result.

aristath commented 5 years ago

There was an issue with the secondary color contrast, #977 I believe fixes that. So if that gets merged we'll be one step closer to accessible colors 👍