Display.focusProperty is not properly instrumented

[zepumph]

There is a conditional when creating Display.focusProperty that doesn't account for sims that are enabling a11y by passing an option into Sim

    // Only instrument if accessibility is enabled
    ( window.phet && phet.chipper && phet.chipper.accessibility ) ? {

We need to find a way around this. Right now we have to add a11y to BASE in order to instrument this property.

[zepumph]

The question to answer is:

(How) can we instrument a static property based on a flag that is only set based on instances? Perhaps we need more of a global. Since by the Sim instance is created, the Display's focusProperty has already resloved into a non-instrumented instance.

[jessegreenberg]

I encountered this in https://github.com/phetsims/phet-io-wrapper-sonification/issues/73 where the instrumentation of the Display.focusProperty wasn't happening when accessibility was enabled, it was only happening when the ?a11y query parameter was added.

[jessegreenberg]

I am assuming that IO instrumentation must occur at instantiation of the Property, is that correct?

I am not seeing any great places to indicate accessibility is enabled in a more global way. But I am also not very familiar with how sims start up.

Can we make Display.focusProperty not static?

[zepumph]

I am not seeing any great places to indicate accessibility is enabled in a more global way. But I am also not very familiar with how sims start up.

For phet-io, we are lucky because it is a brand. That is set before loading any of the requirejs modules. With a11y it seems harder. Perhaps we can brainstorm more sometime.

Can we make Display.focusProperty not static?

I don't want to rule it out, but it seems like it boxes SCENERY/Display into a corner. We likely only want to support a single focus for the whole frame, not just divided on display instances.

[zepumph]

I ran into this again in correlation with recent solidification and validation of phet-io api (through the baselines file). It became impossible to run the sonification wrapper unless a11y was provided as a query parameter. In the above commit I did a bit of scary hackary to get things working. I ended up defining the instrumented focusProperty in the constructor, assigning it as a static there conditionally based on options.accessibility. I also defined it in the section of the statics, with (hopefully) adequate doc for maintainability. The main piece of grossness is the fact that any linking to the first instance of the focusProperty could get wiped away by the second.

What I would have preferred to do would probably be to set Display.focusProperty = null at the bottom of the file with the statics, and only set it in the constructor (if not already defined). The problem I ran into is that AccessiblePeer was calling it before the Display's constructor is called.

@jessegreenberg please review. This be better, I just know it. How? Tagging @samreid in case he can think of anything.

[samreid]

Would it work to always create and instrument the Focus?

Index: js/display/Display.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/display/Display.js	(revision ab99d0f39503c29f91f7fc244de93e54b412a782)
+++ js/display/Display.js	(date 1557196777000)
@@ -273,21 +273,21 @@
     // global reference if we have a Display (useful)
     this.scenery = scenery;
 
-    if ( this.options.accessibility ) {
-
-      // If the display supports accessibility, we want to overwrite the focusProperty, instead instrumenting it with
-      // PhET-iO, see https://github.com/phetsims/scenery/issues/936
-      // NOTE: Do not move this below FocusOverlay's creation below.
-      Display.focusProperty = new Property( null, {
+    // If the display supports accessibility, we want to overwrite the focusProperty, instead instrumenting it with
+    // PhET-iO, see https://github.com/phetsims/scenery/issues/936
+    // NOTE: Do not move this below FocusOverlay's creation below.
+    Display.focusProperty = new Property( null, {
 
-        // Make this a static tandem so that it can be added to PhET-iO Studio correctly (batched and then flushed when the
-        // listener is added).
-        tandem: Tandem.generalTandem.createTandem( 'focusProperty' ),
-        phetioType: PropertyIO( NullableIO( FocusIO ) ),
-        phetioState: false,
-        phetioReadOnly: true
-      } );
+      // Make this a static tandem so that it can be added to PhET-iO Studio correctly (batched and then flushed when the
+      // listener is added).
+      tandem: Tandem.generalTandem.createTandem( 'focusProperty' ),
+      phetioType: PropertyIO( NullableIO( FocusIO ) ),
+      phetioState: false,
+      phetioReadOnly: true
+    } );
 
+    if ( this.options.accessibility ) {
+
       if ( this.options.isApplication ) {
         this._domElement.setAttribute( 'aria-role', 'application' );
       }
@@ -1762,6 +1762,9 @@
      */
     getFocusedNode: function() {
       var focusedNode = null;
+      if ( !this.focusProperty ) {
+        return null;
+      }
       var focus = this.focusProperty.get();
       if ( focus ) {
         focusedNode = focus.trail.lastNode();
@@ -1800,7 +1803,7 @@
   // NOTE: DO NOT LINK TO THIS!!! It is potentially overridden in Display's constructor
   // to support PhET-iO Instrumentation.  see https://github.com/phetsims/scenery/issues/936
   // FocusOverlay links to this as an exception, but only after it is overwritten, see Display's constructor.
-  Display.focusProperty = new Property( null );
+  // Display.focusProperty = new Property( null );
 
   // @public {Emitter} - Fires when we detect an input event that would be considered a "user gesture" by Chrome, so
   // that we can trigger browser actions that are only allowed as a result.

And can you please provide links for sonification wrappers that should work with this?

[jessegreenberg]

I would be OK with always instrumenting focusProperty, as long as we make it clear that this is specific to keyboard/a11y focus (not pointer input). If always instrumented then it could be moved out of the constructor.

Alternatively, maybe we should reconsider whether focusProperty needs to be static. I think our argument was "A document can only have one element with focus so multiple Displays in a single document should share a single focus too". But it seems reasonable to me for each instance of Display to have its own focus.

[zepumph]

@samreid here is the link to the main sonification launchpoint.

http://localhost/phet-io-wrapper-sonification/sonification.html

I was using the first GFLB basics link last night with some alterations. First I built GFLB locally for phet-io, and then I went to gravity-force-lab-basics-sonification.html and changed the phet-io.js script import to point to the local version.

But it seems reasonable to me for each instance of Display to have its own focus.

That makes sense. Does your work on zooming inform this at all? For example, if there was a strategy we may be pursuing where we were to add a smaller display on top of the sim as a "magnifier" display. That sort of solution may benefit from having display be static, so they bother could respond to the same Node (I say this with a big shrug over here). On the other side, I can see a usage where we would want to maintain the current focus for a display, so that we wouldn't look track of it when alternating between two display (even though only one of them would have focus at the same time). Which solution are you leaning towards?

1. static focus that is always instrumented
2. one Property per Display instance, in which case we can only instrument it if we enable a11y for the Display.

[jessegreenberg]

The case you describe is possible, but hasn't been seriously considered yet. I think it is much more likely we will continue with more "standard" types of web zoom and magnify. Either way, I think that could be implemented with on Property per Display instance too because the focused node would be attached to both displays.

I think I am leaning toward one Property per Display instance, but I really don't feel strongly one way or the other.

[samreid]

one Property per Display instance, in which case we can only instrument it if we enable a11y for the Display.

What is the problem with the focusProperty being always PhET-iO instrumented, even if a11y is not enabled?

[zepumph]

What is the problem with the focusProperty being always PhET-iO instrumented, even if a11y is not enabled?

I don't think there is a problem with it, I perhaps just think it is slightly more unclean because it provides a useless item to most phet-io sims (since the majority don't have a11y enabled). Recognizing from https://github.com/phetsims/phet-io/issues/1457 that conditional instrumentation only serves to complicate phet-io, I think I'm leaning towards always instrumenting the Property, and updating the documentation to be clear that this is only useful for alternative input.

[zepumph]

I made the focusProperty always instrumented. Note that it is a bit of a pain to need to compute all of the baselines for small changes like this. It takes a few minutes, even with the small number of sims we currently have. Perhaps we could have a for-each.sh -p to run them all in parallel.

@samreid please review.

[pixelzoom]

af9fe77 broke beers-law-lab in CT:

beers-law-lab : phet-io-fuzz : require.js : load
Query: brand=phet-io&phetioStandalone&ea&phetioValidateTandems&fuzz&memoryLimit=1000
Uncaught Error: Assertion failed: api mismatches present:

beersLawLab.general.focusProperty:  2. Registered PhetioObject baseline must equal baseline schema to ensure that baseline changes are intentional.
Error: Assertion failed: api mismatches present:

beersLawLab.general.focusProperty:  2. Registered PhetioObject baseline must equal baseline schema to ensure that baseline changes are intentional.
    at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/assert/js/assert.js:22:13)
    at PhetioAPIValidation.assertOutIfErrorsPresent (https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/tandem/js/phetioAPIValidation.js?bust=1557288296029:169:9)
    at PhetioAPIValidation.onSimStarted (https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/tandem/js/phetioAPIValidation.js?bust=1557288296029:139:14)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/phet-io/js/phetioEngine.js?bust=1557288296029:125:29
    at TinyEmitter.emit (https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/axon/js/TinyEmitter.js?bust=1557288296029:58:55)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/axon/js/Emitter.js?bust=1557288296029:46:31
    at Emitter.execute (https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/axon/js/Action.js?bust=1557288296029:177:20)
    at Emitter.emit (https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/axon/js/Emitter.js?bust=1557288296029:61:20)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/joist/js/Sim.js?bust=1557288296029:826:50
id: Bayes Chrome
Approximately 5/7/2019, 9:15:38 PM
beers-law-lab : phet-io-fuzz : require.js : run
Query: brand=phet-io&phetioStandalone&ea&phetioValidateTandems&fuzz&memoryLimit=1000
Uncaught Error: Assertion failed: api mismatches present:

beersLawLab.general.focusProperty:  2. Registered PhetioObject baseline must equal baseline schema to ensure that baseline changes are intentional.
Error: Assertion failed: api mismatches present:

beersLawLab.general.focusProperty:  2. Registered PhetioObject baseline must equal baseline schema to ensure that baseline changes are intentional.
    at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/assert/js/assert.js:22:13)
    at PhetioAPIValidation.assertOutIfErrorsPresent (https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/tandem/js/phetioAPIValidation.js?bust=1557288296029:169:9)
    at PhetioAPIValidation.onSimStarted (https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/tandem/js/phetioAPIValidation.js?bust=1557288296029:139:14)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/phet-io/js/phetioEngine.js?bust=1557288296029:125:29
    at TinyEmitter.emit (https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/axon/js/TinyEmitter.js?bust=1557288296029:58:55)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/axon/js/Emitter.js?bust=1557288296029:46:31
    at Emitter.execute (https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/axon/js/Action.js?bust=1557288296029:177:20)
    at Emitter.emit (https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/axon/js/Emitter.js?bust=1557288296029:61:20)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1557285338764/joist/js/Sim.js?bust=1557288296029:826:50
id: Bayes Chrome
Approximately 5/7/2019, 9:15:38 PM

[zepumph]

Sorry @pixelzoom, fixes pushed above. @samreid please review.

[samreid]

This seems the right direction to go in. I fixed a typo and updated formatting. Also, I noted this tandem:

Tandem.generalTandem.createTandem( 'focusProperty' )

Two points:

• I did not see this in the studio tree when launching graphing quadratics. Is that as expected?
• I think we will need to add structure under the general tandem. Perhaps a model/view dichotomy? Doing that now would futureproof changes to that part. Thoughts?

[zepumph]

I did not see this in the studio tree when launching graphing quadratics. Is that as expected?

I do see it there, do you now? It is not featured if that is helpful.

I think we will need to add structure under the general tandem. Perhaps a model/view dichotomy? Doing that now would futureproof changes to that part. Thoughts?

That makes a lot of sense, and sounds futureproof. Sorry I only am seeing this now. Do you think it is still worth it after GQ has been published?

[samreid]

I see it under "all", thanks! We should schedule time to look at all the elements under general.* and think of how we want to organize them (possibly model|view|controller). Let's leave focusProperty alone until then. If that sounds good, perhaps close this issue and open a new one?

[zepumph]

I think that will happen as part of phetsims/tandem#108. Closing