phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

RectangularRadioButtonGroup does not respect touchAreaDilation options #856

Open samreid opened 11 months ago

samreid commented 11 months ago

Probably related to https://github.com/phetsims/sun/issues/851. In https://github.com/phetsims/center-and-variability/issues/462, I was asked to dilate the pointer areas for the radio button groups, but the option is not being respected.

Probably being overwritten in https://github.com/phetsims/sun/blob/56a5f02524cba687a127894294e9724ce813ee41/js/buttons/RectangularButton.ts#L180-L205.

So this may have been broken in https://github.com/phetsims/sun/commit/21fb64f5d9707bd1f2aa0b950abb15269286b88d as part of https://github.com/phetsims/sun/issues/48.

samreid commented 11 months ago

I have a potential workaround to pass through the pointer dilations to the children, but it would likely clash with https://github.com/phetsims/sun/commit/fff91dbfe3fc4156b1da88f9fbe02a955a384125 and https://github.com/phetsims/sun/issues/708

```diff Subject: [PATCH] Expand touch areas, see https://github.com/phetsims/center-and-variability/issues/462 --- Index: js/buttons/RectangularRadioButtonGroup.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/buttons/RectangularRadioButtonGroup.ts b/js/buttons/RectangularRadioButtonGroup.ts --- a/js/buttons/RectangularRadioButtonGroup.ts (revision 036005add1db1fe57725e4e98dd469d1a8d97e35) +++ b/js/buttons/RectangularRadioButtonGroup.ts (date 1692742579427) @@ -221,6 +221,12 @@ if ( item.voicingContextResponse ) { radioButtonOptions.voicingContextResponse = item.voicingContextResponse; } + if ( options.touchAreaXDilation ) { + radioButtonOptions.touchAreaXDilation = options.touchAreaXDilation; + } + if ( options.touchAreaYDilation ) { + radioButtonOptions.touchAreaYDilation = options.touchAreaYDilation; + } const radioButton = new RectangularRadioButton( property, item.value, radioButtonOptions ); ```
jonathanolson commented 11 months ago

This might be good to pair on sometime.

https://github.com/phetsims/sun/blob/08180ca3e5118d72953fdf9602ce8e4fd577c481/js/buttons/RectangularRadioButtonGroup.ts#L283-L295 looks like it should be changed, presumably it should be creating the buttons with the proper touch areas.

samreid commented 11 months ago

What if we comment out the code in https://github.com/phetsims/sun/issues/856#issuecomment-1689172913 and apply the patch in https://github.com/phetsims/sun/issues/856#issuecomment-1689001399 ? it works well when the buttons are the same size. When the buttons are different sizes, are you saying we would compute a different dilation for each so they will have the same touch or mouse areas on startup? But they could change size at runtime based on dynamic strings.

Would it be OK if 2 radio buttons with different sizes have different touch areas? That would simplify things if it is OK.

pixelzoom commented 11 months ago

Rather than every sim having to dilate pointer areas for the buttons in RectangularRadioButtonGroup, it might be nice if the pointer areas were auto-dilated to fill the space between the buttons. This would be consistent with AquaRadioButtonGroup and CheckboxGroup, and would make sims consistent.

samreid commented 11 months ago

In https://github.com/phetsims/center-and-variability/issues/462 it was decided that this is not blocking for CaV. Self-unassigning, but I would recommend review at the next iteration planning in case we want to schedule it.

samreid commented 5 months ago

Please see notes from @pixelzoom in https://github.com/phetsims/projectile-data-lab/issues/187#issuecomment-1971714982 and below

marlitas commented 5 months ago

Dev Meeting 3/7/24

jonathanolson commented 4 months ago

@samreid I believe this is resolved with the above commit, can you confirm?

samreid commented 4 months ago

To test the change, I temporarily added a large y-dilation to the sample size radio buttons in the 4th screen of Projectile Data Lab like so:


Subject: [PATCH] Fix a spelling error, see https://github.com/phetsims/faradays-electromagnetic-lab/issues/2
---
Index: js/sampling/view/SampleSizeSection.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/sampling/view/SampleSizeSection.ts b/js/sampling/view/SampleSizeSection.ts
--- a/js/sampling/view/SampleSizeSection.ts (revision 5d387795957446a92d9d439511035cfd0f6d6a51)
+++ b/js/sampling/view/SampleSizeSection.ts (date 1710452709788)
@@ -69,7 +69,8 @@
       },
       layoutOptions: {
         align: 'center'
-      }
+      },
+      touchAreaYDilation: 20
     } );
     super( ProjectileDataLabStrings.sampleSizeNStringProperty, sampleSizeRadioButtonGroup, providedOptions );
   }

The result worked very nicely:

image
samreid commented 4 months ago

I confirmed the behavior is good, and the first commit https://github.com/phetsims/sun/commit/f80ffeb560bf5510bd10b95ad010d845c497975d is great. However, I don't feel like I can evaluate the second commit https://github.com/phetsims/sun/commit/ce5d9302f7997d9d7c4964d0d44a385e34e8574a. It overlaps with work done by @jessegreenberg in: https://github.com/phetsims/sun/commit/fff91dbfe3fc4156b1da88f9fbe02a955a384125

@jessegreenberg are you available to complete this review?