ImageSizeControls: Replace ButtonGroup with ToggleGroupControl

[hbhalodia]

What?

PR replaces the ButtonGroup component with ToggleGroupControl in the ImageSizeControls.

Why?

• Part Of - Replace ButtonGroup usage with ToggleGroupControl #65339

Testing Instructions

1. Open any post or page.
2. Add latest post block.
3. Enable option to display featured image.
4. Check for the image size controls.
5. Should work as expected as before.

Testing Instructions for Keyboard

• Same

Screenshots or screencast

Before	After

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: hbhalodia <hbhalodia@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[ciampo]

As discussed here, isDeselectable doesn't really work with text options — we should remove it.

One option could be to add an "Auto" option which replaces the 'reset' button. What do you think, @mirka @t-hamano ?

[t-hamano]

If we remove the isDeselectable prop, keyboard users won't be able to set a custom value:

71dcd41756d30d74f869d014456c5284.mp4

As I commented here too, we might need to add an option called "Custom".

[mirka]

keyboard users won't be able to set a custom value

This isn't inherently true — it's only because the logic of addSelectedScaleValue doesn't account for it. In fact the current logic sets invalid values on the ToggleGroupControl, when it should be set back to undefined instead. (cc @hbhalodia)

For the sake of this PR, I'm ok with just replicating the existing functionality. And as it turns out, the Reset button doesn't actually reset, it just sets it to 100%, which is different from an actual reset. So to actually reset, the user needs to clear the width/height input fields. I think that means we don't even need a Reset button nor a "Custom" option to replicate the existing functionality.

[hbhalodia]

Hi @mirka, If I had read it correct, need to update the logic on addSelectedScaleValue() function should return undefined, when we set custom values? is that something need to changed.

Thank You,

[hbhalodia]

it's only because the logic of addSelectedScaleValue doesn't account for it. In fact the current logic sets invalid values on the ToggleGroupControl, when it should be set back to undefined instead.

Hi @mirka, I tried to set directly undefined on that logic, seems like anyhow keyboard user would ultimately land on the first option of the ToggleGroupControl, which ultimately sets the first option even we have undefined set on that logic.

Can you please let me know how to set it undefined for keyboard users on that logic, because ultimately when user lands on the toggleGroupControl it would call handleUpdateDimension() which ultimately sets the dimension based on the scale size, and it would remove the custom dimension set.

[t-hamano]

After investigating this issue, I found that when the ToggleGroupControl component is focused, onChange fires and the value that actually exists in the options is referenced. This may be one of the causes of the unintended behavior.

I thought that the side effects of the useDimensionHandler hook might be affecting it, but deleting useEffect did not solve the problem. Therefore, I still don't know what is causing the unintended onChange event 🤔

Please refer to the verification video below:

51d9a31d054528b23dc9fdd3fdcb3559.mp4

[mirka]

I looked into this, and I think it's a bug in ToggleGroupControl 😭 Sorry. Just a moment while I address this at the root.

[mirka]

Fix proposed in #66151

[hbhalodia]

Thanks, @mirka for getting the bug fixed.

[mirka]

Fix is merged. Thank you for your patience!

[t-hamano]

I think we need to follow the original logic and check that not only the width but also the height matches.

We should be able to determine this as follows:

const selectedValue = IMAGE_SIZE_PRESETS.find( ( scale ) => {
	const scaledWidth = Math.round( imageWidth * ( scale / 100 ) );
	const scaledHeight = Math.round( imageHeight * ( scale / 100 ) );
	return currentWidth === scaledWidth && currentHeight === scaledHeight;
} );

[hbhalodia]

Thanks @t-hamano, For the updated logic, I would add this logic and would wait for ToggleGroupControl bug to be fixed 🤞.

[hbhalodia]

Update: The logic has been updated and waiting for the ToggleGroupControl fix to be land.

Thank You,