WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.36k stars 4.13k forks source link

Global styles: Default font sizes are added #52200

Closed hanneslsm closed 6 months ago

hanneslsm commented 1 year ago

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

```javascript { "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

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 commented 1 year ago

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

hanneslsm commented 1 year ago

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

dabowman commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 7 months ago

Since https://github.com/WordPress/gutenberg/pull/58456 this is now broken again!

justintadlock commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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

bgardner commented 7 months ago

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

scruffian commented 7 months ago

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

annezazu commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

@justintadlock This issue has not existed (at least what I am experiencing as noted at https://github.com/WordPress/gutenberg/issues/57889#issuecomment-1917882277) 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 commented 7 months ago

@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 commented 7 months ago

@justintadlock I understand. The original issue that was created (https://github.com/WordPress/gutenberg/issues/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 commented 7 months ago

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 commented 7 months ago

Circling back to ack that I Zoomed with @annezazu and @ajlende to walk through what I reported in https://github.com/WordPress/gutenberg/issues/57889. More to come.

annezazu commented 7 months ago

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

https://github.com/WordPress/gutenberg/assets/26996883/21009556-4da3-4f07-83c0-b5142e7fabeb

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

bgardner commented 7 months ago

Props @ajlende for this WIP: https://github.com/WordPress/gutenberg/pull/58951

iamtakashi commented 7 months ago

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 commented 6 months ago

This should still be open as the fix was reverted

youknowriad commented 6 months ago

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

scruffian commented 6 months ago

@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 https://github.com/WordPress/gutenberg/pull/58409 is seeking to address.

ajlende commented 6 months ago

I think it's okay to close this now.

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

I still have the PR for adding the defaultFontSizes option in https://github.com/WordPress/gutenberg/pull/58409 which I would still appreciate some reviews on 🙂 But I will be closing this issue now.