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: Form elements #29167

Closed
scruffian opened this issue Feb 19, 2021 · 23 comments
Closed

Global Styles: Form elements #29167

scruffian opened this issue Feb 19, 2021 · 23 comments
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json Needs Dev Ready for, and needs developer efforts

Comments

@scruffian
Copy link
Contributor

scruffian commented Feb 19, 2021

What problem does this address?

Most themes want to have a consistent look across all form elements, so it would be good to be able to customize these in one place.

What is your proposed solution?

We can add form elements to the list of supported elements in theme.json, so that all form elements look the same. Another way to achieve this would be to ensure that all blocks use the same inner blocks for form elements.

@scruffian scruffian added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Feature] Full Site Editing Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Feb 19, 2021
@deborah86
Copy link

I would also like to see this.

@colorful-tones
Copy link
Member

This would alleviate the need for a CSS reset in themes in this particular aspect. I would encourage the following entry as part of the CSS:

input,
button,
textarea,
select {
  font: inherit;
}

Otherwise, it is typically browser default to have monospace font.

Note: this is referenced from Andy Bell's modern-css-reset

Currently, on whether a CSS Reset is necessary for WordPress/TwentyTwentyTwo theme, which is related.

@colorful-tones
Copy link
Member

I would be happy to get a PR going for this. If somebody already knows where in the codebase is likely the place to dig in then feel free to drop it here. Otherwise, I’ll get a PR in next day or two.

@colorful-tones
Copy link
Member

colorful-tones commented Oct 25, 2021

I'm doing some initial discovery to try and move this along a bit. I'm thinking that a good start would be to perhaps add the support in theme.json to have Element Styles like the following:

{
    "version": 1,
    "styles": {
        "elements": {
            "input": {
                "typography": {
                    "fontSize": "var(--wp--preset--font-size--normal)"
                }
            },
            "select": {
                "typography": {
                    "fontSize": "var(--wp--preset--font-size--normal)"
                }
            },
            "label": {
                "typography": {
                    "fontSize": "var(--wp--preset--font-size--normal)"
                }
            },
            "button": {
                "typography": {
                    "fontSize": "var(--wp--preset--font-size--normal)"
                }
            },
            "textarea": {
                "typography": {
                    "fontSize": "var(--wp--preset--font-size--normal)"
                }
            },
        }
    }
}

This change alone would require a significant changeset to any core blocks that use these elements, e.g. Post Comments (other?).

I think we'll want to keep things simple though. Thoughts @MaggieCabrera @scruffian @kjellr @jffng ?

@jffng
Copy link
Contributor

jffng commented Oct 25, 2021

I'm thinking that a good start would be to perhaps add the support in theme.json to have Element Styles like the following:

👍

This change alone would require a significant changeset to any core blocks that use these elements, e.g. Post Comments (other?).

To get a sense for the scope of what blocks may be impacted by styling the default form elements:

Element Core Blocks Impacted
input Post Comments, Search
textarea Post Comments
select Archive
label Archive, Search, Post Comments
button Search

This was off the top of my head but anyone feel free to add / edit. I thought the file block did but it looks like the download button uses a link and styles it to look like a button.

@colorful-tones colorful-tones self-assigned this Oct 27, 2021
@colorful-tones
Copy link
Member

I'm going to try and get a PR together for this today.

@colorful-tones
Copy link
Member

Doing some preliminary discovery and wondering if it is advisable to make my entry point for these modifications here: lib/block-support/elements.php 🤔

@jffng
Copy link
Contributor

jffng commented Oct 27, 2021

cc @oandregal what are your thoughts on introducing form elements to global styles at this point? Do you have some guidance on how this might be implemented and what considerations should be made?

@mtias
Copy link
Member

mtias commented Oct 27, 2021

This needs a bit more thought since it's not clear how it'd map to the block API at all, both from a user perspective and a developer perspective. Would the "button" in these form elements affect the "Button" block? Would it affect other buttons created by other block libraries? How would that connection be expressed? Is there any utility to a block author to designate an element in their block as being a form element? The components package for blocks would need to evolve to allow this mapping as well, and that needs further thought.

Fine to begin explorations but we should target them for >5.9.

@colorful-tones
Copy link
Member

colorful-tones commented Oct 27, 2021

This needs a bit more thought since it's not clear how it'd map to the block API at all, both from a user perspective and a developer perspective.

Agree, there is quite a bit of nuance to consider here, and I'm mostly trying to create as small of an introduction of feature set to move it along a bit.

Thanks for the insight @mtias

Would the "button" in these form elements affect the "Button" block?

No, and I believe that is mostly clear by the explicit entry in theme.json for styles.elements.button. We're targeting the <button> element only in this regard.

I think it is confusing and partly detrimental that the Button block outputs pseudo buttons. Or in other words the Button block outputs <a> elements and not actual <button> elements.

Would it affect other buttons created by other block libraries?

For the example of <button>. Yes, I imagine it would affect any <button> that are used by block libraries.

Is there any utility to a block author to designate an element in their block as being a form element?

Yes, I imagine so. HTML usage should be expressive and semantic. If a block author is creating a form then they should ideally use a <form> element.

The components package for blocks would need to evolve to allow this mapping as well, and that needs further thought.

Yep, I imagined so.

Fine to begin explorations but we should target them for >5.9.

👍

@mtias
Copy link
Member

mtias commented Oct 27, 2021

Yes, I imagine so. HTML usage should be expressive and semantic.

The problem is that it's also undifferentiated. You can use a button element to wrap around an icon and not want to inherit generic button styles as defined by styles.elements.button. A block author would be then prompted to over-qualify selectors ending up in a neverending chase with the theme probably covering itself by over-qualifying their CSS selectors directly and bypassing theme.json entirely, which defeats the whole point.

To remedy that, we need to allow block authors to be able to express something like <Button> over a plain <button> as a way to opt-in into the element assignment. The contract is then declarative and not implicit and we can reason about it better, generate the right style pairings for just those elements, etc.

@colorful-tones
Copy link
Member

colorful-tones commented Oct 27, 2021

The problem is that it's also undifferentiated. You can use a button element to wrap around an icon and not want to inherit generic button styles as defined by styles.elements.button.

I have to focus on an example to extrapolate a bit further. Here is how I would add an icon inside of a <button>

<button aria-label="Menu">
    <svg focusable="false" width="24" height="28" viewBox="0 0 24 28">
        <!-- svg content -->
    </svg>
</button>

I do not believe styles.button.fontSize would have an impact on that icon in any negative manner.

To remedy that, we need to allow block authors to be able to express something like over a plain as a way to opt-in into the element assignment. The contract is then declarative and not implicit and we can reason about it better, generate the right style pairings for just those elements, etc.

I believe I'm sort of following you here. I think with my example above that we would need to allow block authors a means to hook in the aria-label="Menu" should an icon be inserted and author want to have no text within the button. Therefore, we would need a designated <Button> component for block authors to work with.

@mtias
Copy link
Member

mtias commented Oct 27, 2021

In the svg example, if the default button look has a background color but the block using the icon button need to present the icon button without a background for some reason, they'd need to preemptively overwrite the background (because they don't know if one would be set) and to do that they might reach out to CSS because there's no ergonomic way to express it in the declaration.

Therefore, we would need a designated component for block authors to work with.

Correct, the expressiveness of components being a bit higher-order than the plain HTML elements allows us to hook more behaviours and assumptions while making it easier for block authors to build things that work with the system by default.

@colorful-tones
Copy link
Member

In the svg example, if the default button look has a background color but the block using the icon button need to present the icon button without a background for some reason, they'd need to preemptively overwrite the background (because they don't know if one would be set) and to do that they might reach out to CSS because there's no ergonomic way to express it in the declaration.

This was why I focused my initial proposal only on font-size modifications. Although, I do see how it could be a slippery slope. I.e. if we add font-size then why not background-color next, and so on.

I think my next logical step would be to introduce customMargin and customPadding, but I do not think I would explore colors without a designated <Button> component.

@colorful-tones
Copy link
Member

colorful-tones commented Oct 28, 2021

Upon further discovery of how to attempt to incorporate my initial proposal it seems like there is far more work than I anticipated.

Originally, I had thought that the theme.json block support entry for style.elements was handled in lib/block-supports/elements.php. However, upon further digging - this only handles style.elements.link.color.text and the style.elements.h1.typography settings are handled in lib/block-supports/typography.php, and are tied to existing blocks and whether their blocks.json has:

	"supports": {
		"typography": {
			"fontSize": true,
			"lineHeight": true,
			"__experimentalFontWeight": true
		},
		"__experimentalSelector": "h1,h2,h3,h4,h5,h6",
	},

