[ToggleButton] Improve customizability

[simshaun]

• I have followed (at least) the PR section of the contributing guide.

Fixes #17144
Fixes #17145

Also improves documentation on individual usage of ToggleButton.

[mui-pr-bot]

Details of bundle changes.

Comparing: 7b6a14b...93c4408

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	0.00%	-0.01%	329,534	329,534	90,009	90,003
@material-ui/core/Paper	0.00%	0.00%	68,774	68,774	20,487	20,487
@material-ui/core/Paper.esm	0.00%	0.00%	62,148	62,148	19,215	19,215
@material-ui/core/Popper	0.00%	0.00%	28,468	28,468	10,177	10,177
@material-ui/core/Textarea	0.00%	0.00%	5,094	5,094	2,137	2,137
@material-ui/core/TrapFocus	0.00%	0.00%	3,834	3,834	1,614	1,614
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	16,386	16,386	5,827	5,827
@material-ui/core/useMediaQuery	0.00%	0.00%	2,541	2,541	1,058	1,058
@material-ui/lab	+0.16% 🔺	+0.09% 🔺	153,314	153,555	46,716	46,757
@material-ui/styles	0.00%	0.00%	51,492	51,492	15,305	15,305
@material-ui/system	0.00%	0.00%	15,658	15,658	4,371	4,371
Button	0.00%	-0.01%	78,778	78,778	24,073	24,071
Modal	0.00%	0.00%	14,346	14,346	5,009	5,009
Portal	0.00%	0.00%	2,907	2,907	1,319	1,319
Rating	0.00%	+0.01% 🔺	70,245	70,245	21,942	21,945
Slider	0.00%	0.00%	74,483	74,483	23,065	23,065
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	0.00%	0.00%	52,305	52,305	13,790	13,790
docs.main	+0.04% 🔺	+0.02% 🔺	597,308	597,555	190,855	190,892
packages/material-ui/build/umd/material-ui.production.min.js	0.00%	0.00%	300,439	300,439	86,419	86,419

Generated by 🚫 dangerJS against 93c4408

[simshaun]

Hey, not sure what I should do about the argos failure.

[oliviertassinari]

@simshaun I have done an extra commit with the following diffs:

• Remove the tests, I don't think that they will help in the future
• Use the border-radius value from the theme in ToggleButtonGroup
• Use a similar convention to ButtonGroup for the style rule names
• Simplify the demos, the standalone mode is not the primary purpose of the component

[simshaun]

Demo is messed up in dark mode:

Cause:
<Box bgcolor="white" borderRadius={4}>

I think it's best to let a standalone inactive ToggleButton have a transparent background. Any background color makes me thing it's selected.

Edit: I've pushed a commit with <Box> removed.

[mbrookes]

We had a demo in v0 of a checkbox with custom icons as a pseudo toggle-button, but if the styling of this one suits, then fair enough I suppose!

The styling looks wrong with a transparent background, but, as you say, could be misconstrued as selected with one. That says to me this component isn't the best tool for the job, since it isn't what it was designed for.

If these changes have other benefits, then probably still worth applying.

[simshaun]

@mbrookes

In my actual use-case, my app's design makes heavy use of transparent BG controls, so the transparency works fine (for me).

Honestly, trying to use the checkbox component didn't even cross my mind... I had a group of toggle buttons and then wanted to add a standalone toggle button off to the side that looks the same as the others. I didn't make the mental leap to "this is basically a checkbox with a different icon/text". I guess as long as I can make it look like the toggle buttons without much difficulty, this PR isn't really necessary.

Though, if ToggleButton shouldn't be used standalone, maybe some sort of warning or tip in the docs/console could be handy to ward off misuse.

[mbrookes]

The color tool swatch checkboxes use restyled RadioButtons, which are a peer of the checkbox (based on the same underlying component), so it's not inconceivable.

I'm assuming that since @oliviertassinari has supported this PR, that he is in favour of the changes.

I'm wondering if removing the demo, while leaving the other changes would strike a balance between explicitly supporting the use case, vs. "it just so happens to work"?

[oliviertassinari]

I don't have any specific preference. However, I think that this discussion is a good opportunity to define the steps ahead. I have done the following benchmark:

• Gmail.
  
• ckeditor.

• tiny.

• froala.
  

They all seem to:

• have aria-label and aria-pressed attributes on each action of the toolbar. I think that we could update the demos and the core implementation to have it.
• have a transparent background. I'm wondering if we shouldn't remove the background-color property from the group. It could be better handled in a parent element. However, this seems to be a specificity of the Material Design spec, so we might not be able to change it. Overall, I think that a demo with more control on a single bar would be more realistic. Alternatively, we might be able to create a great customization example with one of the above sources of benchmark.
• have no radio or checkbox backing the logic.
• Some support a standalone mode, some don't. I think that it makes sense to do it.

What do you guys think, are you happy with the above path forward?

[mbrookes]

Sounds good to me.

Overall, I think that a demo with more control on a single bar would be more realistic.

I think you just found a use-case for the vertical separator 😄.

[simshaun]

have aria-label and aria-pressed attributes on each action of the toolbar. I think that we could update the demos and the core implementation to have it.

Good idea. I was reading up on aria-pressed and it got me wondering about the appropriate markup for ToggleButtonGroup and children. This is likely worthy of a new, separate issue to flesh out details. e.g. Determine the best way to mark up ToggleButtonGroup and ToggleButton(s) in each scenario:

• ToggleButtonGroup exclusive selection:
  • ToggleButtonGroup has role="group"
  • ToggleButtons have role="menuitemradio", aria-checked="true/false", and maybe aria-label attributes.
  • an example, http://jongund.github.io/oaa-examples/examples/menubar/menubar-2/menubar-2.html (Jon Gunderson does a lot of work on W3C Aria Practices. I'd consider him a reliable source.)
  • FWIW, Angular Material's example uses role="group" on their ToggleButtonGroup and role="button" on their ToggleButtons. Their docs only have an example of exclusive selection though. I haven't bothered seeing what they do in a multiple selection scenario, though they describe it: https://material.angular.io/components/button-toggle/overview#accessibility
• ToggleButtonGroup multiple selection:
  • ToggleButtonGroup has role="group" and aria-label="......" attributes
  • ToggleButtons have role="menuitemcheckbox", aria-pressed="true/false", and, if necessary, aria-label="...." attributes
• Standalone ToggleButton:
  • ToggleButton has role="button", aria-pressed="true/false", and maybe aria-label.

https://www.w3.org/WAI/PF/aria/roles

have a transparent background. I'm wondering if we shouldn't remove the background-color property from the group. It could be better handled in a parent element. However, this seems to be a specificity of the Material Design spec, so we might not be able to change it.

I'm looking at https://material.io/components/buttons/#toggle-button and I don't think a background-color is required per se. Only that "a toggle button’s state makes it clear which button is active." One of the examples they give are these standalone heart icon toggles.

In the following example, the toolbar is essentially transparent (white on white).

So, I don't think the background matters as long as it's clear when a button is active.

[oliviertassinari]

@simshaun This sounds good to me. I think that the changes here are already a good step forward. Maybe we should merge this pull request and follow-up in another one?

What do you guys think?

I think you just found a use-case for the vertical separator 😄.

@mbrookes Yes! 😍😂

[oliviertassinari]

Thanks