Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global styles: Default font sizes are added #52200

Closed
hanneslsm opened this issue Jul 1, 2023 · 34 comments · Fixed by #56661 or #58951
Closed

Global styles: Default font sizes are added #52200

hanneslsm opened this issue Jul 1, 2023 · 34 comments · Fixed by #56661 or #58951
Assignees
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@hanneslsm
Copy link

Description

I created a blank theme and added custom font sizes.
This works intended in the editor, but in the Global styles, default font sizes are added in the drop down (S, M, L, XL)

Step-by-step reproduction instructions

  1. Copy the nearly blank theme.js from below
  2. Go into Global Style → Typography → Text
  3. See in the drop down the additional "Small, Medium, Large, Extra Large" font sizes, that shouldn't be there.
theme.js

{
  "settings": {
    "appearanceTools": true,
    "color": {
      "background": true,
      "custom": true,
      "customDuotone": true,
      "customGradient": true,
      "defaultDuotone": false,
      "defaultGradients": false,
      "defaultPalette": false,
      "duotone": [],
      "gradients": [],
      "palette": [],
      "text": true
    },
    "layout": {
      "contentSize": "620px",
      "wideSize": "1000px"
    },
    "shadow": {
      "defaultPresets": true,
      "presets": []
    },
    "spacing": {
      "customSpacingSize": true,
      "spacingScale": {
        "increment": 1.5,
        "mediumStep": 1.5,
        "operator": "*",
        "steps": 7,
        "unit": "rem"
      },
      "spacingSizes": [],
      "units": ["%", "px", "em", "rem", "vh", "vw"]
    },
    "typography": {
      "customFontSize": true,
      "dropCap": true,
      "fluid": true,
      "fontFamilies": [
        {
          "fontFamily": "-apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen-Sans, Ubuntu, Cantarell, 'Helvetica Neue', sans-serif",
          "name": "System Font",
          "slug": "system-font"
        },
        {
          "fontFace": [
            {
              "fontFamily": "Barlow",
              "fontStyle": "italic",
              "fontWeight": "700",
              "src": ["file:./assets/fonts/barlow_italic_700.ttf"]
            },
            {
              "fontFamily": "Barlow",
              "fontStyle": "normal",
              "fontWeight": "700",
              "src": ["file:./assets/fonts/barlow_normal_700.ttf"]
            },
            {
              "fontFamily": "Barlow",
              "fontStyle": "italic",
              "fontWeight": "600",
              "src": ["file:./assets/fonts/barlow_italic_600.ttf"]
            },
            {
              "fontFamily": "Barlow",
              "fontStyle": "normal",
              "fontWeight": "600",
              "src": ["file:./assets/fonts/barlow_normal_600.ttf"]
            },
            {
              "fontFamily": "Barlow",
              "fontStyle": "italic",
              "fontWeight": "400",
              "src": ["file:./assets/fonts/barlow_italic_400.ttf"]
            },
            {
              "fontFamily": "Barlow",
              "fontStyle": "normal",
              "fontWeight": "400",
              "src": ["file:./assets/fonts/barlow_normal_400.ttf"]
            }
          ],
          "fontFamily": "Barlow",
          "slug": "barlow"
        }
      ],
      "fontSizes": [
        {
          "name": "Display xl",
          "slug": "Display-xl",
          "size": "96px",
          "fluid": {
            "max": "96px",
            "min": "52px"
          }
        },
        {
          "name": "Display lg",
          "slug": "Display-lg",
          "size": "72px",
          "fluid": {
            "max": "72px",
            "min": "46px"
          }
        },
        {
          "name": "Display md",
          "slug": "Display-md",
          "size": "60px",
          "fluid": {
            "max": "60px",
            "min": "42px"
          }
        },
        {
          "name": "Display sm",
          "slug": "Display-sm",
          "size": "52px",
          "fluid": {
            "max": "52px",
            "min": "40px"
          }
        },
        {
          "name": "Heading 1",
          "slug": "Heading-1",
          "size": "48px",
          "fluid": {
            "max": "48px",
            "min": "36px"
          }
        },
        {
          "name": "Heading 2",
          "slug": "Heading-2",
          "size": "36px",
          "fluid": {
            "max": "36px",
            "min": "30px"
          }
        },
        {
          "name": "Heading 3",
          "slug": "Heading-3",
          "size": "28px",
          "fluid": {
            "max": "28px",
            "min": "26px"
          }
        },
        {
          "name": "Heading 4",
          "slug": "Heading-4",
          "size": "24px",
          "fluid": {
            "max": "24px",
            "min": "22px"
          }
        },
        {
          "name": "Heading 5",
          "slug": "Heading-5",
          "size": "20px",
          "fluid": {
            "max": "20px",
            "min": "19px"
          }
        },
        {
          "name": "Heading 6",
          "slug": "Heading-6",
          "size": "18px",
          "fluid": {
            "max": "18px",
            "min": "17px"
          }
        },
        {
          "name": "Paragraph lg",
          "slug": "paragraph-lg",
          "size": "18px",
          "fluid": {
            "max": "18px",
            "min": "17px"
          }
        },
        {
          "name": "Paragraph base",
          "slug": "paragraph-base",
          "size": "16px",
          "fluid": {
            "max": "16px",
            "min": "16px"
          }
        },
        {
          "name": "Paragraph sm",
          "slug": "paragraph-sm",
          "size": "14px",
          "fluid": {
            "max": "14px",
            "min": "14px"
          }
        },
        {
          "name": "Info md",
          "slug": "Info-md",
          "size": "12px",
          "fluid": {
            "max": "12px",
            "min": "12px"
          }
        },
        {
          "name": "Info sm",
          "slug": "Info-sm",
          "size": "10px",
          "fluid": {
            "max": "10px",
            "min": "10px"
          }
        }
      ],
      "fontStyle": true,
      "fontWeight": true,
      "letterSpacing": true,
      "textDecoration": true,
      "textTransform": true
    },
    "useRootPaddingAwareAlignments": true
  },
  "styles": {
    "spacing": {
      "padding": {
        "bottom": "0px",
        "left": "16px",
        "right": "16px",
        "top": "0px"
      }
    }
  },
  "templateParts": [
    {
      "area": "header",
      "name": "header"
    },
    {
      "area": "footer",
      "name": "footer"
    }
  ],
  "version": 2,
  "$schema": "https://schemas.wp.org/trunk/theme.json"
}

