RectangularRadioButtonGroup problem with pointer areas

[pixelzoom]

Reported in https://github.com/phetsims/natural-selection/issues/284.

There is a general problem with pointer areas in RectangularRadioButtonGroup. selectedLineWidth and deselectedLineWidth can be used to set the width of the stroke around the buttons. When these values are significantly different, the button that is initially selected will have significantly different point areas than the other buttons.

For example, if I make this change to equality-explorer/SceneRadioButtonGroup, giving it an extreme selectedLineWidth:

Index: js/common/view/SceneRadioButtonGroup.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/SceneRadioButtonGroup.js b/js/common/view/SceneRadioButtonGroup.js
--- a/js/common/view/SceneRadioButtonGroup.js   (revision ee63a96c2fd604c169db6551271c8f156033c961)
+++ b/js/common/view/SceneRadioButtonGroup.js   (date 1625161283296)
@@ -26,7 +26,8 @@
       baseColor: 'white',
       spacing: 8,
       buttonContentXMargin: 15,
-      buttonContentYMargin: 12
+      buttonContentYMargin: 12,
+      selectedLineWidth: 15
     }, options );

     // describe a radio button for each scene

... then the initial selection will look like this:

... and selecting one of the other buttons will look like this. Note that the button stroke is outside the pointer areas.

The problem is that the pointer areas for each button are set individually, based on initial value of the Property that is associated with the group. The pointer areas should be set uniformly, using the maximum size of the buttons in the group, and therefore accounting for strokes.

[pixelzoom]

It looks like there was an attempt to do the right thing in RectangularRadioButtonGroup, but it's not working:

// Set pointer areas. Extra width is added to the radio buttons so they don't change size if the line width
// changes. That is why lineWidth is subtracted from the width and height when calculating these new areas.
radioButton.touchArea = Shape.rectangle(

[pixelzoom]

In the above commit, pointer areas now take the lineWidth of the stroke into account, and are sized to accommodate the largest of options.selectedLineWidth and options.deselectedLineWidth.

For my Equality Explorer example above, the initial selection now looks like this:

... and selecting one of the other buttons looks like this:

I tested a few other sims that use RectangularRadioButtonGroup, and I didn't notice any regressions. Until this issue is reviewed, it will be labeled "blocks-publication". @zepumph randomly selected to review:

_.sample( [ 'sr', 'mk', 'jb', 'jo', 'ck', 'jg' ] ) "mk"

[zepumph]

Looks right to me! Thanks.

[jessegreenberg]

I noticed a CT issue caused by this change. When a radio button item has a label, the focus highlight was using the calculated mouse area to determine its size, but since that calculation was moved it is no longer available. I am seeing this now on CT:

RectangularRadioButtonGroup.js:263 Uncaught TypeError: Cannot read property 'dilated' of undefined

Also, this change made it so that radio button labels are no longer clickable. The list of "buttons" in this iteration https://github.com/phetsims/sun/blob/721f8f2300457fddd2296d8fe81af44c9f167572/js/buttons/RectangularRadioButtonGroup.js#L286 can include a LayoutBox if one of the items of the RadioButtonGroup has a label.

The mouse area, touch area, and focus highlights all need to be set on the RectangularRadioButton, is there a way we can do that while still calculating the max button width and button height?

[pixelzoom]

I've reverted the changes. It's unclear to me why radio buttons in a group should have labels, or who added this feature. This implementation has become way too complicated. Someone with knowledge of the a11y issues will need to address this issue.

Labeling for dev meeting to assign to someone.

[jessegreenberg]

To be clear, I am referring to the graphical label which has been a feature of RectangularRadioButtonGroup since cd51b22c96f0527abf349039c9656c59b866ccdc in 2014 and is demonstrated in the sun demo.

[pixelzoom]

Thanks for clarifying, and for the pointer to the sun demo.

I'm not clear on where (or whether) the label feature is actually used in sims -- it's difficult to search for "label:". I'm obviously not familiar with the focus highlight implementation or requirements, since I broke it. And it's not at all clear to me what pointer areas should look like when we have labels, or a mishmash of labels and no labels. Should each button+label have the same effective size and thus the same uniform pointer-area sizes?.... The implementation could also be improved by using AlignBox to give things identical effective sizes, instead of adding invisible rectangles, etc.

Anyway... I was happy to take this on when it looked straightforward. But now that I see the complexity, I'm not interested, and I don't have time.

[jessegreenberg]

The setting of the focus highlight was also fragile, would have been pretty easy to break it.

I worked on a fix in the above commit. The implementation is mostly the same as https://github.com/phetsims/sun/commit/a9fc44a2a2d9cf12b0ff6044581619b6d11dd1b5, but we keep a reference to the RectangularRadioButton to set mouse/touch areas and focus highlight on it. To make it explicit I added an inner class ButtonWithLayoutNode to RectangularRadioButtonGroup. @pixelzoom could you please review?

[pixelzoom]

Looks good to me. Thanks for taking this on @jessegreenberg! Closing.