search

themeum / kirki

Extending the customizer
https://kirki.org
MIT License
1.26k stars 329 forks source link

Refactor typography control #1661

Open aristath opened 6 years ago

aristath commented 6 years ago

Right now the way typography controls work causes a few issues - primarily with the way fonts are defined for the control. The new implementation will get the googlefonts via an ajax call and be more solid. Work on this has already began on a separate branch

Needs to be done in 3.0.17 to solve issues I had with the last 3 updates where people keep reporting kirkiAllFonts is throwing errors in the console without any apparent reason. I can't stand this absurdity any more, has to change.

wilrevehl commented 6 years ago

Hi, Aristeides. You're the man! We sincerely appreciate all the hard work you continue to pour into Kirki. It is awesome!

Regarding the typography control, something we could use during your refactor is the font-family name for the font-face reflect the variant selected. The problem we're having is when we have multiple typography instances (h1-h6 in our case) and they use the same font but different variants, it still names them all the same. This causes the most important aspects of font-style and font-weight to not be inherited when using the font family variant. To illustrate, here's my output using the Barlow font:

/* latin-ext */
@font-face {
  font-family: 'Barlow';
  font-style: normal;
  font-weight: 100;
  src: local('Barlow Thin'), local('Barlow-Thin'), url(http://fonts.gstatic.com/s/barlow/v1/DnDmJgjpvLUPu5Lyr7RMYhTbgVql8nDJpwnrE27mub0.woff2) format('woff2');
  unicode-range: U+0100-024F, U+1E00-1EFF, U+20A0-20AB, U+20AD-20CF, U+2C60-2C7F, U+A720-A7FF;
}
/* latin-ext */
@font-face {
  font-family: 'Barlow';
  font-style: normal;
  font-weight: 400;
  src: local('Barlow Regular'), local('Barlow-Regular'), url(http://fonts.gstatic.com/s/barlow/v1/SCyUdhaPYx6RJd17dIjx7vesZW2xOQ-xsNqO47m55DA.woff2) format('woff2');
  unicode-range: U+0100-024F, U+1E00-1EFF, U+20A0-20AB, U+20AD-20CF, U+2C60-2C7F, U+A720-A7FF;
}
/* latin-ext */
@font-face {
  font-family: 'Barlow';
  font-style: normal;
  font-weight: 500;
  src: local('Barlow Medium'), local('Barlow-Medium'), url(http://fonts.gstatic.com/s/barlow/v1/ZqlneECqpsd9SXlmAsD2ExJtnKITppOI_IvcXXDNrsc.woff2) format('woff2');
  unicode-range: U+0100-024F, U+1E00-1EFF, U+20A0-20AB, U+20AD-20CF, U+2C60-2C7F, U+A720-A7FF;
}
/* latin */
@font-face {
  font-family: 'Barlow';
  font-style: normal;
  font-weight: 500;
  src: local('Barlow Medium'), local('Barlow-Medium'), url(http://fonts.gstatic.com/s/barlow/v1/kIIp0WfvDOeW2tTGAi2QqVtXRa8TVwTICgirnJhmVJw.woff2) format('woff2');
  unicode-range: U+0000-00FF, U+0131, U+0152-0153, U+02C6, U+02DA, U+02DC, U+2000-206F, U+2074, U+20AC, U+2212, U+2215;
}
/* latin-ext */
@font-face {
  font-family: 'Barlow';
  font-style: normal;
  font-weight: 700;
  src: local('Barlow Bold'), local('Barlow-Bold'), url(http://fonts.gstatic.com/s/barlow/v1/yS165lxqGuDghyUMXeu6xRJtnKITppOI_IvcXXDNrsc.woff2) format('woff2');
  unicode-range: U+0100-024F, U+1E00-1EFF, U+20A0-20AB, U+20AD-20CF, U+2C60-2C7F, U+A720-A7FF;
}

Since these styles are only inherited by font name to the target element, they're all font-family: Barlow and therefore only use the last Barlow and there's no differentiation.

OR maybe I'm doing something wrong. Feel free to poke around the markup if you're curious http://fbc.gomedia.ws/markup-test/ Thanks again!!

aristath commented 6 years ago

Hey there!

We don't actually generate those in Kirki... This is all coming from the google APIs directly!! For example on https://fonts.google.com/?selection.family=Roboto:400,400i,700 you can see I've selected the Roboto font with the "regular", "italic" and "700" variants. The URL from the google API is https://fonts.googleapis.com/css?family=Roboto:400,400i,700 If you open that URL you'll see this inside:

/* cyrillic-ext */
@font-face {
  font-family: 'Roboto';
  font-style: italic;
  font-weight: 400;
  src: local('Roboto Italic'), local('Roboto-Italic'), url(https://fonts.gstatic.com/s/roboto/v18/WxrXJa0C3KdtC7lMafG4dRkAz4rYn47Zy2rvigWQf6w.woff2) format('woff2');
  unicode-range: U+0460-052F, U+20B4, U+2DE0-2DFF, U+A640-A69F;
}
/* cyrillic */
@font-face {
  font-family: 'Roboto';
  font-style: italic;
  font-weight: 400;
  src: local('Roboto Italic'), local('Roboto-Italic'), url(https://fonts.gstatic.com/s/roboto/v18/OpXUqTo0UgQQhGj_SFdLWBkAz4rYn47Zy2rvigWQf6w.woff2) format('woff2');
  unicode-range: U+0400-045F, U+0490-0491, U+04B0-04B1, U+2116;
}
/* greek-ext */
@font-face {
  font-family: 'Roboto';
  font-style: italic;
  font-weight: 400;
  src: local('Roboto Italic'), local('Roboto-Italic'), url(https://fonts.gstatic.com/s/roboto/v18/1hZf02POANh32k2VkgEoUBkAz4rYn47Zy2rvigWQf6w.woff2) format('woff2');
  unicode-range: U+1F00-1FFF;
}
/* greek */
@font-face {
  font-family: 'Roboto';
  font-style: italic;
  font-weight: 400;
  src: local('Roboto Italic'), local('Roboto-Italic'), url(https://fonts.gstatic.com/s/roboto/v18/cDKhRaXnQTOVbaoxwdOr9xkAz4rYn47Zy2rvigWQf6w.woff2) format('woff2');
  unicode-range: U+0370-03FF;
}
/* vietnamese */
@font-face {
  font-family: 'Roboto';
  font-style: italic;
  font-weight: 400;
  src: local('Roboto Italic'), local('Roboto-Italic'), url(https://fonts.gstatic.com/s/roboto/v18/K23cxWVTrIFD6DJsEVi07RkAz4rYn47Zy2rvigWQf6w.woff2) format('woff2');
  unicode-range: U+0102-0103, U+1EA0-1EF9, U+20AB;
}
/* latin-ext */
@font-face {
  font-family: 'Roboto';
  font-style: italic;
  font-weight: 400;
  src: local('Roboto Italic'), local('Roboto-Italic'), url(https://fonts.gstatic.com/s/roboto/v18/vSzulfKSK0LLjjfeaxcREhkAz4rYn47Zy2rvigWQf6w.woff2) format('woff2');
  unicode-range: U+0100-024F, U+1E00-1EFF, U+20A0-20AB, U+20AD-20CF, U+2C60-2C7F, U+A720-A7FF;
}
/* latin */
@font-face {
  font-family: 'Roboto';
  font-style: italic;
  font-weight: 400;
  src: local('Roboto Italic'), local('Roboto-Italic'), url(https://fonts.gstatic.com/s/roboto/v18/vPcynSL0qHq_6dX7lKVByXYhjbSpvc47ee6xR_80Hnw.woff2) format('woff2');
  unicode-range: U+0000-00FF, U+0131, U+0152-0153, U+02C6, U+02DA, U+02DC, U+2000-206F, U+2074, U+20AC, U+2212, U+2215;
}
/* cyrillic-ext */
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 400;
  src: local('Roboto'), local('Roboto-Regular'), url(https://fonts.gstatic.com/s/roboto/v18/sTdaA6j0Psb920Vjv-mrzH-_kf6ByYO6CLYdB4HQE-Y.woff2) format('woff2');
  unicode-range: U+0460-052F, U+20B4, U+2DE0-2DFF, U+A640-A69F;
}
/* cyrillic */
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 400;
  src: local('Roboto'), local('Roboto-Regular'), url(https://fonts.gstatic.com/s/roboto/v18/uYECMKoHcO9x1wdmbyHIm3-_kf6ByYO6CLYdB4HQE-Y.woff2) format('woff2');
  unicode-range: U+0400-045F, U+0490-0491, U+04B0-04B1, U+2116;
}
/* greek-ext */
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 400;
  src: local('Roboto'), local('Roboto-Regular'), url(https://fonts.gstatic.com/s/roboto/v18/tnj4SB6DNbdaQnsM8CFqBX-_kf6ByYO6CLYdB4HQE-Y.woff2) format('woff2');
  unicode-range: U+1F00-1FFF;
}
/* greek */
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 400;
  src: local('Roboto'), local('Roboto-Regular'), url(https://fonts.gstatic.com/s/roboto/v18/_VYFx-s824kXq_Ul2BHqYH-_kf6ByYO6CLYdB4HQE-Y.woff2) format('woff2');
  unicode-range: U+0370-03FF;
}
/* vietnamese */
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 400;
  src: local('Roboto'), local('Roboto-Regular'), url(https://fonts.gstatic.com/s/roboto/v18/NJ4vxlgWwWbEsv18dAhqnn-_kf6ByYO6CLYdB4HQE-Y.woff2) format('woff2');
  unicode-range: U+0102-0103, U+1EA0-1EF9, U+20AB;
}
/* latin-ext */
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 400;
  src: local('Roboto'), local('Roboto-Regular'), url(https://fonts.gstatic.com/s/roboto/v18/Ks_cVxiCiwUWVsFWFA3Bjn-_kf6ByYO6CLYdB4HQE-Y.woff2) format('woff2');
  unicode-range: U+0100-024F, U+1E00-1EFF, U+20A0-20AB, U+20AD-20CF, U+2C60-2C7F, U+A720-A7FF;
}
/* latin */
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 400;
  src: local('Roboto'), local('Roboto-Regular'), url(https://fonts.gstatic.com/s/roboto/v18/oMMgfZMQthOryQo9n22dcuvvDin1pK8aKteLpeZ5c0A.woff2) format('woff2');
  unicode-range: U+0000-00FF, U+0131, U+0152-0153, U+02C6, U+02DA, U+02DC, U+2000-206F, U+2074, U+20AC, U+2212, U+2215;
}
/* cyrillic-ext */
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 700;
  src: local('Roboto Bold'), local('Roboto-Bold'), url(https://fonts.gstatic.com/s/roboto/v18/77FXFjRbGzN4aCrSFhlh3oX0hVgzZQUfRDuZrPvH3D8.woff2) format('woff2');
  unicode-range: U+0460-052F, U+20B4, U+2DE0-2DFF, U+A640-A69F;
}
/* cyrillic */
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 700;
  src: local('Roboto Bold'), local('Roboto-Bold'), url(https://fonts.gstatic.com/s/roboto/v18/isZ-wbCXNKAbnjo6_TwHToX0hVgzZQUfRDuZrPvH3D8.woff2) format('woff2');
  unicode-range: U+0400-045F, U+0490-0491, U+04B0-04B1, U+2116;
}
/* greek-ext */
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 700;
  src: local('Roboto Bold'), local('Roboto-Bold'), url(https://fonts.gstatic.com/s/roboto/v18/UX6i4JxQDm3fVTc1CPuwqoX0hVgzZQUfRDuZrPvH3D8.woff2) format('woff2');
  unicode-range: U+1F00-1FFF;
}
/* greek */
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 700;
  src: local('Roboto Bold'), local('Roboto-Bold'), url(https://fonts.gstatic.com/s/roboto/v18/jSN2CGVDbcVyCnfJfjSdfIX0hVgzZQUfRDuZrPvH3D8.woff2) format('woff2');
  unicode-range: U+0370-03FF;
}
/* vietnamese */
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 700;
  src: local('Roboto Bold'), local('Roboto-Bold'), url(https://fonts.gstatic.com/s/roboto/v18/PwZc-YbIL414wB9rB1IAPYX0hVgzZQUfRDuZrPvH3D8.woff2) format('woff2');
  unicode-range: U+0102-0103, U+1EA0-1EF9, U+20AB;
}
/* latin-ext */
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 700;
  src: local('Roboto Bold'), local('Roboto-Bold'), url(https://fonts.gstatic.com/s/roboto/v18/97uahxiqZRoncBaCEI3aW4X0hVgzZQUfRDuZrPvH3D8.woff2) format('woff2');
  unicode-range: U+0100-024F, U+1E00-1EFF, U+20A0-20AB, U+20AD-20CF, U+2C60-2C7F, U+A720-A7FF;
}
/* latin */
@font-face {
  font-family: 'Roboto';
  font-style: normal;
  font-weight: 700;
  src: local('Roboto Bold'), local('Roboto-Bold'), url(https://fonts.gstatic.com/s/roboto/v18/d-6IYplOFocCacKzxwXSOJBw1xU1rKptJj_0jans920.woff2) format('woff2');
  unicode-range: U+0000-00FF, U+0131, U+0152-0153, U+02C6, U+02DA, U+02DC, U+2000-206F, U+2074, U+20AC, U+2212, U+2215;
}

They may all be the same font-family, but they also have font-style and font-weight defined inside them, so the browser differentiates them and chooses which one to use based on those properties. If for example you've got this:

body {
  font-family: Barlow;
  font-weight: 500;
}

then it will use the one that has font-weight:500, font-style:normal.

But in version 3.0.17 we no longer use those links anyway! We now use the webfontloader https://github.com/typekit/webfontloader which is becoming the recommended method for google fonts and will also allow us to use other webfont sites as well (besides googlefonts)

wilrevehl commented 6 years ago

Hmm okay, maybe the issue I was having is with cache. I'll test further. I swear it was working on earlier versions. Thanks for the insight!

notrealdev commented 6 years ago

I get an error with Kirki 3.0.18 and WP 4.9.1Uncaught ReferenceError: WebFont is not defined

wilrevehl commented 6 years ago

Aris, I tested further and it does seem your latest refactor leaves out the font variant to font-weight conversion. When I look at your process_output for Kirki_Output_Field_Typography previous to v3, you have the needed font-weight and font-style extraction from the variant selection. It seems to be gone now.

Or do we need to change the way we're using it now? We could count on the font weight and font style being applied by your automatic output which was nice and made sense. Let me know if you would like us to help you via GIT to resolve this. Thanks!

tanaykhandelwal commented 6 years ago

Hi There, Pleasant Greetings :-)

Thanks for this great plugin.

Updating the '\kirki\modules\css\field\class-kirki-output-field-typography.php' file with below sorted out the problem on my site.

<?php
/**
 * Handles CSS output for typography fields.
 *
 * @package     Kirki
 * @subpackage  Controls
 * @copyright   Copyright (c) 2017, Aristeides Stathopoulos
 * @license     http://opensource.org/licenses/https://opensource.org/licenses/MIT
 * @since       2.2.0
 */

/**
 * Output overrides.
 */
class Kirki_Output_Field_Typography extends Kirki_Output {

    /**
     * Processes a single item from the `output` array.
     *
     * @access protected
     * @param array $output The `output` item.
     * @param array $value  The field's value.
     */
    protected function process_output( $output, $value ) {

        $output['media_query'] = ( isset( $output['media_query'] ) ) ? $output['media_query'] : 'global';
        $output['element']     = ( isset( $output['element'] ) ) ? $output['element'] : 'body';
        $output['prefix']      = ( isset( $output['prefix'] ) ) ? $output['prefix'] : '';
        $output['suffix']      = ( isset( $output['suffix'] ) ) ? $output['suffix'] : '';

        $value = Kirki_Field_Typography::sanitize( $value );

        $properties = array(
            'font-family',
            'font-size',
            'variant',    // Changed the key from font-weight to variant
            'font-style',
            'letter-spacing',
            'word-spacing',
            'line-height',
            'text-align',
            'text-transform',
            'text-decoration',
            'color',
        );

        foreach ( $properties as $property ) {

            // Early exit if the value is not in the defaults.
            if ( ! isset( $this->field['default'][ $property ] ) ) {
                continue;
            }

            // Early exit if the value is not saved in the values.
            if ( ! isset( $value[ $property ] ) || ! $value[ $property ] ) {
                continue;
            }

            // Early exit if we use "choice" but not for this property.
            if ( isset( $output['choice'] ) && $output['choice'] !== $property ) {
                continue;
            }

            // Take care of variants.
            if ( 'variant' == $property && isset( $value['variant'] ) && ! empty( $value['variant'] ) ) {

                // Get the font_weight.
                $font_weight = str_replace( 'italic', '', $value['variant'] );
                $font_weight = ( in_array( $font_weight, array( '', 'regular' ) ) ) ? '400' : $font_weight;

                // Is this italic?
                $is_italic = ( false !== strpos( $value['variant'], 'italic' ) );
                $this->styles[ $output['media_query'] ][ $output['element'] ]['font-weight'] = $font_weight;
                if ( $is_italic ) {
                    $this->styles[ $output['media_query'] ][ $output['element'] ]['font-style'] = 'italic';
                }
                continue;
            }

            $property_value = $this->process_property_value( $property, $value[ $property ] );
            if ( 'font-family' === $property ) {
                $value['font-backup'] = ( isset( $value['font-backup'] ) ) ? $value['font-backup'] : '';
                $property_value = $this->process_property_value( $property, array(
                    $value['font-family'],
                    $value['font-backup'],
                ) );
            }
            $property = ( isset( $output['choice'] ) && isset( $output['property'] ) ) ? $output['property'] : $property;
            $property_value = ( is_array( $property_value ) && isset( $property_value[0] ) ) ? $property_value[0] : $property_value;
            $this->styles[ $output['media_query'] ][ $output['element'] ][ $property ] = $output['prefix'] . $property_value . $output['suffix'];
        }
    }
}

Thanks!

wilrevehl commented 6 years ago

Thanks, Tanay! That does indeed resolve this issue. Aris, please let us know if you'll be able to revisit this and I'll keep an eye on it. In the meantime, I owe you a donation. :)

aristath commented 6 years ago

@xttn which theme? I can't replicate this...

@wilrevehl Thank you for your donation, I am grateful! Looking at the variants issue now and it doesn't make a lot of sense... Worst part is I can't replicate it!!!

@tanaykhandelwal hmmm... I see you added this part there:

// Take care of variants.
if ( 'variant' == $property && isset( $value['variant'] ) && ! empty( $value['variant'] ) ) {

    // Get the font_weight.
    $font_weight = str_replace( 'italic', '', $value['variant'] );
    $font_weight = ( in_array( $font_weight, array( '', 'regular' ) ) ) ? '400' : $font_weight;

    // Is this italic?
    $is_italic = ( false !== strpos( $value['variant'], 'italic' ) );
    $this->styles[ $output['media_query'] ][ $output['element'] ]['font-weight'] = $font_weight;
    if ( $is_italic ) {
        $this->styles[ $output['media_query'] ][ $output['element'] ]['font-style'] = 'italic';
    }
    continue;
}

That's all good and fine, and in fact it's kind of what happens in the fallback class so I have no objection to add that. However, it still doesn't explain why this happens. Makes no sense to me... In the control's JS when you select the variant, we also calculate and save the values for font-weight & font-style: https://github.com/aristath/kirki/blob/e624890d2af9121ff19ee7cb33e9a099c82a5c2a/controls/js/src/typography.js#L262-L271

Then this goes through the sanitization function which also makes sure these are converted, just in case someone has royally messed-up their site somehow:

https://github.com/aristath/kirki/blob/a7aa6b80cd7d758d2a94a18baefd13e351f275e1/field/class-kirki-field-typography.php#L165-L175

Finally in the output, one of the first things that runs is - again - calling the sanitization:

$value = Kirki_Field_Typography::sanitize( $value );

That makes sure that in case the value was saved with a really old value of kirki, the font-weight & font-style will get calculated (in old versions we were only saving the variant and not the other 2). So in total there are 3 steps that would have to go wrong in order for the font-weights not to be shown. I'll keep exploring and see what I can find... As soon as this one gets fixed I'll release v3.0.19 to fix it.

wilrevehl commented 6 years ago

Ah yeah the sanitize method on variant does the same thing Tanay did to fix it, but at the point of saving. What's weird is that I changed the variant multiple times and resaved when I was verifying the bug and it continued to not add font-weight to the output.

The difference may be these options had already been saved prior to updating Kirki. Also, I still think there's a possibility of cache playing a role, such as maybe an old version of the JS file was still being loaded after the update.

aristath commented 6 years ago

Ah yeah the sanitize method on variant does the same thing Tanay did to fix it, but at the point of saving

That's the part that drives me crazy! Not only does it do it on save, but also right above Tanay's code! So in theory it should be working without the extra code 🤔

aristath commented 6 years ago

I'll check this again first thing in the morning, it's late here so I'll sleep on it and fix it in a few hours. In case I don't find the solution I'll release an update with the code posted above as a workaround until we find the root of the issue.