WordPress / gutenberg

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

Button: add support for iconPosition top and showDescription #62373

Open afercia opened 4 months ago

afercia commented 4 months ago

What problem does this address?

I've been exploring potential solutions for https://github.com/WordPress/gutenberg/issues/60206 and realized there's many cases where the editor uses custom components or ad-hoc CSS to modify or replace the Button base component.

One notable example of this is for the Group, Columns, and Query block placeholders. These placeholders design and accessibility would benefit from two new features of the Button component:

Cc @mirka

After a quick look at the Button component, seems to me that:

I'd appreciate any feedback and thoughts.

Use cases examples:

For better accessibility in the block placeholders that show variations, I'd like to change the design to make the variation title and description visible. Example screenshot:

Screenshot 2024-06-06 at 16 09 55

To evaluate: Other editor components e.g. PreferenceToggleMenuItem > MenuItem already show both a label and a description. They do it wrongly, by putting both the label and description as content of the button. As such, the accessible name of the button is a very long string that is a barrier for speech recognition software users ang generally not a good practice. The description should be separated from teh label and likely the visible description should be supported by the base Button component instead of reimplementing ad-hoc solutions. Screenshot:

Screenshot 2024-06-06 at 16 39 50

Finally, in the future the editor may just have the need to have buttons that show icon and label vertically stacked. Example screenshot:

Screenshot 2024-06-06 at 16 15 10

What is your proposed solution?

afercia commented 4 months ago

Note: the screenshots above are only very rough examples made by quickly altering some code and using the browser dev tools inspector. Please do not take these screenshots as a 'strict' reference. The main point is to be able to:

mirka commented 4 months ago

The Button props are bloated as it is, and I don't think it's sustainable to add any more internal layout options on top of it. The matrix of possible prop combinations is huge, and many of those combinations don't make sense at all. Even for a simple iconPosition="top", it already doesn't mesh well with the size scheme which is height-based.

One way I think we can approach this is to make Button more composable in terms of layout. Currently, the children prop does take arbitrary elements, but there are limitations to what layouts you can achieve due to paddings and fixed height on the container.

Ideally we'd have a more customizable button component (perhaps a facade component of Button with non-compatible props omitted), where it can support completely arbitrary internal layouts while maintaining the standard focus styles, accessible disabled behavior, etc.

Then it would be a matter of resusably encapsulating whatever internal layout patterns you want, at whichever layer is appropriate (wp-block-editor, your plugin, etc).

afercia commented 4 months ago

Even for a simple iconPosition="top", it already doesn't mesh well with the size scheme which is height-based.

Of course, the 'size' scheme doesn't make sense with a button that shows icon on top and label below the icon. That's inherent to a stacked layout where the actual height is determined by the content.

Extending your reasoning, then I would argue that if the 'sustainability' is a problem, many other props should be removed and any layout (size, icon position) removed entirely. I'm not sure that would be the best approach in terms of what the editor actually needs.

While I do understand arguments about the 'pureness' of the components package, I'm more on the pragmatic side. The editor needs components that are flexible. Otherwise, any 'local implementation' would be just redundant code prone to maintenance problems, maintenance cost, inconsistencies, and bugs.

joedolson commented 4 months ago

Is it worse to add additional props to the Button component, or to add more custom code in each variant usage of the component where we want an alternate design? If we want consistency across the application and maintainable code, I think that the Button component needs to support the properties that we frequently use.

This change is a pretty minimal increase in complexity for the component that can make usages more consistent across the application. It seems very reasonable to me.

I don't think our base components need to be simple; they need to have simple, sensible defaults, so that they can be used with minimal required props, but they should be richly featured so that we can use them in a wide variety of contexts without requiring extra code.

afercia commented 4 months ago

I totally agree with @joedolson. To me, the main purpose of a library of reusable base component is having components to serve the needs of WordPress in terms of functionality, usability, accessibility, and design. These are the WordPress components, they are made for WordPress. People can use them in other projects, if they like. Still, the base components should be crafted to align with the WordPress needs.

