Search block: Add typography supports #43499
Conversation
ramonjd
added
[Type] Enhancement
ramonjd
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
ramonjd
changed the title
|
Size Change: +552 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for tackling typography on the search block @ramonjd 👍
In general, this works for me in the editor however I hit a couple of small issues on the frontend.
If we continue with the approach of applying the typography support styles to the root of the Search block, we'll need to also inherit font-weight, font-style, and text-transform on the button as you did for letter-spacing.
| Editor | Frontend |
|---|---|
 |
 |
Also, I know text-decoration is an extreme edge case in most situations however it feels to me like it's more of a problem when a user is typing a value into the search input and it's appearing with the line-through style.
Additionally, the input with text decoration appears to be rendered differently across browsers. Firefox ignored the text decoration, Safari only rendered the line through for the user entered text, and Chrome rendered it for both the placeholder and entered text.
The current directive is to supply consistent typographical design tools (albeit optionally displayed). Would we be better served here by skipping the serialization of the typography supports and applying them manually to the label and button elements? Then leveraging the feature level selectors so that global styles and theme.json apply their styles only to those same elements.
What do you think?
I knew it was too good to be true. 😄 Thanks for testing. I'll sort it. |
|
I have the typography styles applied to the label, input (with the exception of text-decoration) and button just so we can test things out. Safari and Firefox looking okay! I think the input does require at least font-size, otherwise things look a bit uneven:
TODO
|
ramonjd
force-pushed
the
update/search-block-typography-support
branch
from
8f10249 to
bc10961
Compare
| @@ -360,13 +378,118 @@ function styles_for_block_core_search( $attributes ) { | |||
| $button_styles[] = sprintf( 'background: %s;', $attributes['style']['color']['gradient'] ); | |||
| } | |||
|
|
|||
| // Add typography styles. | |||
| $has_font_size = ! empty( $attributes['style']['typography']['fontSize'] ); | |||
There was a problem hiding this comment.
Note to self: this is possibly something the style engine could optimize, since it will do all the $has_ checks internally, and return the classnames.
E.g.,
$typography_styles = gutenberg_style_engine_get_styles( array( 'typography' => $$attributes['style']['typography'] ) );
$button_styles = $typography_styles['css'];
$input_styles = $typography_styles['css'];
if ( $show_label ) {
$label_styles = $typography_styles['css'];
}Doesn't take into account no textDecoration for the input.
Maybe a follow up refactor anyway....
|
Styling the placeholder text and the input field text can make the block less accessible, for example by using a too low contrast. I would prefer if these options did not apply to the input field at all. If that is not an option, is there at least a specific color contrast warning? |
There was a problem hiding this comment.
@ramonjd Thanks for taking the leap to skip the serialization of typography styles for the Search block. 🙇
I'm not sure my suggestion to skip serialization and use feature-level selectors works unless we skip applying all typography from the input. As this PR stands now, if you configure text-decoration via theme.json, it will still be applied to the input.
You can test this via the snippet below:
"core/search": {
"typography": {
"textDecoration": "line-through"
}
}I agree with your earlier comment though about things looking odd and out-of-whack if we don't apply the non-decoration typography styles to the input.
At this stage, I don't believe we can exclude text-decoration styles from being included in the theme.json/global styles generated stylesheets. So short of adding that, we could override it via the block's CSS. However, due to the specificity of the generated selector, we'd need to fight that if taking this approach.
I appreciate the next suggestion will be a pain, but maybe we need to tweak the approach again. Could we do something similar to the following?
- Continue to skip serialization of typography supports
- Remove use of feature-level selectors
- Apply the typography supports (excluding text-decoration) on the root wrapper
- Apply text decoration to the button and label if appropriate
- Add CSS override (which should only have to be now more specific than
.wp-block-search)
What do you think?
I'd be happy to help on this or create an alternative PR for comparison if you think it would help.
Styling the placeholder text and the input field text can make the block less accessible, for example by using a too low contrast
@carolinan, unless I'm misunderstanding something, this PR should only be applying typography styles to the input. The color block support styles remain unchanged.
That said, I appreciate that color isn't the only characteristic that might impact accessibility. How much of an issue is changing font size or weight? Particularly, if it matches the button as per Ramon's earlier example?
No problems at all. Thanks for retesting and the advice. I'll try to work things in the direction you mentioned. |
ramonjd
force-pushed
the
update/search-block-typography-support
branch
from
3bfb881 to
745f682
Compare
|
Failing tests seem related. |
Ensuring that letter spacing is inherited.
…ception of text-decoration for the input element) so we can test and eliminate features we think aren't required.
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
Apply all typography styles except text decoration to block wrapper Inherit block wrapper styles in CSS for each element Use getFontSizeClass() Remove __experimentalSelector Make getTypographyClassesAndStyles() stable
ramonjd
force-pushed
the
update/search-block-typography-support
branch
from
432c77c to
3865941
Compare
I think it was the font weight that I got confused with color, as in the example, the light font weight (left image) is barely readable. Font size should not be an issue (disregarding cases where a user choose a font size that is too small in general) |
There was a problem hiding this comment.
Props for the continued iterations on this while wrangling everything else you are doing for 6.1 @ramonjd 🙇 Nice work.
✅ Typography styles are appropriately applied for individual blocks in both editors and the front end
✅ Search block typography can be styled via theme.json or Global Styles (with small compromise)
As we've encountered previously, without using a feature-level selector, we can't prevent theme.json or global styles text decoration from being applied to the input. The downside of omitting styles for the input, resulting in its font size not matching the label and button, is more severe. I think the current approach is a fair compromise, particularly given how extremely niche any use of text-decoration would be for this block.
I also left an inline comment regarding a small unnecessary change we should remove before landing this.
| const textFieldStyles = { | ||
| ...( isButtonPositionInside | ||
| ? { borderRadius } | ||
| : borderProps.style ), | ||
| }; |
There was a problem hiding this comment.
After rolling back previous changes, I don't think we need this change anymore.
There was a problem hiding this comment.
Thanks for testing again @aaronrobertshaw, and for spotting the unnecessary code snippet here.
It made me see something else: I'd forgotten to roll back the application of the font size and family preset classnames to the individual elements. I've applied them to the wrapper only now.
You shouldn't see any differences in testing.
If it looks okay on Monday, please feel free to merge this PR. 🙇 🙇 🙇 🙇
…er, not on the individual elements.
This reverts commit f778d69.
…he wrapper, not on the individual elements." This reverts commit 384ce7d.
This commit reverts the approach to an earlier option tried so we can more reliably apply user-made typography selections without simple theme styles overriding the previous inherit styles. This includes changes refining the previous approach so that text decoration isn't applied to the input by theme.json or global styles. It also fixes a bug around font family class generation in the render callback.
There was a problem hiding this comment.
Thanks for refining this PR further, @ramonjd . 👍
While re-testing, I found some themes adding a simple .wp-block-search__button style would break the application of user-made typography selections due to the low specificity of the inherit rule.
I wrestled a bit with trying to find a neat way to overcome this. Including:
-
Applying just styles like
font-sizeandfont-familyto the button and label elements. The result is pretty inconsistent in terms of where styles are being applied as well as which styles should be. -
Adding custom classnames that we could hook onto to increase the specificity of the inherit styles. This worked for individual blocks but not the styles generated by theme.json or global styles.
-
Ultimately, I circled back on one of your previous approaches where we apply all the styles to the inner elements and use feature-level selectors so the theme.json and global styles generated styles target the inner elements.
- Without text-decoration being applied on the parent we can override the theme.json and global styles generated class and forcibly unset the text-decoration for the input.
- Applying the styles on the inner elements helps mitigate the risk of themes such as TwentyTwentyTwo overriding user-made selections.
- Via the feature-level selectors we can better control the specificity of the search block's styles via theme.json and global styles.
Given the heavy changes I've made to the PR and going around in circles a bit. I think we need some fresh eyes to review this again.
There was a problem hiding this comment.
Looks pretty close! It's working well in testing, except for a small detail: with big line heights the button icon is slightly off-center:
Adding vertical-align: text-bottom; to .wp-block-search__button svg seems to fix it.
Apart from that, just a couple questions below.
| @@ -6,6 +6,7 @@ export { | |||
| getBorderClassesAndStyles as __experimentalGetBorderClassesAndStyles, | |||
| useBorderProps as __experimentalUseBorderProps, | |||
| getColorClassesAndStyles as __experimentalGetColorClassesAndStyles, | |||
| getTypographyClassesAndStyles, | |||
There was a problem hiding this comment.
Should we be marking this experimental?
There was a problem hiding this comment.
@ramonjd initially had this as experimental until I suggested it be stabilized as we'll be stabilizing all experimental block support APIs post-6.1. This would just be another experimental API to process. Given it follows the same pattern as the other block supports, e.g. color and border, and they're due for stabilization. I figure we can save double handling this one.
We can change this if you have strong concerns.
There was a problem hiding this comment.
That makes sense to me! If color/border are similar and we're not planning on making further changes before stabilising, I wouldn't be opposed to doing it pre-6.1 either. Or were we aiming to stabilise all block supports together? It might take a while to get layout up to spec 😬
There was a problem hiding this comment.
To my knowledge, there are no plans for further changes to these util functions. I was under the impression that we'd stabilize each block support in full, e.g. all the typography support APIs, then color, and so on. Layout support has always been a special case, so I don't think there is a rush to stabilize it before it's ready.
Regarding stabilizing the others pre-6.1, I don't think that's on the cards. My focus has been on adopting the block supports more consistently within Gutenberg and then seeing if there is anything we should alter before stabilizating them.
That said, if there's a window in which we could actually get it all done, I wouldn't be against it.
Good catch @tellthemachines, I'll tweak the styles accordingly. Thanks. |
This centres the icon for the icon only button on the frontend when it has a tall line-height.
Related:
What?
Adds typography supports to the Search block.
Why?
How?
Testing Instructions
Example block code
Example theme.json snippet.
Screenshots or screencast
2022-08-25.16.05.55.mp4