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

Button: add support for iconPosition top and showDescription #62373

Open
afercia opened this issue Jun 6, 2024 · 16 comments · May be fixed by #62412
Open

Button: add support for iconPosition top and showDescription #62373

afercia opened this issue Jun 6, 2024 · 16 comments · May be fixed by #62412
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@afercia
Copy link
Contributor

afercia commented Jun 6, 2024

What problem does this address?

I've been exploring potential solutions for #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:

  • The ability to make a button show the Icon at the top and the text below the icon. Currently, there's only support for iconPosition?: 'left' | 'right';
  • The ability to show the Button description. Currently, when passing a description to the describedBy prop, a visually hidden element is rendered after the button and properly referenced with aria-describedby/id attributes. The description is visually hidden but in some scenarios it would be beneficial to make it visible.

Cc @mirka

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

  • Support for iconPosition="top" would be pretty trivial. The DOM order would be the same as left. It would likely need only a CSS class and some styling.
  • A new prop showDescription seems pretty trivial as well. When true, it should make the button description use a paragraph insteaed of the VisuallyHidden component.

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 afercia added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components labels Jun 6, 2024
@afercia
Copy link
Contributor Author

afercia commented Jun 6, 2024

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:

  • Show a meaningful label of the button.
  • Show a description that is separate from the button label and doesn't pollute the button accessible name.

@afercia afercia self-assigned this Jun 7, 2024
@mirka
Copy link
Member

mirka commented Jun 7, 2024

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
Copy link
Contributor Author

afercia commented Jun 7, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

afercia commented Jun 11, 2024

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
Copy link
Contributor Author

afercia commented Jul 12, 2024

Looking back into this issue and the proposed PR #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

  • The visible text below the buttons is just unsemantic text placed there. This text should be the label (the text content) of the buttons.

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
Copy link
Contributor Author

afercia commented Oct 11, 2024

An attempt to solve this issue was tried in #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:

  • An icon in the top part of the button.
  • A label below the icon.
  • A visible description.

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:

  • Either the Button needs to be changed to implement the design.
  • Or, there is a need of a new base component to achieve the design in an accessible way.
  • Or, the design needs to be changed.

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

@afercia afercia added the Needs Design Feedback Needs general design feedback. label Oct 11, 2024
@mirka
Copy link
Member

mirka commented Oct 11, 2024

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
Copy link
Contributor Author

afercia commented Oct 21, 2024

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 #62412 (comment)

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

@afercia
Copy link
Contributor Author

afercia commented Oct 21, 2024

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
Copy link
Member

mirka commented Oct 24, 2024

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.

@afercia
Copy link
Contributor Author

afercia commented Oct 28, 2024

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.

Thanks for clarifying.

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

The problem when using the pronoun 'we' is that it's not clear who 'we' is in that context. In these cases, it should be always clarified who 'we' is and when and where any discussion occurred. In a collaborative open source project, discussions should be open and public, thanks.

@mirka
Copy link
Member

mirka commented Nov 26, 2024

Issue for unstyled Button usage created at #67320

@afercia
Copy link
Contributor Author

afercia commented Nov 27, 2024

Thanks for creating the new issue @mirka that would address the styling part (icon on top).

IMO, the button still needs a way to make the description visible. At the moment, the component assumes the description must always be visually hidden while there are legitimate cases where we need to have a visible description.

<VisuallyHidden>
<span id={ descriptionId }>{ description }</span>
</VisuallyHidden>

Making it visible to better illustrate:

Image

@draganescu
Copy link
Contributor

I would like to echo @joedolson 's question:

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.

I think we need to be aware of some semantics, sorry if it will sound pedantic: simple does not mean not complex. Simple means not complicated.

There is inherent complexity with providing foundational tooling: one needs sensible defaults but also flexibility, configurability and maintainability too.

The focus is to avoid complication.

Bloated APIs are complicated, in that they inevitably bear the possibility of incompatible combinations and unforeseen behaviours. Avoiding complication is possible, but avoidiing complexity is not. Hence the directions suggested by @mirka and @ciampo seem to me to be a good approach in extending what we can do, but in a manageable future proof way. Neither is about "let's have less props", it's about let's make sure we keep things simple depite them being complex to solve.

At the same time I too fully support buttons with vertical icons, the separation of label and description, design API that removes the need to make divs with button roles and sensible ways to always enable text (labels instead of icons, and visible descriptions). Except that it takes time to solve properly if we want to solve it at the most basic level.

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?

I think it's worse to have custom code long term, but not short term. Just like the engineering/design ICs had to do with what they've got and made the style selection buttons divs with role button because buttons can't temporarily support that kind of design. But we can't not evolve the product because we don't support stuff at component level. It's the exact opposite, the design pushes the system to see the limitations.

I think @afercia 's work here to collect all these problesm is awesome. It connected in my head to @youknowriad 's work to keep track of things that engineering had to go around - for instance we still hardcode WP specific stuff in non WP specific packages, because the foundation is not ready, but we learn (see #61622) and improve accordingly in steps. So it's not only design. It's the fact that the product will always, always be farther into the future compared to the fromework, there is always some catch up to do.

@afercia
Copy link
Contributor Author

afercia commented Nov 28, 2024

But we can't not evolve the product because we don't support stuff at component level.

I totally agree but I would argue the base components should move fast to support what the product needs.
Otherwise, the big risk is accumulating a ton of technical debt in the product implementation just because the base components are behind. That has a huge maintenance cost and as we all know refactoring at a later time is costly and sometimes it never happens because of other priorities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants