InputControl: Allow inline styles to be applied to wrapper instead of inner input

[aaronrobertshaw]

Related:

• UnitControl: Normalize wrapper classnames by removing outer wrapper #45139

What?

• Updates the InputControl to apply any passed inline styles to the wrapper rather than the inner input.

Why?

When #45139 removed the outer wrapper it allowed inline styles to be passed to the inner input rather than the InputControl's outer wrapper.

Applying them to the outer wrapper appears to be the expected behaviour. A number of blocks control's were just updated to remove passing of inline styles to restrict the control's outer width (#45329).

It also turns out the block gap control passes a CSS grid style to achieve it's layout. This won't work without this fix.

How?

Destructure the inline style prop for the InputBase component and pass it to the wrapper instead of the input via the rest props.

Testing Instructions

1. On trunk, edit a post, add a group block, select it and switch to a custom block gap
2. Note the broken layout for this control.
3. Checkout this PR, reload the editor and navigate back to the block gap control
4. The layout should be fixed.

Screenshots or screencast

Before	After

✍️ Dev Note

In order to improve consistency across the @wordpress/components package, any inline styles passed to the InputControl component through its style prop will be applied to its outer wrapper element, instead of an inner input wrapper element.

These changes may also affect usages of other components relying on InputControl, such as NumberControl and UnitControl.

[codesandbox]

  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

[ciampo]

The change makes sense to me, but it has the potential of introducing regressions for usages of InputControl / NumberControl / UnitControl etc.. that were relying on the style prop — we should probably take a close look at that to ensure that there are no regressions.

Curious to hear @mirka 's thoughts on this one too

[aaronrobertshaw]

The change makes sense to me, but it has the potential of introducing regressions for usages of InputControl / NumberControl / UnitControl etc..

For what it's worth, we can remove UnitControl from that list.

The changes in #45139 meant a unit control's style prop moved those inline styles from it's wrapper to the inner input.

The potential regressions for NumberControl & InputControl are a problem but I wondered whether this was a case of actually moving the inline styles to where they were expected. That doesn't sound like the case then.

Could a compromise be to introduce a new prop that UnitControl can leverage to fix the regression caused by #45139?

[ciampo]

I wondered whether this was a case of actually moving the inline styles to where they were expected. That doesn't sound like the case then.

In my opinion, for the same reason we expect className to be applied to the outer wrapper, I would also expect styles to be applied to the outer wrapper. In that sense, this change moves the inline styles to where I would expect them!

I'm only careful about the potential repercussions of this change (which is why I suggested we took a closer look at how inline styles are passed to InputControl & derived)

[mirka]

In terms of regressions, style is not part of our public contract, so I'm only thinking about regressions in the Gutenberg app.

Doing a quick scan of our codebase, I don't see a lot of instances of style usage on InputControl and friends. Only two, if I'm not mistaken, and they are not valid use cases for style. (It's an imperfect way to search, but I often use a regex like <UnitControl[\w\n]([^>])*style for these things.)

My preference will be to fix the app-side code so it doesn't rely on style. I don't think it's ever necessary to pass inline styles, and it's fragile by nature since wp-components does not guarantee that the inner element structure is stable. It also has a lot of consistency implications for our other components, which pass down restProps to non-wrapper elements, or do not pass down restProps at all.

I definitely do not want style to be explicitly documented in our props tables. That would really open a can of worms (i.e. what about all the other possible restProps?).

[ciampo]

I think I agree with a few different points that don't necessarily contradict each other:

• I agree that using inline styles is mostly unnecessary and fragile, and it would be definitely good if we removed inline style usage from the app-side
• I also agree that, if inline styles were to be applied to a component, I'd expect them to be applied to the outer wrapper
• Finally, I definitely agree that we don't want the style prop to be part of our docs / prop tables (it would be if we added it explicitly, instead of via WordPressComponentProps)

[mirka]

So I guess my suggested approach is two-fold:

• Fix the app-side code so it doesn't rely on style.
• Don't do anything about the style forwarding. It would be too much overhead to consistently guarantee for all our components that some subset of restProps always go to a wrapper. Undocumented restProps are not part of the public API, so behavior is undefined.

The latter is just my opinion on what I think is the sustainable approach, but if y'all think it's worth making an exception in certain places, I won't block it 😄

[andrewserong]

Great discussion here, and thank you for digging into a fix @aaronrobertshaw!

In my opinion, for the same reason we expect className to be applied to the outer wrapper, I would also expect styles to be applied to the outer wrapper. In that sense, this change moves the inline styles to where I would expect them!

Same, and I agree with the points above that it'd be good to update usages within Gutenberg to avoid the inline styles. Though @mirka makes a very good point about the expected API. I suppose a challenge from my perspective is that I'm used to the idea of importing relatively simple components where things like Flex, View, or Button do allow passing in a style prop explicitly to the wrapper, whereas the more complex / compound components don't, so it can be a little confusing to work out which ones come as-is and which can be custom styled 🤔

If support for passing style in to the component and having it be applied to the wrapper isn't preserved, it might be worth flagging it as a breaking change even if it was never intended to be used that way? My main concern would be if Gutenberg has had examples that use the inline style for a while, there's a chance that other consumers might have copied it too. Though I imagine UnitControl might be one of the lesser used components outside Gutenberg?

[aaronrobertshaw]

Thank you for the discussion, thoughts, and concerns, everyone 🙇

Doing a quick scan of our codebase, I don't see a lot of instances of style usage on InputControl and friends. Only two, if I'm not mistaken, and they are not valid use cases for style.

Those two are for the native component for UnitControl, so are a slightly different case, and not the focus of this PR.

There is one other case, the SpacingSizesControl. I have put up a quick PR removing its use of inline styles: #45412.

It's an imperfect way to search, but I often use a regex like <UnitControl\w\n*style for these things

Thanks, I was a little paranoid about missing cases so searched only for uses of UnitControl and manually inspected each one.

There are a couple of edge cases here;

1. The LetterSpacingControl spreads rest props onto its UnitControl
2. The BoxControl passes through anything in its inputProps to the underlying UnitControl uses, so they could also conceivably be given style props.

Personally, I don't feel either of those are big issues.

If support for passing style in to the component and having it be applied to the wrapper isn't preserved, it might be worth flagging it as a breaking change even if it was never intended to be used that way?

That's the question we need consensus on. Do we restore style going to the wrapper (typing via WordPressComponentProps) or flag #45139 as a breaking change?

At this stage, I'm happy to go either way.

[ciampo]

restore style going to the wrapper (typing via WordPressComponentProps)

This to me seems the easier change, with the lower impact (basically the style prop reverts back to behaving like it used to before #45139 — but let's hear Lena's opinion on this one too.

[mirka]

restore style going to the wrapper (typing via WordPressComponentProps)

I'm fine with this too 👍

[aaronrobertshaw]

This PR has been updated now to leverage WordPressComponentProps to type InputBase. It's also been rebased to address the changelog conflict.

It should be right for a final review now unless I've missed something.

[ciampo]

LGTM 🚀

Code changes look good, and the style prop works as it was agreed

[bph]

Flagged it for a mention in a 'Component' changes Dev Note

If support for passing style in to the component and having it be applied to the wrapper isn't preserved, it might be worth flagging it as a breaking change even if it was never intended to be used that way? My main concern would be if Gutenberg has had examples that use the inline style for a while, there's a chance that other consumers might have copied it too. (by @andrewserong )

I'd rather err on the side of caution.