Fix button toggles.

[jasmussen]

Fixes #18825.

This restores the rules necessary for the delicate formatting. I know it's not pretty code, but I think we should restore it for now. We can revisit and very probably improve it a great deal if we look at the improvements from #18667. But that should be separate.

[youknowriad]

@jasmussen I thought we discussed this and agreed that consistency is better there :)

The problem here is that you'll notice that the dropdowns in the toolbar don't have the same styles. Also, this assumes that all toolbars are these small bordered rectangles which is not always the case, I don't think the default Toolbar component has these styles. For example, you can trigger the "Top Toolbar" mode and you'll see that these new styles are not great because they don't allign with the styles of the other buttons in the top toolbar.

I see two options:

• Keep things as is in master
• Target specifically the buttons of the block contextual toolbar and apply these overrides and avoid adding these in a generic way to all ToolbarButton components.

[gziolo]

It regressed in the top toolbar as well, from master:

[youknowriad]

Oh I guess, this restores the toggled state as well. So yeah! let's restore just that part (and not the hover styles etc...), also it makes me wonder why the toggled styles are not by default in the Button component instead of the Toolbar one. It seems that the component already has anisToggled prop so it should have the corresponding styles as well;

[gziolo]

I opened PR which refactors code to stop using aria-pressed as a primary way to indicate that the button was pressed but promotes isToggled prop which was never styled:
#17748

Maybe we should land it first and apply all those styles using isToggled class name?

[gziolo]

I also opened #18817 which ensures that we don't use button tag names in the toolbars directly, but rather we promote usage of Button or ToolbarButton components.

Putting those two together, it should help to make it much easier to style those buttons.

[gziolo]

The problem here is that you'll notice that the dropdowns in the toolbar don't have the same styles.

Yes, I can see it:

[gziolo]

Bonus thing, should it have any outline when focused even when it's disabled but focusable?

[jasmussen]

The problem here is that you'll notice that the dropdowns in the toolbar don't have the same styles. Also, this assumes that all toolbars are these small bordered rectangles which is not always the case, I don't think the default Toolbar component has these styles. For example, you can trigger the "Top Toolbar" mode and you'll see that these new styles are not great because they don't allign with the styles of the other buttons in the top toolbar.

Yes, absolutely. But it's a somewhat non-trivial refactor, because the toggled state requires the hover style to be slightly inset.

I agree it's in need of a refactor, but it feels like this refactor might be better saved for exploring an implementation of the UI in #18667.

[jasmussen]

To clarify a bit further, I don't have a strong visual attachment to how things look in master right now — consider this more of a "revert" PR for the toggle state. If @gziolo has a better refactor, definitely feel free to merge that instead of this one!

[gziolo]

The problem here is that you'll notice that the dropdowns in the toolbar don't have the same styles. Also, this assumes that all toolbars are these small bordered rectangles which is not always the case, I don't think the default Toolbar component has these styles. For example, you can trigger the "Top Toolbar" mode and you'll see that these new styles are not great because they don't allign with the styles of the other buttons in the top toolbar.

Yes, absolutely. But it's a somewhat non-trivial refactor, because the toggled state requires the hover style to be slightly inset.

When I was playing with it on Friday, I wasted 2 or 3 hours, the issue was that the toggled button had this additional outline coming which aligned with the rest of buttons but it looked ugly 😅
I also tried to target with styles only those buttons which were toggled, can you try to do it? I failed :)

I agree it's in need of a refactor, but it feels like this refactor might be better saved for exploring an implementation of the UI in #18667.

I tend to agree. Let's apply the minimal set of changes to move forward.

[youknowriad]

What about the minimal change for me to avoid regressions being a isToggled style added to the Button component.

[gziolo]

I opened #18868 with an alternative fix proposed.

[jasmussen]

Closing in favor of #18868.