NumberSpinner description disturbed by conversion to StringProperty

[arouinfar]

Discovered during review of the Layer Model Screen description in Greenhouse Effect, https://github.com/phetsims/greenhouse-effect/issues/374 in VoiceOver.

Typically, focus on a NumberSpinner should result in a description like, "{{value}}, {{accessible name}}, {{role/type of component}}" which is what things currently look like in the published version of Gravity Force Lab: Basics.

However, on main, instead of "vertical number spinner" VoiceOver says something like "vertical Property#123{number spinner}" as seen in Greenhouse Effect and Gravity Force Lab: Basics on main:

@jbphet hypothesizes that this is related to the conversion of strings to StringProperty, so assigning to him to investigate.

[jbphet]

I was able to correct the behavior with the commit shown above. In essence, the value of the string Property is now being used as the value in setPDOMAttribute instead of the Property itself. A few things seem odd with this. First, the code to set this attribute was changed to be a Property over a year ago in da5b264db84970c1b0fa6ca1c4dacbc0614e3d48. It's hard for me to believe that it has been broken since then with no one noticing, so I'm wondering if some of the code it depends on has changed. Second, I think our intention has been to start using Property values in this situation in order to ultimately support dynamic properties, so it feels odd to have to use a string. And finally, why didn't type checking catch this?

@pixelzoom and @jessegreenberg - Can you each please take a look at this change and see if it makes sense and isn't indicative of some larger problem? @pixelzoom made the change to a Property, and @jessegreenberg is the lead author of ParallelDOM, so you seem like the people to ask.

[pixelzoom]

... First, the code to set this attribute was changed to be a Property over a year ago in https://github.com/phetsims/sun/commit/da5b264db84970c1b0fa6ca1c4dacbc0614e3d48 ... @pixelzoom made the change to a Property, and @jessegreenberg is the lead author of ParallelDOM, so you seem like the people to ask.

The change that I made was done as part of a larger conversion of sun components to support dynamic local by using StringProperty instead of static strings. I vaguely recall consulting @jessegreenberg about whether PDOM supported StringProperty, the answer was yes, and that is confirmed by the fact that setPDOMAttribute second parameter is value: PDOMValueType | boolean | number, where type PDOMValueType = string | TReadOnlyProperty<string>. So a StringProperty is a valid arg to setPDOMAttribute.

That said.... The change that I made to AccessibleNumberSpinner was:

-     thisNode.setPDOMAttribute( 'aria-roledescription', numberSpinnerRoleDescriptionString );
+     thisNode.setPDOMAttribute( 'aria-roledescription', SunStrings.a11y.numberSpinnerRoleDescriptionStringProperty

@jbphet's change in https://github.com/phetsims/sun/commit/cba546f1cd3d7025471a1ff7a1551d3c366b10d5 is:

-     thisNode.setPDOMAttribute( 'aria-roledescription', SunStrings.a11y.numberSpinnerRoleDescriptionStringProperty );
+     thisNode.setPDOMAttribute( 'aria-roledescription', SunStrings.a11y.numberSpinnerRoleDescriptionStringProperty.value );

... so passing in numberSpinnerRoleDescriptionStringProperty.value instead of numberSpinnerRoleDescriptionStringProperty.

That change should not be necessary, because setPDOMAttribute supports TReadOnlyProperty<string> for its second arg. And it defeats the point of making strings dynamic. So I recommend reverting https://github.com/phetsims/sun/commit/cba546f1cd3d7025471a1ff7a1551d3c366b10d5, and consulting with @jessegreenberg on what is likely a deeper problem.

[jessegreenberg]

Thanks for finding and investigating this @jbphet - Scenery should be able to take the Property, I made https://github.com/phetsims/scenery/issues/1598 to find out why that didn't work. When fixed, Ill revert the change in this issue.

[jessegreenberg]

This should be fixed in scenery now after https://github.com/phetsims/scenery/commit/118bf973511f1c1c369dab6145a58056627ffe8d so I reverted https://github.com/phetsims/sun/commit/c26042604907d469973f91dc4c9d7c15495e2ea7.

I tested with NVDA, and it still sounds correct. @jbphet would you like to review/confirm before closing?

[jbphet]

I tested @jessegreenberg's changes by verifying that the number spinners are properly described in Greenhouse Effect and Gravity Force Lab: Basics, and they are. I look over the changes, and they seem reasonable, though I don't know the PDOM code well enough to do a very thorough review. All in all, I think we're good here. Closing.