afercia commented 3 months ago

Looking back into this issue and the proposed PR https://github.com/WordPress/gutenberg/pull/62412 what I'd really like to see is the Button component to be flexible enough to avoid custom implementation, code bloat and maintenance cost in the editor.

I can think at three different examples, at least, where the editor uses custom implementations or overrides just because the Button component can't be used to implement the provided design. In all these three examples there are relevant accessibility problems:

Block placeholder buttons

bad

Style variations

Screenshot 2024-07-12 at 10 26 47

These style variations should be buttons. Instead, for design purposes, they have been implemented with a focusable diw element with a role=button: <div role="button" tabindex="0" aria-label ... The omplementaiton is broken because it doesn't fully replicates all the native behaviors of a button element e.g. they don't work with the Spacebar key. This usage of focusable div elements should not be allowed in the first place.

Menu items with description

Screenshot 2024-07-12 at 12 04 43

The descriptions of the menu items in the Options panel should be separated from the button accessible name. Instead, the description is just appended to the button text, thus polluting the button name.

All these custom implementations introduce semantics and accessibility problems that could be avoided by using the Button base component.

On the other hand, I do understand the concerns about the props bloat in the Button component and its current complexity.

Thinking at an alternative solution, at least for the iconPosition prop, how about providing a naked variant of the Button? As in: a Button variant that comes with all its built-in functionalities but that is completely unstyled. The only exception would be the focus style, which is a feature that should be always provided by the base compoennt.

Would a naked variant be more acceptable? Cc @ciampo @mirka

Regarding the new showDescription prop, I'd really like to see that being added. There's really no reason why the accessible description must be always visually hidden with no way to make it visible.

afercia commented 1 week ago

An attempt to solve this issue was tried in https://github.com/WordPress/gutenberg/pull/62412

I understand the concerns about introducing too many props and variations to the Button component.

However, there are many cases in the editor where the provided design needs a button that displays:

Currently, to achieve the provided design, a series of ad-hoc implementations and style overrides have been implemented. These implementations are sometimes not accessible, to varying degrees. Ad-hoc implementations and style overrides should be avoided in the first place because they defeat the purpose of having a library of reusable, consistent (and accessible) components.

As such:

I'm not opposed to any alternative solution but the current ad-hoc implementations are far from ideal and I'd appreciate this issue to get more focus. Thanks.

Cc @WordPress/gutenberg-core @WordPress/gutenberg-components @WordPress/gutenberg-design

mirka commented 1 week ago

Would a naked variant be more acceptable? Cc @ciampo @mirka

Yes, in fact we have been discussing the need for some kind of unstyled variant, or a way to compose Button styles more modularly (e.g. so a consumer could opt into focus indicators, size, padding styles etc. separately). I think providing modular styles could be very flexible. Maybe through a useButtonStyles hook that returns CSS classes.

afercia commented 3 days ago

Yes, in fact we have been discussing the need for some kind of unstyled variant

Thanks for the clarification and also thanks @ciampo for your feedback at https://github.com/WordPress/gutenberg/pull/62412#issuecomment-2422338184

Can you please point other contributors (including myself) to where that discussion has taken place? Thanks.

afercia commented 3 days ago

a way to compose Button styles more modularly (e.g. so a consumer could opt into focus indicators, size, padding styles etc. separately)

To some extent, I'd agree. However, things like focus indication should be built-in and not be modular. Since the very beginning one of the goals of the reusable components library has been to provide the highest level of accessibility built-in in the components. As such, focus indication must be a built-in feature.

mirka commented 6 hours ago

Can you please point other contributors (including myself) to where that discussion has taken place?

Nothing concrete or more detailed than that one sentence summary, yet. But I think we've all been feeling the need for something like it, so let's discuss specifics in an issue when we're ready to take it on.

However, things like focus indication should be built-in and not be modular.

I'm not immediately sure if we can cover all styling needs with a single focus style, but yes, it would make things a lot simpler if that's possible.