Screenshots, screen recording, code snippet

Block styles (as it should be) Global styles (with additonal font sizes)
image image

Environment info

  • WP 6.2.2
  • GB 16.1.0

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@carolinan
Copy link
Contributor

Hi
I copied the code example from the issue and used it to replace the theme.json in Twenty Twenty-Three.

  • 6.2.2 only: No issue
  • 6.2.2 and 16.1.0: Additional font sizes as described
  • 6.3-beta2-56121: Additional font sizes as described

@hanneslsm
Copy link
Author

hanneslsm commented Sep 17, 2023

@annezazu Can you please add this issue to the Polish board? Cheers!

@dabowman
Copy link

Just bumping. I indeed noticed this as well. I'm seeing the default sizes appearing in the editor alongside custom ones as well as in the DOM as variables. I can't actually find a way to remove them. I don't think this happened in the past so I'm not sure what changed. I'm running 6.4.1 and the Gutenberg 17.0.2

@dabowman
Copy link

Also noting that this can cause a known issue where two type sizes can conflict with eachother. If two type sizes have the same value then selecting one will cause the block to revert to the first matching style that appears in the list. Normally this isn't a big issue, unless the default styles don't disappear when you define custom ones. The default styles appear first in the list so selecting a custom style with the same value forces it to select the default.

@ajlende
Copy link
Contributor

ajlende commented Nov 27, 2023

I'm seeing the same settings in both places (WP 6.4 + GB trunk); however, the four default font sizes ('Small', 'Medium', 'Large', 'Extra Large') appear first in the block styles, but last in the global styles. In other places where Gutenberg provides default values for something we add a default* option to disable showing those settings, but I don't see one for font sizes right now.

So, it sounds to me like there are two parts to solving this fully.

  1. Add a defaultFontSizes option that allows for the default size options to be hidden.
  2. Ensure that the order of the default, theme, and user sizes are the same in both places when they are all enabled.

@scruffian
Copy link
Contributor

Since #58456 this is now broken again!

@justintadlock
Copy link
Contributor

