RectangularRadioButton problems related to appearance strategies

[pixelzoom]

A couple of TypeScript problems in RectangularRadioButton.ts:

• FlatAppearanceStrategyOptions and ContentAppearanceStrategyOptions are missing, and any is used.

• SelfOptions duplicates a bunch of definitons that should be in FlatAppearanceStrategyOptions, ContentAppearanceStrategyOptions

• These comment in SelfOptions are troubling, I don't know how someone would "add your own options". And these should be nested options.

  // Options used by RectangularRadioButton.FlatAppearanceStrategy.
  // If you define your own buttonAppearanceStrategy, then you may need to add your own options.
...
  // Options used by RectangularRadioButton.ContentAppearanceStrategy.
  // If you define your own contentAppearanceStrategy, then you may need to add your own options.

Assigning to @jbphet since he's the architect of appearance strategies.

[pixelzoom]

Note that those options that should be in FlatAppearanceStrategyOptions and ContentAppearanceStrategyOptions are also duplicated in RectangularRadioButtonGroup. Discovered while cleaning up RectangularRadioButtonGroup in https://github.com/phetsims/sun/issues/740. So this will also need to be addressed in RectangularRadioButtonGroup.

[pixelzoom]

This was noted yesterday in Slack, but came up again in a design meeting today. Adding block-publication to this issue because radio button appearance is currently broken.

Stroke on radio buttons is missing, so you can't tell which radio button is selected. I'm not sure if there are other problems. Some examples...

Equality Explorer:

MOTHA:

[jessegreenberg]

After pulling today I also noticed that button strokes have gotten a little thicker Master:

Published:

And rectangular radio buttons might be missing an opacity or filter on deselected buttons: Master:

Published:

[pixelzoom]

Raising the priority of this. This has been broken for way too long. @jbphet can you please take a look? Or if it's been fixed, can you please move this issue forward?

[pixelzoom]

@Nancy-Salpepi reported this problem in Slack#dev-public, which seems related:

Hi. I was testing Gravity and Orbits and I noticed that when I hover over different scenes an outline now appears around them that isn’t present in the published version or older dev versions. Was this an intentional change? Here is an example with the earth/moon scene on projector mode:

Edit to link this to https://github.com/phetsims/qa/issues/838, which this was found in.

[jbphet]

My apologies for dropping the ball on this, and thanks for raising the priority. That will get it back on my radar.

Back in late July of this year, I converted the options for the appearance strategies into their own type and nested them. This solved the TypeScript typing problems and allowed us to get rid of the related ts-ignore and any directives, but it introduced some behavioral problems, as shown above. I addressed most of those, but I also noticed some subtle differences with regular buttons versus their published counterparts, and I was having a hard time figuring the root cause of the difference. I put the issue on the back burner to address other priorities, and just never got back to it. I'll see if I can finish it off in the next couple of days, at least to the point where it is ready for review.

[jbphet]

I've investigated this, and found that some of the changes I made when porting this to TypeScript removed some default behaviors that were needed. I've put those back, and verified the behavior on several sims (see list below), and they seem to be behaving correctly now.

For reference, the sims I checked were molecules-and-light, equality-explorer, gravity-and-orbits, models-of-the-hydrogen-atom (which isn't published, so I just validated that it looked reasonable), number-line-integers, area-model-algebra, and bending-light.

I'd like to review the current code with at least one other developer. The appearance strategy uses the values for the deselected case for the over case in a number of places, which strikes me as a little odd, but many of the sims are relying on this behavior, so perhaps we just leave it.

[jbphet]

@pixelzoom and I discussed this code, and decided to add some commentary and otherwise leave it as is, since it works reasonably well and sims seem to be relying on it. I've added the docs. Closing.