BoxControl: update design

[brookewp]

What?

While working on adding updated sizing in #56185, we noticed that the larger sizing affects the visibility of the input for BoxControl when the width is constrained. And while there is a max-width on this component, the main concern is this happens regardless due to the width of the inspector:

Why?

BoxControl is only used in the dimension panel for backward compatibility. So it was decided to change the design of the component to match custom padding when presets are provided (excluding the settings icon): #56185 (comment)

How?

Since this component isn't used in many places, the main goal was to match the design specs so we could move forward with updating to the 40px sizing. So there could be potential follow-ups to reduce some redundancy currently present, but I don't believe it is a priority due to its current usage. Therefore, the main changes are:

• changing flex-direction to column
• adding a RangeControl component to each side
• adjusting where/when the icons are shown
• grouping the buttons together (reset and linked)

Screenshots

AllInputControl

Before	After

InputControls

Before	After

AxialInputControls

Will look similar but with two inputs when unlinked and the icons highlighting two sides (vertical and horizontal)

Testing Instructions

Ensure tests pass and component behaves the same as before.

For new changes:

Ensure that RangeControl updates to the correct value when typing in the input.

[brookewp]

There are a couple things that I'd like to run by design (cc @jameskoster) before finishing up this PR and marking it ready for review.

Buttons

The placement of the linked button doesn't work as well with the vertical inputs, especially if allowReset is false. So, I grouped both buttons in the same area as the icon in SpacingInputControl:

And this is how it looks if allowReset is false:

Before	After

When the Reset button is present, should it also have the 40px height when the inputs do?

Input Size

The input is a bit bigger than the one in SpacingInputControl. This is due to the missing settings icon. In lieu of that, do we want to:

Keep it as it is now, with the UnitControl and RangeControl as equal sizes?

Or add a max size of 90px to the UnitControl to match the sizing of SpacingInputControl? Or maybe a compact size option for this?

---

And if there's any other feedback, please share! One thing to note is the Axial Controls haven't been updated with the changes yet so those will look strange right now.

[jameskoster]

The placement of the link button seems sensible 👍

When the Reset button is present, should it also have the 40px height when the inputs do?

I think the current size is okay, and consistent with other reset buttons.

For the input size, a max-width is probably a good idea as it's unlikely to be populated with more than 3 characters. Using the compact variant could make sense too (while retaining the 40px row height), to avoid the inputs butting up against each other.

What do you think?

[brookewp]

What do you think?

That looks great, thank you for the info and example! I initially didn't add spacing between the inputs to match the current SpacingInputControl, but I prefer what you've shared instead.

[ciampo]

Overall things are looking good, although I spotted two aspects that may need improving:

1. When the slider has its maximum value, both the thumb and the tooltip (shown when the thumb is focused) render partially outside of the component's boundary. This may be problematic in the sidebar, as they would either cause a horizontal scrollbar to appear, or they could be potentially cut off.

2. The sliders have a fixed max value of 100, which, depending on the selected unit, may be too little or too much. We should take inspiration from SpacingSizesControl and adapt max and step values per unit (see code)

[brookewp]

When the slider has its maximum value, both the thumb and the tooltip (shown when the thumb is focused) render partially outside of the component's boundary. This may be problematic in the sidebar, as they would either cause a horizontal scrollbar to appear, or they could be potentially cut off.

Good find! SpacingSizesControl has a settings icon to the right, so that problem wasn't there. But I'll look at other use cases of RangeControl to be consistent with those.

The sliders have a fixed max value of 100, which, depending on the selected unit, may be too little or too much. We should take inspiration from SpacingSizesControl and adapt max and step values per unit (see code)

Thank you for sharing that example! I was initially concerned about using RangeControl because of the max prop limitation since BoxControl doesn't have a max limit. But if we're happy to use the unit/max/step pairing from SpacingSizesControl then that works for me!

[ciampo]

I'll look at other use cases of RangeControl to be consistent with those.

I see that you added an 8px right margin, which mitigates the issue but doesn't necessarily solve it entirely

Adding more right margin would feel awkward, though. I wonder if the component would still look alright in the editor, since the sidebar should have some padding?

[brookewp]

I wonder if the component would still look alright in the editor, since the sidebar should have some padding?

As you suspected, the thumb isn't cut off because of the sidebar's padding. I think the small right margin helps the slider track edge align a bit more with the ellipsis menu:

With Margin	Without

[brookewp]

I based this on a similar example I saw from @andrewhayward:

 <label id="lbl-2" for="ipt-2">Label 2</label>
  <input id="ipt-2">
  <input id="ipt-3" aria-labelledby="lbl-2" aria-controls="ipt-2">

BoxControl has very different markup, so it isn't implemented the same. In this instance, it may be completely wrong, so I would appreciate a more expert eye on this. However, this did get rid of the redundant label on the slider, while avoiding a violation for having no label.

Some additional context:

When unlinking the sides, the unit control labels are labelled by aria-labels. So I added aria-controls and aria-labelledby with the intention to connect it to those inputs.

In AllInputControl when it's a single/mixed set of values, there isn't an explicit label, but it inherits one from the BaseControl.VisualLabel:

gutenberg/packages/components/src/box-control/index.tsx

Lines 157 to 165 in 3192ad7

	<Grid
	id={ id }
	columns={ 3 }
	role="group"
	aria-labelledby={ headingId }
	>
	<BaseControl.VisualLabel id={ headingId }>
	{ label }
	</BaseControl.VisualLabel>

[brookewp]

The labels have been updated to be more clear, as discussed: #56665 (comment)

Context of the change:

All of the changes in 25cf3f6 will just be naming updates, except for the highlighted portion above. The change here was adding an additional assertion to still check for the input, but with the new name. Before, the label for the component (group role) was also the label of the first input, but only when the sides were linked. Therefore the assertion would pass if the query was screen.getByRole( 'group', { name: 'Box Control' } or screen.getByRole( 'textbox', { name: 'Box Control' }. Which is why the query could be updated to group from textbox with no additional changes needed. The original labelling could be confusing when using a screenreader, with just the one input having a different labelling scheme and it being so generic.

Tl;dr, what has been changed:

So instead, now, the label implemented by the consumer for the purpose of the component will only be applied to the group. And each input is labelled, so the linked input will now be labelled as 'All sides' or 'Mixed sides'. And unlinked will be 'Top side', 'Left side', 'Left and Right sides', etc.

[mirka]

Great work wrangling all the feedback! Looks good to go once the tests are updated 👍