This should work as a quick fix for disabling the default font sizes, but I'm pretty sure the CSS for them won't be printed either:

add_filter('wp_theme_json_data_default', function($theme_json) {
	$data = [
		'version'  => 2,
		'settings' => [
			'typography' => [
				'fontSizes' => []
			]
		]
	];

	return $theme_json->update_with($data);
});

@bgardner
Copy link

@justintadlock Can confirm the filter above is working perfectly and as expected. CSS is printed and Small, Medium, Large, and Extra Large sizes show correct values in the Typography > Font Size menu in post editor as well as correctly on the front end when chosen.

@carolinan
Copy link
Contributor

We can't expect theme developers to add this filter as a long term solution.

@bgardner
Copy link

@carolinan Agreed. I was merely confirming it worked to see if it helped inform a permanent fix in core.

@scruffian
Copy link
Contributor

I have removed this from the Polish board is it's too big to go into 6.5.

@annezazu
Copy link
Contributor

annezazu commented Feb 7, 2024

@scruffian I'm a bit confused by this last comment. Can you clarify? I've added it to the 6.5 board to be safe as it's clearly impacting theme authors and seems important to get fixed. Ideally, this is resolved for 6.5 but I definitely want to understand more if/why that might not be feasible.

@scruffian
Copy link
Contributor

We put in a fix for this, but it ended up creating worse bugs, so we took it out again. We are still working on a fix, but it's going to take longer than we have before the cut off. I hope it will make it into a Gutenberg release soon :)

@widoz
Copy link
Contributor

widoz commented Feb 7, 2024

We can't expect theme developers to add this filter as a long term solution.

I agree with @carolinan on this, it is inconvenient to ask to touch two places to configure the theme but at the same time I think it is nice to give the opportunity to confiugure the theme dynamically.

I've seen some options line blockGap accepting three values, false, true, null triggering different results. Could be the same for the collections, something like false deactivate the feature, []|{} empty object add nothing more. Indeed we cannot pass an empty collection to signaling the theme we want any option because it would make difficult to address the cases where someone might want to edit the value via a filter giving dynamic data which might be empty or not while at the same time they want to keep the current theme / core configuration.

Update

Sorry, I've not took into account the theme override the core configuration, disabling via the same option wouldn't work because there'll be no way to add more options. (edited)

@ajlende
Copy link
Contributor

ajlende commented Feb 7, 2024

I have removed this from the Polish board is it's too big to go into 6.5.

I'm a bit confused by this last comment. Can you clarify? I've added it to the 6.5 board to be safe as it's clearly impacting theme authors and seems important to get fixed. Ideally, this is resolved for 6.5 but I definitely want to understand more if/why that might not be feasible.

We put in a fix for this, but it ended up creating worse bugs, so we took it out again. We are still working on a fix, but it's going to take longer than we have before the cut off. I hope it will make it into a Gutenberg release soon :)

@annezazu To clarify some more, we want to fix this issue by adding a defaultFontSizes option in theme.json which matches how many other presets work. And we thought we had it working in #56661.

Unfortunately, the behavior of having that setting either enabled or disabled is different from the current implementation (merging default options with theme options). This is the "worse bug" that @scruffian mentioned—old themes looked different after the change. To fix that issue, we need to bump the theme.json version so old (theme.json v2) themes can continue to work as they currently do and new (theme.json v3) themes can start using the new defaults.

Increasing a theme.json version isn't a small change. And this is the first time that we're changing a default value, so there's no precedent set for how to do it. We want to get it right because there's two other options that could be updated the same way.

You can see my progress in #58409. The change already seemed big enough that we didn't want to rush it for 6.5.

@annezazu
Copy link
Contributor

annezazu commented Feb 7, 2024

Got it! Thank you for the additional context. A number of theme authors are keen on seeing this resolved so continued update here I know would be appreciated as work continues. I think it's wise to delay from 6.5 considering the bump in theme.json versioning.

@colorful-tones
Copy link
Member

It seems we're keeping this on the WordPress 6.5 Editor Task board intentionally in the No Status column in hopes that it'll be addressed for 6.5. It does seem like a critical issue for many builders. I see it as In Progress on the Automattic team Ignite's project board. @ajlende do you have any updates on this, please?

@annezazu
Copy link
Contributor