So, this implies to me that we would have to create unique blocks for each of the elements (<button>, <input>, <select>, and <textarea>) we would want to target. This would result in new core blocks for <Button>, <Input>, <Select>, and <Textarea>, which I have not foreseen.

I bit off way more than I could chew. 🤦

@colorful-tones
Copy link
Member

To provide a little more background for the uninitiated. The theme.json currently only supports the following within the style.elements entry:

{
	"version": 1,
	"styles": {
		"elements": {
			"h1": {
				"typography": {
					"fontFamily": "var(--wp--preset--font-family--source-serif-pro)",
					"fontWeight": "300",
					"lineHeight": "var(--wp--custom--typography--line-height--tiny)",
					"fontSize": "var(--wp--custom--typography--font-size--colossal)"
				}
			},
			"h2": {
				"typography": {
					"fontFamily": "var(--wp--preset--font-family--source-serif-pro)",
					"fontWeight": "300",
					"lineHeight": "var(--wp--custom--typography--line-height--small)",
					"fontSize": "var(--wp--custom--typography--font-size--gigantic)"
				}
			},
			"h3": {
				"typography": {
					"fontFamily": "var(--wp--preset--font-family--source-serif-pro)",
					"fontWeight": "300",
					"lineHeight": "var(--wp--custom--typography--line-height--tiny)",
					"fontSize": "var(--wp--preset--font-size--huge)"
				}
			},
			"h4": {
				"typography": {
					"fontFamily": "var(--wp--preset--font-family--source-serif-pro)",
					"fontWeight": "300",
					"lineHeight": "var(--wp--custom--typography--line-height--tiny)",
					"fontSize": "var(--wp--preset--font-size--large)"
				}
			},
			"h5": {
				"typography": {
					"fontFamily": "var(--wp--preset--font-family--system-font)",
					"fontWeight": "700",
					"textTransform": "uppercase",
					"lineHeight": "var(--wp--custom--typography--line-height--normal)",
					"fontSize": "var(--wp--preset--font-size--normal)"
				}
			},
			"h6": {
				"typography": {
					"fontFamily": "var(--wp--preset--font-family--system-font)",
					"fontWeight": "400",
					"textTransform": "uppercase",
					"lineHeight": "var(--wp--custom--typography--line-height--normal)",
					"fontSize": "var(--wp--preset--font-size--normal)"
				}
			},
			"link": {
				"color": {
					"text": "var(--wp--preset--color--foreground)"
				}
			}
		}
        }
}

Basically, h1, h2, h3, h4, h5, h6, and link, which translates to <a> element styling.

@colorful-tones colorful-tones removed their assignment Oct 28, 2021
@colorful-tones
Copy link
Member

I'm un-assigning this Issue for now, because I do not think I'll have the bandwidth, knowledge and fortitude to work through maturing some of the necessary APIs and do not want to hold up progress from anybody else. Thanks!

@Mamaduka Mamaduka added the Needs Dev Ready for, and needs developer efforts label Oct 29, 2021
@noisysocks
Copy link
Member

Sharing this comment:

As Matías mentions on this issue, it needs some considerable extra thought. There’ll be a lot of edge cases and ripple effects. Unfortunately, I believe this needs to be cut from the 5.9 wishlist.

Happy to cut this from WP 5.9. My only concern is that Twenty Twenty-one might need it. What do you think @kjellr?

@jffng
Copy link
Contributor

jffng commented Oct 29, 2021

We can do without this in 2022 since it needs a lot more consideration. Thanks for checking @noisysocks.

@noisysocks
Copy link
Member

Let's punt this then! 🥾

@zaguiini
Copy link
Contributor

zaguiini commented Dec 13, 2021

Some blocks do use actual input and button elements instead of anchors. Not having a reset/visual parity on both kinds of buttons make them inconsistent.

More information: WordPress/twentytwentytwo#295

@scruffian
Copy link
Contributor Author

I had a go at adding support for a button element: #40260

@scruffian
Copy link
Contributor Author

Closing in deference to #34198

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json Needs Dev Ready for, and needs developer efforts
Projects
None yet
Development

No branches or pull requests

8 participants