Toolbar: review the toolbar button stylesheet

[afercia]

Description

Splitting this out from #56123 (comment)

It appears the packages/components/src/toolbar/toolbar-button/style.scss file only contains CSS that is scoped to some kind of data-subscript HTML attribute used on the toolbar buttons.

After some code archeology, I have the impression this is no longer in use in the editor and it was meant for some subscript text within the toolbar buttons / dropdowns like the heading levels.

I'm not 100% sure it is no longer in use and I'd appreciate some feedback from anyone more familiar with this. Cc @WordPress/gutenberg-design

If it is proved it is no longer in user, this file could be entirely removed together with occurrences of data-subscript in the codebase. I found at least two occurrences of a data-subscript attribute and subscript prop in two files:
packages/components/src/button/index.native.js
packages/components/src/toolbar/toolbar-button/index.tsx

Also, I'm not sure why some toolbar buttons are supposed to have a components-toolbar__control CSS class. That seems part of some old implementation as well.

Step-by-step reproduction instructions

• Check the contents of the packages/components/src/toolbar/toolbar-button/style.scss file
• Observe all CSS rules are scoped to a data-subscript attribute.
• Observe also the position: relative; property is meant for the subscript text positioning.
• Check for actual usage of subscript text within the buttons (I couldn't find any).

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[ciampo]

While Gutenberg may be not using the subscript property for the ToolbarButton component, removing it would potentially be a breaking change for any third-party consumers of the library that are still relying on that feature.

If design folks confirm that this is not anymore a feature that this component should support, we could at most discuss deprecating it?

[jasmussen]

If I recall correctly, the subscript was used for heading levels, where we used a single SVG icon for the "H", and then a subscript for 1-6 to generate the 6 levels of headings as an icon. These were replaced with 6 individual, and tailored SVG icons, one for each heading level. If I am remembering correctly then I also believe that feature isn't used anymore.

[afercia]

Thanks both for your feedback,

I'm not sure why some toolbar buttons are supposed to have a components-toolbar__control CSS class. That seems part of some old implementation as well.

Any thoughts on this other point?

[ciampo]

I'm not sure why some toolbar buttons are supposed to have a components-toolbar__control CSS class. That seems part of some old implementation as well.

Any thoughts on this other point?

One potential reason for classnames to be kept around is for backward compatibility reasons.

In fact, even though internal components classnames are not part of the public APIs of components (and shouldn't therefore be used by external consumers), the truth is that there are usually quite a lot of style overrides that rely on those classnames. Therefore, when we refactor some components, we usually keep those classnames around to avoid UI regressions (even if we stop using those classnames internally).

Specifically to .components-toolbar__control, as you mention, the classname is used internally by the ToolbarButton component to apply some styles which could be mostly removed — although there's a position: relative rule that, if removed, could cause some potential regression.

Unfortunately 😱 I can also see that the classname is also:

• used by external consumers 😱 (example, example, example, example)
• used externally to the package to apply some style overrides 😱

Instead of using .components-toolbar__control, those external consumers should define their own private classname, and use that classname to apply style overrides if needed (although I also discourage applying style overrides when possible).

So, as a recap:

• We could remove all of the subscript-related code, including styles
• We should be careful removing the position:relative rule
• We should be refactoring all usages of .components-toolbar__control outside of the components package to use a different classname
• We could, after all of this, consider removing the .components-toolbar__control classname, although that could cause some UI regressions on third-party consumers that were relying on that classname (even though technically that classname is not part of public APIs)

[afercia]

although there's a position: relative rule that, if removed, could cause some potential regression.

I think that was only meant for the absolute positioning of the subscript text, as absolutely positioned element need a 'relative container'.

used externally to the package to apply some style overrides 😱

😱

[ciampo]

What do folks think about the proposed plan of action?

So, as a recap:

• We could remove all of the subscript-related code, including styles
• We should be careful removing the position:relative rule
• We should be refactoring all usages of .components-toolbar__control outside of the components package to use a different classname
• We could, after all of this, consider removing the .components-toolbar__control classname, although that could cause some UI regressions on third-party consumers that were relying on that classname (even though technically that classname is not part of public APIs)

I may not be able to work on this first hand, but I'm happy to help with reviewing PRs.