This isn't going to make it to 6.5 as stated above :( I left it for a bit to see if we could make progress since, as you noted, it's an important issue but it's not going to happen 24 hours before beta 1. Punting now.

@bgardner
Copy link

bgardner commented Feb 12, 2024

@annezazu Are you saying that this specific method of trying to fix the existing issue with font sizes isn't going into 6.5, or that the issue altogether won't be addressed/fixed in 6.5?

@justintadlock
Copy link
Contributor

Are you saying that this specific method of trying to fix the existing issue with font sizes isn't going into 6.5, or that the issue altogether won't be addressed/fixed in 6.5?

This issue has been punted to 6.6.

@bgardner
Copy link

bgardner commented Feb 12, 2024

@justintadlock Are you're saying that font sizes will be broken in the site editor for any theme that uses slugs similar to core?

@justintadlock
Copy link
Contributor

Yes.

This has been an issue for several major releases and no one has a fix for it yet that doesn't break other things. Given the complexity of the issue, it's unlikely to be fixed in time for 6.5. Of course, if any contributor has time for a PR to fix in the next 24 hours, that could change. I'm sure plenty of folks would be willing to test.

@bgardner
Copy link

bgardner commented Feb 12, 2024

@justintadlock This issue has not existed (at least what I am experiencing as noted at #57889 (comment)) until recently. The current version 6.4.3 is not experiencing this issue, and if ignored, this will become a major disaster for users of these themes. I am not sure at this point if anyone understands the severity of it, and I will gladly Zoom with you (or @annezazu) or whomever to show exactly what is happening.

@justintadlock
Copy link
Contributor

@bgardner - This specific ticket was created for Wordpress 6.2.2 and Gutenberg 16.1.0, so I want to make sure it's clear we're talking about punting something that's been understood to be an ongoing thing.

Not sure if #57889 should be reopened at the moment, especially if it's a new issue introduced since WP 6.4. In that case, it should definitely be addressed for 6.5. Anyway, pinging some folks to see what we can get done. :)

@bgardner
Copy link

@justintadlock I understand. The original issue that was created (#57889) was created at 6.4.2, and that is where I chimed in. It has since been closed, but the problem still exists. I think it needs to be re-opened (again) and made a priority for 6.5.

@bgardner
Copy link

bgardner commented Feb 12, 2024

Because this issue touches all of my themes, Ollie by Mike McAlister, even @ndiego's personal theme. Not to mention the (possible) 100's of themes that are using font sizes/slugs similar to core.

@bgardner
Copy link

Circling back to ack that I Zoomed with @annezazu and @ajlende to walk through what I reported in #57889. More to come.

@annezazu
Copy link
Contributor

Thank you all for jumping on! We talked for just under 14 minutes and here's the relevant part of the recording for transparency:

Shorter.version.of.recording.mp4

At this stage, Alex is going to work on getting a revert PR in place and test it against Brian's Powder Theme.

@bgardner
Copy link

Props @ajlende for this WIP: #58951

@iamtakashi
Copy link

I tried to describe the problem we still see in this thread. Sorry, if it wasn't clear.

I can confirm that blocks now appear with the correct font sizes on both the editor and the front of the site, but the sizes with the same slugs as core are labelled incorrectly.

@scruffian
Copy link
Contributor

This should still be open as the fix was reverted

@scruffian scruffian reopened this Mar 11, 2024
@youknowriad
Copy link
Contributor

youknowriad commented Mar 12, 2024

@scruffian I'm not sure the fix was reverted, we reverted a change that created this issue.

@scruffian
Copy link
Contributor

scruffian commented Mar 14, 2024

@scruffian I'm not sure the fix was reverted, we reverted a change that created this issue.

That's true but there is still an issue with default font sizes appearing when they shouldn't, which #58409 is seeking to address.

@ajlende
Copy link
Contributor

ajlende commented Mar 20, 2024

I think it's okay to close this now.

I figured it was still in a broken state because #56661 had fixed it, but was reverted in #58951 causing it to be an issue again. #58409 was intended to reimplement #56661 and solve the issue again. However, #55219 which was the origin of this bug was reverted in #58951 fixing this issue. 😵‍💫

I still have the PR for adding the defaultFontSizes option in #58409 which I would still appreciate some reviews on 🙂 But I will be closing this issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
Status: Done