Hide focus highlights but keep DOM focus in response to pointer interaction

For several reasons, we need to change how focus highlights work. Currently, focus is lost in response to mouse/touch input because the active element gets blurred. It needs to change so that DOM focus persists, but the focus highlight becomes invisible.
This is related to https://github.com/phetsims/scenery/issues/1094 and comes directly from suggestions in https://github.com/phetsims/sun/issues/668.

This will involve changes to FocusOverlay to support making the highlights invisible but still active and joist so that on down events the focus highlight can disappear.  The question is then when focus highlights should become visible again. Easiest thing is probably to make it visible again on focus events.

This will involve changes to FocusOverlay to support making the highlights invisible but still active and joist so that on down events the focus highlight can disappear.  The question is then when focus highlights should become visible again. Easiest thing is probably to make it visible again on focus events.

The behavior needs to be a bit smarter than this. After tabbing to a button then clicking on the screen, the focus highlight dissappears but the button still has focus, and so pressing enter or spacebar will still fire the button. The button also has its own styling from the interaction state from still having DOM focus.
Behvior should be more like this, which matches Chrome:

If down event goes down on a Node that is focusable, focus goes on that element.
If down event goes down on a Node that is not focusable, focused element is blurred.

The behavior needs to be a bit smarter than this. After tabbing to a button then clicking on the screen, the focus highlight dissappears but the button still has focus, and so pressing enter or spacebar will still fire the button. The button also has its own styling from the interaction state from still having DOM focus.
Behvior should be more like this, which matches Chrome:

If down event goes down on a Node that is focusable, focus goes on that element.
If down event goes down on a Node that is not focusable, focused element is blurred.

The above snippet is working pretty well with iOS 13 VoiceOver.

The above snippet is working pretty well with iOS 13 VoiceOver.

I just tested this change on a slider and I am not hearing the aria-valuetext on iOS VoiceOver, which was one of the motivations for this change.  We should test that double-tap and hold interaction to move sliders reads aria-valuetext in another context.

I just tested this change on a slider and I am not hearing the aria-valuetext on iOS VoiceOver, which was one of the motivations for this change.  We should test that double-tap and hold interaction to move sliders reads aria-valuetext in another context.

I just tried this HTML and did not hear either the value or the aria-valutext on a slider when double-tap and dragging the element

<!DOCTYPE html>

  <meta charset="UTF-8">
  <title>title</title>

  <input type="range" id="slider">

Will report this in the related issue where this was initially requested.

I just tried this HTML and did not hear either the value or the aria-valutext on a slider when double-tap and dragging the element

<!DOCTYPE html>

  <meta charset="UTF-8">
  <title>title</title>

  <input type="range" id="slider">

Will report this in the related issue where this was initially requested.

Changes made above, @zepumph would you mind reviewing this? Are there any problems you can think of with this change?

Changes made above, @zepumph would you mind reviewing this? Are there any problems you can think of with this change?

Reviewed with @zepumph - we think this change is OK and gets us events working correctly in iOS 14 VoiceOver. While discussing we noticed that the focus highlight remained visible with mouse interaction in Friction. @jessegreenberg will look into that.

Reviewed with @zepumph - we think this change is OK and gets us events working correctly in iOS 14 VoiceOver. While discussing we noticed that the focus highlight remained visible with mouse interaction in Friction. @jessegreenberg will look into that.

We think this should block publication of anything that supports iOS and has a grab drag interaction.

We think this should block publication of anything that supports iOS and has a grab drag interaction.

The bug mentioned in https://github.com/phetsims/scenery/issues/1137#issuecomment-759106109 was caused by focusHighlightLayerable. Fixed in the above commit. I tested many sims and ran ?fuzz and ?fuzzBoard tests. Closing.

The bug mentioned in https://github.com/phetsims/scenery/issues/1137#issuecomment-759106109 was caused by focusHighlightLayerable. Fixed in the above commit. I tested many sims and ran ?fuzz and ?fuzzBoard tests. Closing.

Reopening, we would like to change this behavior again. Since iOS 14 VO uses pointer events for some elements, we are losing keyboard focus or not moving focus correctly because events are not flagged with isFromPDOM. Specifically, in ComboBox opening on iOS VoiceOver does not move focus into the ComboBoxListBox.
We are going to try changing so that focus highlights become invisible whenever there is pointer input, and visible again only when there is keyboard input. Currently it becomes visible again whenever there is a focus change. If it only becomes visible with keyboard interaction we could set focus regardless of input in most usages, which would fix this behavior and also improve code.

Reopening, we would like to change this behavior again. Since iOS 14 VO uses pointer events for some elements, we are losing keyboard focus or not moving focus correctly because events are not flagged with isFromPDOM. Specifically, in ComboBox opening on iOS VoiceOver does not move focus into the ComboBoxListBox.
We are going to try changing so that focus highlights become invisible whenever there is pointer input, and visible again only when there is keyboard input. Currently it becomes visible again whenever there is a focus change. If it only becomes visible with keyboard interaction we could set focus regardless of input in most usages, which would fix this behavior and also improve code.

Here is initial pass at the new behavior, a note for myself to come back to soon.

      // focus highlights are initially invisible
      this.display.setFocusHighlightVisible( false );

      this.display.addInputListener( {
        down: event => {

          // in the self-voicing prototype we want the focus highlight to remain visible with
          // mouse/touch presses
          if ( !phet.chipper.queryParameters.supportsSelfVoicing ) {

            // An AT might have sent a down event outside of the display, if this happened we will not do anything
            // to change focus
            if ( this.display.bounds.containsPoint( event.pointer.point ) ) {

              // no need to do this work unless some element in the simulation has focus
              if ( Display.focusedNode ) {

                // in response to pointer events, always hide the focus highlight so it isn't distracting
                this.display.setFocusHighlightVisible( false );

                // if the event trail doesn't include the focusedNode, clear it - otherwise DOM focus is kept on the
                // active element so that it can remain the target for assistive devices using pointer events
                // on behalf of the user, see https://github.com/phetsims/scenery/issues/1137
                if ( !event.trail.nodes.includes( Display.focusedNode ) ) {
                  Display.focus = null;
                }
              }
            }
          }
        }
      } );

      // for any key press, focus highlights become visible again -
      globalKeyStateTracker.keydownEmitter.addListener( domEvent => {
        this.display.setFocusHighlightVisible( true );
      } );
    }

Will want to inspect usages of SceneryEvent.isFromPDOM() as likely most can be removed.

Here is initial pass at the new behavior, a note for myself to come back to soon.

      // focus highlights are initially invisible
      this.display.setFocusHighlightVisible( false );

      this.display.addInputListener( {
        down: event => {

          // in the self-voicing prototype we want the focus highlight to remain visible with
          // mouse/touch presses
          if ( !phet.chipper.queryParameters.supportsSelfVoicing ) {

            // An AT might have sent a down event outside of the display, if this happened we will not do anything
            // to change focus
            if ( this.display.bounds.containsPoint( event.pointer.point ) ) {

              // no need to do this work unless some element in the simulation has focus
              if ( Display.focusedNode ) {

                // in response to pointer events, always hide the focus highlight so it isn't distracting
                this.display.setFocusHighlightVisible( false );

                // if the event trail doesn't include the focusedNode, clear it - otherwise DOM focus is kept on the
                // active element so that it can remain the target for assistive devices using pointer events
                // on behalf of the user, see https://github.com/phetsims/scenery/issues/1137
                if ( !event.trail.nodes.includes( Display.focusedNode ) ) {
                  Display.focus = null;
                }
              }
            }
          }
        }
      } );

      // for any key press, focus highlights become visible again -
      globalKeyStateTracker.keydownEmitter.addListener( domEvent => {
        this.display.setFocusHighlightVisible( true );
      } );
    }

Will want to inspect usages of SceneryEvent.isFromPDOM() as likely most can be removed.

I haven't tested this, but just for discussion, I would have thought that we would want the following behavior:
Instead of:
      // for any key press, focus highlights become visible again -
      globalKeyStateTracker.keydownEmitter.addListener( domEvent => {
        this.display.setFocusHighlightVisible( true );
      } );

We would need something like:

      const setToTrue = () => this.display.setFocusHighlightVisible( true );
      const setFocusHighlightVisibleOnPDOMInputListener = {};

     // For every PDOM event, when we receive it, make sure that we have visible focus highlights whenever events come from the PDOM.
      Input.PDOM_EVENT_TYPES.forEach( eventType => {
        setFocusHighlightVisibleOnPDOMInputListener[ eventType ] = setToTrue;
      } );
      this.display.addInputListener( setFocusHighlightVisibleOnPDOMInputListener );

I haven't tested this, but just for discussion, I would have thought that we would want the following behavior:
Instead of:
      // for any key press, focus highlights become visible again -
      globalKeyStateTracker.keydownEmitter.addListener( domEvent => {
        this.display.setFocusHighlightVisible( true );
      } );

We would need something like:

      const setToTrue = () => this.display.setFocusHighlightVisible( true );
      const setFocusHighlightVisibleOnPDOMInputListener = {};

     // For every PDOM event, when we receive it, make sure that we have visible focus highlights whenever events come from the PDOM.
      Input.PDOM_EVENT_TYPES.forEach( eventType => {
        setFocusHighlightVisibleOnPDOMInputListener[ eventType ] = setToTrue;
      } );
      this.display.addInputListener( setFocusHighlightVisibleOnPDOMInputListener );

@zepumph and I worked on this today and got far in getting a more refined version of the above proposals working.
Here is a patch covering many files:

Index: molecules-and-light/js/moleculesandlight/view/MoleculesAndLightScreenView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- molecules-and-light/js/moleculesandlight/view/MoleculesAndLightScreenView.js	(revision adeedb195b7f2b72a30b691bc49b7783aafc2a80)
+++ molecules-and-light/js/moleculesandlight/view/MoleculesAndLightScreenView.js	(date<PHONE_NUMBER>827)
@@ -193,11 +193,7 @@
       listener: () => {
         const dialog = lightSpectrumDialogCapsule.getElement();
         dialog.show();
-
-        // if listener was fired because of accessibility
-        if ( showLightSpectrumButton.isPDOMClicking() ) {
-          dialog.focusCloseButton();
-        }
+        dialog.focusCloseButton();
       },
       tandem: tandem.createTandem( 'showLightSpectrumButton' ),

Index: sun/js/MenuItem.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/MenuItem.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/MenuItem.js	(date<PHONE_NUMBER>191)
@@ -71,7 +71,7 @@
       // @param {SceneryEvent} - Only called after PDOM interaction and called AFTER closeCallback, use this to move
       // focus to a particular Node in the document. By default focus is moved to the top of the document after a
       // MenuItem action since the PhET Menu closes after activation
-      handleFocusCallback: event => {
+      handleFocusCallback: () => {

         // limit search of next focusable to root accessible HTML element
         const rootElement = phet.joist.display.accessibleDOMElement;
@@ -118,11 +118,7 @@
         closeCallback( event );
         callback( event );

-        // send focus to a custom spot, but focus should only be placed if the fire event
-        // came from the PDOM
-        if ( event.isFromPDOM() ) {
-          options.handleFocusCallback( event );
-        }
+        options.handleFocusCallback( event );
       }
     } ) );

Index: sun/js/ComboBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/ComboBox.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/ComboBox.js	(date<PHONE_NUMBER>655)
@@ -212,16 +212,7 @@
     // Clicking on the button toggles visibility of the list box
     this.button.addListener( () => {
       this.listBox.visibleProperty.value = !this.listBox.visibleProperty.value;
-    } );
-
-    //TODO sun#462 integrate this with above button listener, to eliminate order dependency
-    // Handle button clicks, for a11y
-    this.button.addInputListener( {
-      click: () => {
-        if ( this.listBox.visible ) {
-          this.listBox.focus();
-        }
-      }
+      this.listBox.visibleProperty.value && this.listBox.focus();
     } );

     // @private the display that clickToDismissListener is added to, because the scene may change, see sun#14
Index: scenery/js/display/Display.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/display/Display.js	(revision 9c6549efc6ce9b9affc1df835e13f6983b87ede6)
+++ scenery/js/display/Display.js	(date<PHONE_NUMBER>804)
@@ -330,10 +330,6 @@

     if ( this._accessible ) {

-      // make the PDOM invisible in the browser - it has some width and is shifted off screen so that AT can read the
-      // formatting tags, see https://github.com/phetsims/scenery/issues/730
-      // SceneryStyle.addRule( '.accessibility, .accessibility * { position: relative; left: -1000px; top: 0; width: 250px; height: 0; clip: rect(0,0,0,0); pointerEvents: none }' );
-
       // @private {Node}
       this._focusRootNode = new Node();

@@ -812,13 +808,13 @@

   /**
    * Get the root accessible DOM element which represents this display and provides semantics for assistive
-   * technology.
+   * technology. If this Display is not accessible, returns null.
    * @public
    *
-   * @returns {HTMLElement}
+   * @returns {HTMLElement|null}
    */
   getAccessibleDOMElement() {
-    return this._rootAccessibleInstance.peer.primarySibling;
+    return this._accessible ? this._rootAccessibleInstance.peer.primarySibling : null;
   }

   get accessibleDOMElement() { return this.getAccessibleDOMElement(); }
Index: scenery-phet/js/accessibility/GrabDragInteraction.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery-phet/js/accessibility/GrabDragInteraction.js	(revision 363ba0f72eb6f95109a7829250a417ea07e1608f)
+++ scenery-phet/js/accessibility/GrabDragInteraction.js	(date<PHONE_NUMBER>698)
@@ -397,13 +397,13 @@
     // @private - from non-PDOM pointer events, change representations in the PDOM - necessary for accessible tech that
     // uses pointer events like iOS VoiceOver. The above listeners manage input from the PDOM.
     this.pressListener = new PressListener( {
-      press: ( event, listener ) => {
+      press: event => {
         if ( !event.isFromPDOM() ) {
           this.turnToDraggable();
           this.onGrab();
         }
       },
-      release: ( event, listener ) => {
+      release: event => {

         // release if PressListener is interrupted, but only if not already
         // grabbable, which is possible if the GrabDragInteraction has been
Index: joist/js/Sim.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- joist/js/Sim.js	(revision 0253af291fed06ba3ccf1a5462dd5ca74edd78b5)
+++ joist/js/Sim.js	(date<PHONE_NUMBER>261)
@@ -28,7 +28,9 @@
 import platform from '../../phet-core/js/platform.js';
 import StringUtils from '../../phetcommon/js/util/StringUtils.js';
 import BarrierRectangle from '../../scenery-phet/js/BarrierRectangle.js';
+import globalKeyStateTracker from '../../scenery/js/accessibility/globalKeyStateTracker.js';
 import KeyboardFuzzer from '../../scenery/js/accessibility/KeyboardFuzzer.js';
+import KeyboardUtils from '../../scenery/js/accessibility/KeyboardUtils.js';
 import webSpeaker from '../../scenery/js/accessibility/speaker/webSpeaker.js';
 import Display from '../../scenery/js/display/Display.js';
 import InputFuzzer from '../../scenery/js/input/InputFuzzer.js';
@@ -609,35 +611,49 @@
       // NOTE: When translatable this will need to update with language, change to phet.chipper.local
       this.display.accessibleDOMElement.lang = 'en';

-      // If a down event is received we either remove DOM focus or make focus highlights invisible, depending
-      // on the target of the event. If the event trail contains the currently focused Node, focus highlights
-      // are made invisible so that they do not distract for sighted users. DOM focus is kept on the active element so
-      // that it can still remain a target for assistive devices that use pointer events on behalf of the user.
-      // See https://github.com/phetsims/scenery/issues/1137
       this.display.addInputListener( {
         down: event => {

-          // in the self-voicing prototype we want the focus highlight to remain with
+          // in the self-voicing prototype we want the focus highlight to remain visible with
           // mouse/touch presses
           if ( !phet.chipper.queryParameters.supportsSelfVoicing ) {

-            // An AT might have sent a down event outside of the display, if this happened we will not remove focus.
+            // An AT might have sent a down event outside of the display, if this happened we will not do anything
+            // to change focus
             if ( this.display.bounds.containsPoint( event.pointer.point ) ) {

+              // in response to pointer events, always hide the focus highlight so it isn't distracting
+              this.display.setFocusHighlightVisible( false );
+
               // no need to do this work unless some element in the simulation has focus
               if ( Display.focusedNode ) {
-                if ( event.trail.nodes.includes( Display.focusedNode ) ) {
-                  this.display.setFocusHighlightVisible( false );
-                }
-                else {
+
+                // if the event trail doesn't include the focusedNode, clear it - otherwise DOM focus is kept on the
+                // active element so that it can remain the target for assistive devices using pointer events
+                // on behalf of the user, see https://github.com/phetsims/scenery/issues/1137
+                if ( !event.trail.nodes.includes( Display.focusedNode ) ) {
                   Display.focus = null;
                 }
               }
             }
           }
-        },
-        focus: event => {
-          this.display.setFocusHighlightVisible( true );
+        }
+      } );
+
+      // restore display of focus highlights if we receive PDOM events, but don't for focus so that we can
+      const eventsForFocus = [ 'click', 'input', 'change', 'keydown', 'keyup' ];
+      const setHighlightsVisible = () => this.display.setFocusHighlightVisible( true );
+      const focusHighlightVisibleListener = {};
+      eventsForFocus.forEach( eventType => {
+        focusHighlightVisibleListener[ eventType ] = setHighlightsVisible;
+      } );
+      this.display.addInputListener( focusHighlightVisibleListener );
+
+      // when tabbing into the sim, make focus highlights visible - on keyup and on a listener on
+      // the body because the first tab into the sim will not have an event target under the PDOM
+      globalKeyStateTracker.keyupEmitter.addListener( event => {
+        if ( event.keyCode === KeyboardUtils.KEY_TAB ) {
+          setHighlightsVisible();
         }
       } );
     }
Index: scenery/js/overlays/FocusOverlay.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/overlays/FocusOverlay.js	(revision 9c6549efc6ce9b9affc1df835e13f6983b87ede6)
+++ scenery/js/overlays/FocusOverlay.js	(date<PHONE_NUMBER>206)
@@ -24,6 +24,7 @@
 let outerGroupHighlightColor = FocusHighlightPath.OUTER_LIGHT_GROUP_FOCUS_COLOR;

 class FocusOverlay {
+
   /**
    * @param {Display} display
    * @param {Node} focusRootNode - the root node of our display
Index: sun/js/Dialog.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/Dialog.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/Dialog.js	(date<PHONE_NUMBER>444)
@@ -439,7 +439,7 @@
   // Since many Dialogs are dynamic elements, these need to be in the state. The value of the state object doesn't
   // matter, but it instead just serves as a marker to tell the state engine to recreate the Dialog (if dynamic) when
   // setting state.
-  supertype: ReferenceIO(Node.NodeIO)
+  supertype: ReferenceIO( Node.NodeIO )
 } );

 sun.register( 'Dialog', Dialog );
Index: joist/js/PhetButton.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- joist/js/PhetButton.js	(revision 0253af291fed06ba3ccf1a5462dd5ca74edd78b5)
+++ joist/js/PhetButton.js	(date<PHONE_NUMBER>681)
@@ -88,6 +88,7 @@
       highlightCenterOffsetY: 4,
       listener: () => {
         phetMenu.show();
+        phetMenu.items[ 0 ].focus();
         pushButtonSoundPlayer.play();
       },
       phetioType: PhetButton.PhetButtonIO,
@@ -124,20 +125,6 @@
     // No need to be removed because the PhetButton exists for the lifetime of the sim.
     this.buttonModel.enabledProperty.link( enabled => { menuIcon.visible = enabled; } );

-    // pdom - add a listener that opens the menu on 'click' and 'reset', and closes it on escape and if the
-    // button receives focus again
-    this.addInputListener( {
-      click: () => {
-
-        // open and set focus on the first item
-        phetMenu.show();
-        phetMenu.items[ 0 ].focus();
-
-        // sound generation
-        pushButtonSoundPlayer.play();
-      }
-    } );
-
     // pdom - add an attribute that lets the user know the button opens a menu
     AriaHasPopUpMutator.mutateNode( this, true );
   }
Index: scenery/js/listeners/PressListener.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/listeners/PressListener.js	(revision 9c6549efc6ce9b9affc1df835e13f6983b87ede6)
+++ scenery/js/listeners/PressListener.js	(date<PHONE_NUMBER>036)
@@ -543,6 +543,7 @@
     }

     this.isOverProperty.value = this.isFocusedProperty.value || ( this.overPointers.length > 0 && !pointerAttachedToOther );
+    this.looksOverProperty = ( this.isFocusedProperty.value && focusHighlights.areVisibleProperty.get() ) || ( this.overPointers.length > 0 && !pointerAttachedToOther );
   }

   /**
Index: joist/js/PhetMenu.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- joist/js/PhetMenu.js	(revision 0253af291fed06ba3ccf1a5462dd5ca74edd78b5)
+++ joist/js/PhetMenu.js	(date<PHONE_NUMBER>092)
@@ -255,9 +255,7 @@
           visiblePropertyOptions: { phetioFeatured: true },

           // pdom
-          handleFocusCallback: () => {
-            this.restoreFocus();
-          }
+          handleFocusCallback: () => this.restoreFocus()
         }
       },

Index: molecules-and-light/js/moleculesandlight/view/ObservationWindow.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- molecules-and-light/js/moleculesandlight/view/ObservationWindow.js	(revision adeedb195b7f2b72a30b691bc49b7783aafc2a80)
+++ molecules-and-light/js/moleculesandlight/view/ObservationWindow.js	(date<PHONE_NUMBER>614)
@@ -125,7 +125,7 @@
         // pdom
         // move focus to the emission control slider only when the button is clicked
         // retain focus on other elements if button was clicked without focus
-        this.returnMoleculeButtonNode.isFocused() && photonEmitterNode.button.focus();
+        photonEmitterNode.button.focus();

         photonAbsorptionModel.restoreActiveMolecule();
         this.returnMoleculeButtonVisibleProperty.set( false );
Index: sun/js/ComboBoxListBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/ComboBoxListBox.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/ComboBoxListBox.js	(date<PHONE_NUMBER>666)
@@ -76,6 +76,8 @@

       // prevent nodes (eg, controls) behind the list from receiving the event
       event.abort();
+
+      focusButtonCallback();
     }, {
       parameters: [ { phetioPrivate: true, valueType: SceneryEvent } ],

@@ -97,14 +99,12 @@
       keyup: event => {
         if ( KeyboardUtils.KEY_ENTER === event.domEvent.keyCode || KeyboardUtils.KEY_SPACE === event.domEvent.keyCode ) {
           fireAction.execute( event );
-          focusButtonCallback();
         }
       },

       // handle activation from an assistive device that may not use a keyboard (such as mobile VoiceOver)
       click: event => {
         fireAction.execute( event );
-        focusButtonCallback();
       }
     };

To summarize: Focus highlights are made invisible on down events. If the down Event Trail does not include the focused Node, focus is removed from the document. Otherwise it remains for in sim for screen reader functionality.  Focus highlights become visible again on most DOM events other than focus. This way we can always call focus() in sim code so that the virtual cursor moves with DOM focus, while the focus highlights stay invisible and don't distract sighted users.
The big problem is that many UI components update visually with the "over" interaction state, and they are considered "over" with "focus". For example, when focus moves from a ComboBox to a ComboBoxListItemNode after pressing the ComboBoxButton, the item node will look highlighted. This wasn't too distracted when we reviewed in RaP, but when I checked in other sims with brighter highlightFill colors the change was pretty noticeable. We found other cases of this too with joist buttons. I didn't feel comfortable commiting this change without reviewing with other team members.
Before this though, I would like to play with suppressing the 'over' look with focus when focus highlights are invisible.

@zepumph and I worked on this today and got far in getting a more refined version of the above proposals working.
Here is a patch covering many files:

Index: molecules-and-light/js/moleculesandlight/view/MoleculesAndLightScreenView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- molecules-and-light/js/moleculesandlight/view/MoleculesAndLightScreenView.js	(revision adeedb195b7f2b72a30b691bc49b7783aafc2a80)
+++ molecules-and-light/js/moleculesandlight/view/MoleculesAndLightScreenView.js	(date<PHONE_NUMBER>827)
@@ -193,11 +193,7 @@
       listener: () => {
         const dialog = lightSpectrumDialogCapsule.getElement();
         dialog.show();
-
-        // if listener was fired because of accessibility
-        if ( showLightSpectrumButton.isPDOMClicking() ) {
-          dialog.focusCloseButton();
-        }
+        dialog.focusCloseButton();
       },
       tandem: tandem.createTandem( 'showLightSpectrumButton' ),

Index: sun/js/MenuItem.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/MenuItem.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/MenuItem.js	(date<PHONE_NUMBER>191)
@@ -71,7 +71,7 @@
       // @param {SceneryEvent} - Only called after PDOM interaction and called AFTER closeCallback, use this to move
       // focus to a particular Node in the document. By default focus is moved to the top of the document after a
       // MenuItem action since the PhET Menu closes after activation
-      handleFocusCallback: event => {
+      handleFocusCallback: () => {

         // limit search of next focusable to root accessible HTML element
         const rootElement = phet.joist.display.accessibleDOMElement;
@@ -118,11 +118,7 @@
         closeCallback( event );
         callback( event );

-        // send focus to a custom spot, but focus should only be placed if the fire event
-        // came from the PDOM
-        if ( event.isFromPDOM() ) {
-          options.handleFocusCallback( event );
-        }
+        options.handleFocusCallback( event );
       }
     } ) );

Index: sun/js/ComboBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/ComboBox.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/ComboBox.js	(date<PHONE_NUMBER>655)
@@ -212,16 +212,7 @@
     // Clicking on the button toggles visibility of the list box
     this.button.addListener( () => {
       this.listBox.visibleProperty.value = !this.listBox.visibleProperty.value;
-    } );
-
-    //TODO sun#462 integrate this with above button listener, to eliminate order dependency
-    // Handle button clicks, for a11y
-    this.button.addInputListener( {
-      click: () => {
-        if ( this.listBox.visible ) {
-          this.listBox.focus();
-        }
-      }
+      this.listBox.visibleProperty.value && this.listBox.focus();
     } );

     // @private the display that clickToDismissListener is added to, because the scene may change, see sun#14
Index: scenery/js/display/Display.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/display/Display.js	(revision 9c6549efc6ce9b9affc1df835e13f6983b87ede6)
+++ scenery/js/display/Display.js	(date<PHONE_NUMBER>804)
@@ -330,10 +330,6 @@

     if ( this._accessible ) {

-      // make the PDOM invisible in the browser - it has some width and is shifted off screen so that AT can read the
-      // formatting tags, see https://github.com/phetsims/scenery/issues/730
-      // SceneryStyle.addRule( '.accessibility, .accessibility * { position: relative; left: -1000px; top: 0; width: 250px; height: 0; clip: rect(0,0,0,0); pointerEvents: none }' );
-
       // @private {Node}
       this._focusRootNode = new Node();

@@ -812,13 +808,13 @@

   /**
    * Get the root accessible DOM element which represents this display and provides semantics for assistive
-   * technology.
+   * technology. If this Display is not accessible, returns null.
    * @public
    *
-   * @returns {HTMLElement}
+   * @returns {HTMLElement|null}
    */
   getAccessibleDOMElement() {
-    return this._rootAccessibleInstance.peer.primarySibling;
+    return this._accessible ? this._rootAccessibleInstance.peer.primarySibling : null;
   }

   get accessibleDOMElement() { return this.getAccessibleDOMElement(); }
Index: scenery-phet/js/accessibility/GrabDragInteraction.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery-phet/js/accessibility/GrabDragInteraction.js	(revision 363ba0f72eb6f95109a7829250a417ea07e1608f)
+++ scenery-phet/js/accessibility/GrabDragInteraction.js	(date<PHONE_NUMBER>698)
@@ -397,13 +397,13 @@
     // @private - from non-PDOM pointer events, change representations in the PDOM - necessary for accessible tech that
     // uses pointer events like iOS VoiceOver. The above listeners manage input from the PDOM.
     this.pressListener = new PressListener( {
-      press: ( event, listener ) => {
+      press: event => {
         if ( !event.isFromPDOM() ) {
           this.turnToDraggable();
           this.onGrab();
         }
       },
-      release: ( event, listener ) => {
+      release: event => {

         // release if PressListener is interrupted, but only if not already
         // grabbable, which is possible if the GrabDragInteraction has been
Index: joist/js/Sim.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- joist/js/Sim.js	(revision 0253af291fed06ba3ccf1a5462dd5ca74edd78b5)
+++ joist/js/Sim.js	(date<PHONE_NUMBER>261)
@@ -28,7 +28,9 @@
 import platform from '../../phet-core/js/platform.js';
 import StringUtils from '../../phetcommon/js/util/StringUtils.js';
 import BarrierRectangle from '../../scenery-phet/js/BarrierRectangle.js';
+import globalKeyStateTracker from '../../scenery/js/accessibility/globalKeyStateTracker.js';
 import KeyboardFuzzer from '../../scenery/js/accessibility/KeyboardFuzzer.js';
+import KeyboardUtils from '../../scenery/js/accessibility/KeyboardUtils.js';
 import webSpeaker from '../../scenery/js/accessibility/speaker/webSpeaker.js';
 import Display from '../../scenery/js/display/Display.js';
 import InputFuzzer from '../../scenery/js/input/InputFuzzer.js';
@@ -609,35 +611,49 @@
       // NOTE: When translatable this will need to update with language, change to phet.chipper.local
       this.display.accessibleDOMElement.lang = 'en';

-      // If a down event is received we either remove DOM focus or make focus highlights invisible, depending
-      // on the target of the event. If the event trail contains the currently focused Node, focus highlights
-      // are made invisible so that they do not distract for sighted users. DOM focus is kept on the active element so
-      // that it can still remain a target for assistive devices that use pointer events on behalf of the user.
-      // See https://github.com/phetsims/scenery/issues/1137
       this.display.addInputListener( {
         down: event => {

-          // in the self-voicing prototype we want the focus highlight to remain with
+          // in the self-voicing prototype we want the focus highlight to remain visible with
           // mouse/touch presses
           if ( !phet.chipper.queryParameters.supportsSelfVoicing ) {

-            // An AT might have sent a down event outside of the display, if this happened we will not remove focus.
+            // An AT might have sent a down event outside of the display, if this happened we will not do anything
+            // to change focus
             if ( this.display.bounds.containsPoint( event.pointer.point ) ) {

+              // in response to pointer events, always hide the focus highlight so it isn't distracting
+              this.display.setFocusHighlightVisible( false );
+
               // no need to do this work unless some element in the simulation has focus
               if ( Display.focusedNode ) {
-                if ( event.trail.nodes.includes( Display.focusedNode ) ) {
-                  this.display.setFocusHighlightVisible( false );
-                }
-                else {
+
+                // if the event trail doesn't include the focusedNode, clear it - otherwise DOM focus is kept on the
+                // active element so that it can remain the target for assistive devices using pointer events
+                // on behalf of the user, see https://github.com/phetsims/scenery/issues/1137
+                if ( !event.trail.nodes.includes( Display.focusedNode ) ) {
                   Display.focus = null;
                 }
               }
             }
           }
-        },
-        focus: event => {
-          this.display.setFocusHighlightVisible( true );
+        }
+      } );
+
+      // restore display of focus highlights if we receive PDOM events, but don't for focus so that we can
+      const eventsForFocus = [ 'click', 'input', 'change', 'keydown', 'keyup' ];
+      const setHighlightsVisible = () => this.display.setFocusHighlightVisible( true );
+      const focusHighlightVisibleListener = {};
+      eventsForFocus.forEach( eventType => {
+        focusHighlightVisibleListener[ eventType ] = setHighlightsVisible;
+      } );
+      this.display.addInputListener( focusHighlightVisibleListener );
+
+      // when tabbing into the sim, make focus highlights visible - on keyup and on a listener on
+      // the body because the first tab into the sim will not have an event target under the PDOM
+      globalKeyStateTracker.keyupEmitter.addListener( event => {
+        if ( event.keyCode === KeyboardUtils.KEY_TAB ) {
+          setHighlightsVisible();
         }
       } );
     }
Index: scenery/js/overlays/FocusOverlay.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/overlays/FocusOverlay.js	(revision 9c6549efc6ce9b9affc1df835e13f6983b87ede6)
+++ scenery/js/overlays/FocusOverlay.js	(date<PHONE_NUMBER>206)
@@ -24,6 +24,7 @@
 let outerGroupHighlightColor = FocusHighlightPath.OUTER_LIGHT_GROUP_FOCUS_COLOR;

 class FocusOverlay {
+
   /**
    * @param {Display} display
    * @param {Node} focusRootNode - the root node of our display
Index: sun/js/Dialog.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/Dialog.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/Dialog.js	(date<PHONE_NUMBER>444)
@@ -439,7 +439,7 @@
   // Since many Dialogs are dynamic elements, these need to be in the state. The value of the state object doesn't
   // matter, but it instead just serves as a marker to tell the state engine to recreate the Dialog (if dynamic) when
   // setting state.
-  supertype: ReferenceIO(Node.NodeIO)
+  supertype: ReferenceIO( Node.NodeIO )
 } );

 sun.register( 'Dialog', Dialog );
Index: joist/js/PhetButton.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- joist/js/PhetButton.js	(revision 0253af291fed06ba3ccf1a5462dd5ca74edd78b5)
+++ joist/js/PhetButton.js	(date<PHONE_NUMBER>681)
@@ -88,6 +88,7 @@
       highlightCenterOffsetY: 4,
       listener: () => {
         phetMenu.show();
+        phetMenu.items[ 0 ].focus();
         pushButtonSoundPlayer.play();
       },
       phetioType: PhetButton.PhetButtonIO,
@@ -124,20 +125,6 @@
     // No need to be removed because the PhetButton exists for the lifetime of the sim.
     this.buttonModel.enabledProperty.link( enabled => { menuIcon.visible = enabled; } );

-    // pdom - add a listener that opens the menu on 'click' and 'reset', and closes it on escape and if the
-    // button receives focus again
-    this.addInputListener( {
-      click: () => {
-
-        // open and set focus on the first item
-        phetMenu.show();
-        phetMenu.items[ 0 ].focus();
-
-        // sound generation
-        pushButtonSoundPlayer.play();
-      }
-    } );
-
     // pdom - add an attribute that lets the user know the button opens a menu
     AriaHasPopUpMutator.mutateNode( this, true );
   }
Index: scenery/js/listeners/PressListener.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/listeners/PressListener.js	(revision 9c6549efc6ce9b9affc1df835e13f6983b87ede6)
+++ scenery/js/listeners/PressListener.js	(date<PHONE_NUMBER>036)
@@ -543,6 +543,7 @@
     }

     this.isOverProperty.value = this.isFocusedProperty.value || ( this.overPointers.length > 0 && !pointerAttachedToOther );
+    this.looksOverProperty = ( this.isFocusedProperty.value && focusHighlights.areVisibleProperty.get() ) || ( this.overPointers.length > 0 && !pointerAttachedToOther );
   }

   /**
Index: joist/js/PhetMenu.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- joist/js/PhetMenu.js	(revision 0253af291fed06ba3ccf1a5462dd5ca74edd78b5)
+++ joist/js/PhetMenu.js	(date<PHONE_NUMBER>092)
@@ -255,9 +255,7 @@
           visiblePropertyOptions: { phetioFeatured: true },

           // pdom
-          handleFocusCallback: () => {
-            this.restoreFocus();
-          }
+          handleFocusCallback: () => this.restoreFocus()
         }
       },

Index: molecules-and-light/js/moleculesandlight/view/ObservationWindow.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- molecules-and-light/js/moleculesandlight/view/ObservationWindow.js	(revision adeedb195b7f2b72a30b691bc49b7783aafc2a80)
+++ molecules-and-light/js/moleculesandlight/view/ObservationWindow.js	(date<PHONE_NUMBER>614)
@@ -125,7 +125,7 @@
         // pdom
         // move focus to the emission control slider only when the button is clicked
         // retain focus on other elements if button was clicked without focus
-        this.returnMoleculeButtonNode.isFocused() && photonEmitterNode.button.focus();
+        photonEmitterNode.button.focus();

         photonAbsorptionModel.restoreActiveMolecule();
         this.returnMoleculeButtonVisibleProperty.set( false );
Index: sun/js/ComboBoxListBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/ComboBoxListBox.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/ComboBoxListBox.js	(date<PHONE_NUMBER>666)
@@ -76,6 +76,8 @@

       // prevent nodes (eg, controls) behind the list from receiving the event
       event.abort();
+
+      focusButtonCallback();
     }, {
       parameters: [ { phetioPrivate: true, valueType: SceneryEvent } ],

@@ -97,14 +99,12 @@
       keyup: event => {
         if ( KeyboardUtils.KEY_ENTER === event.domEvent.keyCode || KeyboardUtils.KEY_SPACE === event.domEvent.keyCode ) {
           fireAction.execute( event );
-          focusButtonCallback();
         }
       },

       // handle activation from an assistive device that may not use a keyboard (such as mobile VoiceOver)
       click: event => {
         fireAction.execute( event );
-        focusButtonCallback();
       }
     };

To summarize: Focus highlights are made invisible on down events. If the down Event Trail does not include the focused Node, focus is removed from the document. Otherwise it remains for in sim for screen reader functionality.  Focus highlights become visible again on most DOM events other than focus. This way we can always call focus() in sim code so that the virtual cursor moves with DOM focus, while the focus highlights stay invisible and don't distract sighted users.
The big problem is that many UI components update visually with the "over" interaction state, and they are considered "over" with "focus". For example, when focus moves from a ComboBox to a ComboBoxListItemNode after pressing the ComboBoxButton, the item node will look highlighted. This wasn't too distracted when we reviewed in RaP, but when I checked in other sims with brighter highlightFill colors the change was pretty noticeable. We found other cases of this too with joist buttons. I didn't feel comfortable commiting this change without reviewing with other team members.
Before this though, I would like to play with suppressing the 'over' look with focus when focus highlights are invisible.

suppressing the 'over' look with focus when focus highlights are invisible.

The way I propose to do this is to

Add a static focusHighlightsVisibleProperty to Display (or somewhere global), rather than an instance variable on FocusOverlay.
Add a looksOverProperty to PressListener. It is true if there is focus and Display.focusHighlightsVisibleProperty is true OR there are any over pointers.
Add a looksOverProperty to ButtonModel which gets updated like looksPressedProperty.
Change the various interaction state Properties like MomentaryButtonInteractionStateProperty  to be over with looksOverProperty rather than overProperty.

This is working well so far, but would be great to discuss with @zepumph and @jonathanolson since this involves some larger changes.
Here is the patch with this as a proof of concept:

Index: sun/js/buttons/ButtonModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/buttons/ButtonModel.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/buttons/ButtonModel.js	(date<PHONE_NUMBER>708)
@@ -57,6 +57,8 @@
     // See PressListener.click for more details.
     this.looksPressedProperty = new BooleanProperty( false );

+    this.looksOverProperty = new BooleanProperty( false );
+
     // @public (read-only by users, read-write in subclasses) - emitter that is fired when sound should be produced
     this.produceSoundEmitter = new Emitter();

@@ -71,6 +73,8 @@
     // first listener is created. See this.createPressListener.
     this.looksPressedMultilink = null;

+    this.looksOverMultilink = null;
+
     // startCallback on pointer down, endCallback on pointer up. lazyLink so they aren't called immediately.
     this.downProperty.lazyLink( down => {
       if ( down ) {
@@ -101,6 +105,7 @@
       this.produceSoundEmitter.dispose();

       this.looksPressedMultilink && this.looksPressedMultilink.dispose();
+      this.looksOverMultilink && this.looksOverMultilink.dispose();

       this.listeners = [];
     };
@@ -143,10 +148,13 @@

     // dispose the previous multilink in case we already created a PressListener with this model
     this.looksPressedMultilink && this.looksPressedMultilink.dispose();
+    this.looksOverMultilink && this.looksOverMultilink.dispose();

     const looksPressedProperties = this.listeners.map( function( listener ) { return listener.looksPressedProperty; } );
     looksPressedProperties.push( this.downProperty );

+    const looksOverProperties = this.listeners.map( listener => { return listener.looksOverProperty; } );
+
     // assign a new Multilink (for disposal), and make sure that the button looks pressed when any of the
     // PressListeners created by this ButtonModel look pressed. Note that this cannot be an arrow function
     // because its implementation relies on arguments.
@@ -155,6 +163,10 @@
       self.looksPressedProperty.value = _.reduce( arguments, ( sum, newValue ) => sum || newValue, false );
     } );

+    this.looksOverMultilink = Property.multilink( looksOverProperties, function() {
+      self.looksOverProperty.value = _.reduce( arguments, ( sum, newValue ) => sum || newValue, false );
+    } );
+
     return pressListener;
   }
 }
Index: scenery/js/overlays/FocusOverlay.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/overlays/FocusOverlay.js	(revision 9c6549efc6ce9b9affc1df835e13f6983b87ede6)
+++ scenery/js/overlays/FocusOverlay.js	(date<PHONE_NUMBER>167)
@@ -24,6 +24,7 @@
 let outerGroupHighlightColor = FocusHighlightPath.OUTER_LIGHT_GROUP_FOCUS_COLOR;

 class FocusOverlay {
+
   /**
    * @param {Display} display
    * @param {Node} focusRootNode - the root node of our display
@@ -117,7 +118,7 @@
     this.visibilityListener = this.onVisibilityChange.bind( this );

     Display.focusProperty.link( this.focusListener );
-    this.highlightsVisibleProperty.link( this.visibilityListener );
+    Display.focusHighlightsVisibleProperty.link( this.visibilityListener );
   }

   /**
@@ -130,18 +131,7 @@
     }

     Display.focusProperty.unlink( this.focusListener );
-    this.highlightsVisibleProperty.unlink( this.visibilityListener );
-  }
-
-  /**
-   * Sets whether or not focus highlights will be visible. Effectively enables/disables visual highlights that
-   * follow DOM focus.
-   * @public
-   *
-   * @param {boolean} visible
-   */
-  setVisible( visible ) {
-    this.highlightsVisibleProperty.set( visible );
+    Display.focusHighlightsVisibleProperty.unlink( this.visibilityListener );
   }

   /**
@@ -198,8 +188,8 @@

         // the focusHighlight is just a node in the scene graph, so set it visible - visibility of other highlights
         // controlled by visibility of parent Nodes but that cannot be done in this case because the highlight
-        // can be anywhere in the scene graph, so have to check highlightsVisibleProperty
-        this.nodeFocusHighlight.visible = this.highlightsVisibleProperty.get();
+        // can be anywhere in the scene graph, so have to check the Property
+        this.nodeFocusHighlight.visible = Display.focusHighlightsVisibleProperty.get();
       }
       else {
         this.nodeFocusHighlight.visible = true;
@@ -442,7 +432,7 @@
    * @private
    */
   onVisibilityChange() {
-    const visible = this.highlightsVisibleProperty.get();
+    const visible = Display.focusHighlightsVisibleProperty.get();

     this.highlightNode.visible = visible;
     this.groupFocusHighlightParent.visible = visible;
Index: sun/js/buttons/MomentaryButtonInteractionStateProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/buttons/MomentaryButtonInteractionStateProperty.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/buttons/MomentaryButtonInteractionStateProperty.js	(date<PHONE_NUMBER>216)
@@ -20,9 +20,9 @@
     const options = { valueType: ButtonInteractionState };

     super(
-      [ buttonModel.overProperty, buttonModel.looksPressedProperty ],
-      ( over, looksPressed ) => {
-        return over && !looksPressed ? ButtonInteractionState.OVER :
+      [ buttonModel.looksOverProperty, buttonModel.looksPressedProperty ],
+      ( looksOver, looksPressed ) => {
+        return looksOver && !looksPressed ? ButtonInteractionState.OVER :
                looksPressed ? ButtonInteractionState.PRESSED :  // remain pressed regardless of whether 'over' is true
                ButtonInteractionState.IDLE;
       },
Index: sun/js/buttons/ToggleButtonInteractionStateProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/buttons/ToggleButtonInteractionStateProperty.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/buttons/ToggleButtonInteractionStateProperty.js	(date<PHONE_NUMBER>085)
@@ -18,9 +18,9 @@
     const options = { valueType: ButtonInteractionState };

     super(
-      [ buttonModel.overProperty, buttonModel.looksPressedProperty ],
-      ( over, looksPressed ) => {
-        return over && !looksPressed ? ButtonInteractionState.OVER :
+      [ buttonModel.looksOverProperty, buttonModel.looksPressedProperty ],
+      ( looksOver, looksPressed ) => {
+        return looksOver && !looksPressed ? ButtonInteractionState.OVER :
                looksPressed ? ButtonInteractionState.PRESSED :
                ButtonInteractionState.IDLE;
       },
Index: scenery/js/listeners/PressListener.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/listeners/PressListener.js	(revision 9c6549efc6ce9b9affc1df835e13f6983b87ede6)
+++ scenery/js/listeners/PressListener.js	(date<PHONE_NUMBER>414)
@@ -31,6 +31,7 @@
 import PhetioObject from '../../../tandem/js/PhetioObject.js';
 import Tandem from '../../../tandem/js/Tandem.js';
 import NullableIO from '../../../tandem/js/types/NullableIO.js';
+import Display from '../display/Display.js';
 import Mouse from '../input/Mouse.js';
 import SceneryEvent from '../input/SceneryEvent.js';
 import Node from '../nodes/Node.js';
@@ -174,6 +175,8 @@
     // @public {Property.<boolean>} (read-only) - It will be set to true when at least one pointer is over the listener.
     this.isOverProperty = new BooleanProperty( false );

+    this.looksOverProperty = new BooleanProperty( false );
+
     // @public {Property.<boolean>} (read-only) - It will be set to true when either:
     //   1. The listener is pressed and the pointer that is pressing is over the listener.
     //   2. There is at least one unpressed pointer that is over the listener.
@@ -284,6 +287,7 @@
     // update isOverProperty (not a DerivedProperty because we need to hook to passed-in properties)
     this.overPointers.lengthProperty.link( this.invalidateOver.bind( this ) );
     this.isFocusedProperty.link( this.invalidateOver.bind( this ) );
+    Display.focusHighlightsVisibleProperty.link( this.invalidateOver.bind( this ) );

     // update isHoveringProperty (not a DerivedProperty because we need to hook to passed-in properties)
     this.overPointers.lengthProperty.link( this._isHoveringListener );
@@ -543,6 +547,7 @@
     }

     this.isOverProperty.value = this.isFocusedProperty.value || ( this.overPointers.length > 0 && !pointerAttachedToOther );
+    this.looksOverProperty.value = ( this.isFocusedProperty.value && Display.focusHighlightsVisibleProperty.get() ) || ( this.overPointers.length > 0 && !pointerAttachedToOther );
   }

   /**
Index: scenery/js/display/Display.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/display/Display.js	(revision 9c6549efc6ce9b9affc1df835e13f6983b87ede6)
+++ scenery/js/display/Display.js	(date<PHONE_NUMBER>361)
@@ -50,6 +50,7 @@
  * @author Jonathan Olson<EMAIL_ADDRESS>  */

+import BooleanProperty from '../../../axon/js/BooleanProperty.js';
 import Emitter from '../../../axon/js/Emitter.js';
 import Property from '../../../axon/js/Property.js';
 import stepTimer from '../../../axon/js/stepTimer.js';
@@ -330,10 +331,6 @@

     if ( this._accessible ) {

-      // make the PDOM invisible in the browser - it has some width and is shifted off screen so that AT can read the
-      // formatting tags, see https://github.com/phetsims/scenery/issues/730
-      // SceneryStyle.addRule( '.accessibility, .accessibility * { position: relative; left: -1000px; top: 0; width: 250px; height: 0; clip: rect(0,0,0,0); pointerEvents: none }' );
-
       // @private {Node}
       this._focusRootNode = new Node();

@@ -812,26 +809,26 @@

   /**
    * Get the root accessible DOM element which represents this display and provides semantics for assistive
-   * technology.
+   * technology. If this Display is not accessible, returns null.
    * @public
    *
-   * @returns {HTMLElement}
+   * @returns {HTMLElement|null}
    */
   getAccessibleDOMElement() {
-    return this._rootAccessibleInstance.peer.primarySibling;
+    return this._accessible ? this._rootAccessibleInstance.peer.primarySibling : null;
   }

   get accessibleDOMElement() { return this.getAccessibleDOMElement(); }

-  /**
-   * Sets whether or not focus highlights are shown for this Display.
-   * @public
-   *
-   * @param {boolean} visible
-   */
-  setFocusHighlightVisible( visible ) {
-    this._focusOverlay.setVisible( visible );
-  }
+  // /**
+  //  * Sets whether or not focus highlights are shown for this Display.
+  //  * @public
+  //  *
+  //  * @param {boolean} visible
+  //  */
+  // setFocusHighlightVisible( visible ) {
+  //   this._focusOverlay.setVisible( visible );
+  // }

   /*
    * Returns the bitmask union of all renderers (canvas/svg/dom/webgl) that are used for display, excluding
@@ -2189,6 +2186,8 @@
   phetioReadOnly: true
 } );

+Display.focusHighlightsVisibleProperty = new BooleanProperty( false );
+
 // @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.
 // See https://github.com/phetsims/scenery/issues/802 and https://github.com/phetsims/vibe/issues/32 for more
Index: sun/js/buttons/PushButtonInteractionStateProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/buttons/PushButtonInteractionStateProperty.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/buttons/PushButtonInteractionStateProperty.js	(date<PHONE_NUMBER>774)
@@ -22,9 +22,9 @@
     }, options );

     super(
-      [ buttonModel.overProperty, buttonModel.looksPressedProperty ],
-      ( over, looksPressed ) => {
-        return over && !looksPressed ? ButtonInteractionState.OVER :
+      [ buttonModel.overProperty, buttonModel.looksOverProperty, buttonModel.looksPressedProperty ],
+      ( over, looksOver, looksPressed ) => {
+        return looksOver && !looksPressed ? ButtonInteractionState.OVER :
                over && looksPressed ? ButtonInteractionState.PRESSED :
                ButtonInteractionState.IDLE;
       },
Index: sun/js/buttons/StickyToggleButtonInteractionStateProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/buttons/StickyToggleButtonInteractionStateProperty.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/buttons/StickyToggleButtonInteractionStateProperty.js	(date<PHONE_NUMBER>373)
@@ -21,10 +21,10 @@
     const options = { valueType: ButtonInteractionState };

     super(
-      [ buttonModel.overProperty, buttonModel.looksPressedProperty, buttonModel.valueProperty ],
-      ( over, looksPressed, propertyValue ) => {
+      [ buttonModel.overProperty, buttonModel.looksOverProperty, buttonModel.looksPressedProperty, buttonModel.valueProperty ],
+      ( over, looksOver, looksPressed, propertyValue ) => {
         const isValueDown = ( propertyValue === buttonModel.valueDown );
-        return over && !( looksPressed || isValueDown ) ? ButtonInteractionState.OVER :
+        return looksOver && !( looksPressed || isValueDown ) ? ButtonInteractionState.OVER :
                over && ( looksPressed || isValueDown ) ? ButtonInteractionState.PRESSED :
                isValueDown ? ButtonInteractionState.PRESSED :
                ButtonInteractionState.IDLE;

suppressing the 'over' look with focus when focus highlights are invisible.

The way I propose to do this is to

Add a static focusHighlightsVisibleProperty to Display (or somewhere global), rather than an instance variable on FocusOverlay.
Add a looksOverProperty to PressListener. It is true if there is focus and Display.focusHighlightsVisibleProperty is true OR there are any over pointers.
Add a looksOverProperty to ButtonModel which gets updated like looksPressedProperty.
Change the various interaction state Properties like MomentaryButtonInteractionStateProperty  to be over with looksOverProperty rather than overProperty.

This is working well so far, but would be great to discuss with @zepumph and @jonathanolson since this involves some larger changes.
Here is the patch with this as a proof of concept:

Index: sun/js/buttons/ButtonModel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/buttons/ButtonModel.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/buttons/ButtonModel.js	(date<PHONE_NUMBER>708)
@@ -57,6 +57,8 @@
     // See PressListener.click for more details.
     this.looksPressedProperty = new BooleanProperty( false );

+    this.looksOverProperty = new BooleanProperty( false );
+
     // @public (read-only by users, read-write in subclasses) - emitter that is fired when sound should be produced
     this.produceSoundEmitter = new Emitter();

@@ -71,6 +73,8 @@
     // first listener is created. See this.createPressListener.
     this.looksPressedMultilink = null;

+    this.looksOverMultilink = null;
+
     // startCallback on pointer down, endCallback on pointer up. lazyLink so they aren't called immediately.
     this.downProperty.lazyLink( down => {
       if ( down ) {
@@ -101,6 +105,7 @@
       this.produceSoundEmitter.dispose();

       this.looksPressedMultilink && this.looksPressedMultilink.dispose();
+      this.looksOverMultilink && this.looksOverMultilink.dispose();

       this.listeners = [];
     };
@@ -143,10 +148,13 @@

     // dispose the previous multilink in case we already created a PressListener with this model
     this.looksPressedMultilink && this.looksPressedMultilink.dispose();
+    this.looksOverMultilink && this.looksOverMultilink.dispose();

     const looksPressedProperties = this.listeners.map( function( listener ) { return listener.looksPressedProperty; } );
     looksPressedProperties.push( this.downProperty );

+    const looksOverProperties = this.listeners.map( listener => { return listener.looksOverProperty; } );
+
     // assign a new Multilink (for disposal), and make sure that the button looks pressed when any of the
     // PressListeners created by this ButtonModel look pressed. Note that this cannot be an arrow function
     // because its implementation relies on arguments.
@@ -155,6 +163,10 @@
       self.looksPressedProperty.value = _.reduce( arguments, ( sum, newValue ) => sum || newValue, false );
     } );

+    this.looksOverMultilink = Property.multilink( looksOverProperties, function() {
+      self.looksOverProperty.value = _.reduce( arguments, ( sum, newValue ) => sum || newValue, false );
+    } );
+
     return pressListener;
   }
 }
Index: scenery/js/overlays/FocusOverlay.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/overlays/FocusOverlay.js	(revision 9c6549efc6ce9b9affc1df835e13f6983b87ede6)
+++ scenery/js/overlays/FocusOverlay.js	(date<PHONE_NUMBER>167)
@@ -24,6 +24,7 @@
 let outerGroupHighlightColor = FocusHighlightPath.OUTER_LIGHT_GROUP_FOCUS_COLOR;

 class FocusOverlay {
+
   /**
    * @param {Display} display
    * @param {Node} focusRootNode - the root node of our display
@@ -117,7 +118,7 @@
     this.visibilityListener = this.onVisibilityChange.bind( this );

     Display.focusProperty.link( this.focusListener );
-    this.highlightsVisibleProperty.link( this.visibilityListener );
+    Display.focusHighlightsVisibleProperty.link( this.visibilityListener );
   }

   /**
@@ -130,18 +131,7 @@
     }

     Display.focusProperty.unlink( this.focusListener );
-    this.highlightsVisibleProperty.unlink( this.visibilityListener );
-  }
-
-  /**
-   * Sets whether or not focus highlights will be visible. Effectively enables/disables visual highlights that
-   * follow DOM focus.
-   * @public
-   *
-   * @param {boolean} visible
-   */
-  setVisible( visible ) {
-    this.highlightsVisibleProperty.set( visible );
+    Display.focusHighlightsVisibleProperty.unlink( this.visibilityListener );
   }

   /**
@@ -198,8 +188,8 @@

         // the focusHighlight is just a node in the scene graph, so set it visible - visibility of other highlights
         // controlled by visibility of parent Nodes but that cannot be done in this case because the highlight
-        // can be anywhere in the scene graph, so have to check highlightsVisibleProperty
-        this.nodeFocusHighlight.visible = this.highlightsVisibleProperty.get();
+        // can be anywhere in the scene graph, so have to check the Property
+        this.nodeFocusHighlight.visible = Display.focusHighlightsVisibleProperty.get();
       }
       else {
         this.nodeFocusHighlight.visible = true;
@@ -442,7 +432,7 @@
    * @private
    */
   onVisibilityChange() {
-    const visible = this.highlightsVisibleProperty.get();
+    const visible = Display.focusHighlightsVisibleProperty.get();

     this.highlightNode.visible = visible;
     this.groupFocusHighlightParent.visible = visible;
Index: sun/js/buttons/MomentaryButtonInteractionStateProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/buttons/MomentaryButtonInteractionStateProperty.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/buttons/MomentaryButtonInteractionStateProperty.js	(date<PHONE_NUMBER>216)
@@ -20,9 +20,9 @@
     const options = { valueType: ButtonInteractionState };

     super(
-      [ buttonModel.overProperty, buttonModel.looksPressedProperty ],
-      ( over, looksPressed ) => {
-        return over && !looksPressed ? ButtonInteractionState.OVER :
+      [ buttonModel.looksOverProperty, buttonModel.looksPressedProperty ],
+      ( looksOver, looksPressed ) => {
+        return looksOver && !looksPressed ? ButtonInteractionState.OVER :
                looksPressed ? ButtonInteractionState.PRESSED :  // remain pressed regardless of whether 'over' is true
                ButtonInteractionState.IDLE;
       },
Index: sun/js/buttons/ToggleButtonInteractionStateProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/buttons/ToggleButtonInteractionStateProperty.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/buttons/ToggleButtonInteractionStateProperty.js	(date<PHONE_NUMBER>085)
@@ -18,9 +18,9 @@
     const options = { valueType: ButtonInteractionState };

     super(
-      [ buttonModel.overProperty, buttonModel.looksPressedProperty ],
-      ( over, looksPressed ) => {
-        return over && !looksPressed ? ButtonInteractionState.OVER :
+      [ buttonModel.looksOverProperty, buttonModel.looksPressedProperty ],
+      ( looksOver, looksPressed ) => {
+        return looksOver && !looksPressed ? ButtonInteractionState.OVER :
                looksPressed ? ButtonInteractionState.PRESSED :
                ButtonInteractionState.IDLE;
       },
Index: scenery/js/listeners/PressListener.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/listeners/PressListener.js	(revision 9c6549efc6ce9b9affc1df835e13f6983b87ede6)
+++ scenery/js/listeners/PressListener.js	(date<PHONE_NUMBER>414)
@@ -31,6 +31,7 @@
 import PhetioObject from '../../../tandem/js/PhetioObject.js';
 import Tandem from '../../../tandem/js/Tandem.js';
 import NullableIO from '../../../tandem/js/types/NullableIO.js';
+import Display from '../display/Display.js';
 import Mouse from '../input/Mouse.js';
 import SceneryEvent from '../input/SceneryEvent.js';
 import Node from '../nodes/Node.js';
@@ -174,6 +175,8 @@
     // @public {Property.<boolean>} (read-only) - It will be set to true when at least one pointer is over the listener.
     this.isOverProperty = new BooleanProperty( false );

+    this.looksOverProperty = new BooleanProperty( false );
+
     // @public {Property.<boolean>} (read-only) - It will be set to true when either:
     //   1. The listener is pressed and the pointer that is pressing is over the listener.
     //   2. There is at least one unpressed pointer that is over the listener.
@@ -284,6 +287,7 @@
     // update isOverProperty (not a DerivedProperty because we need to hook to passed-in properties)
     this.overPointers.lengthProperty.link( this.invalidateOver.bind( this ) );
     this.isFocusedProperty.link( this.invalidateOver.bind( this ) );
+    Display.focusHighlightsVisibleProperty.link( this.invalidateOver.bind( this ) );

     // update isHoveringProperty (not a DerivedProperty because we need to hook to passed-in properties)
     this.overPointers.lengthProperty.link( this._isHoveringListener );
@@ -543,6 +547,7 @@
     }

     this.isOverProperty.value = this.isFocusedProperty.value || ( this.overPointers.length > 0 && !pointerAttachedToOther );
+    this.looksOverProperty.value = ( this.isFocusedProperty.value && Display.focusHighlightsVisibleProperty.get() ) || ( this.overPointers.length > 0 && !pointerAttachedToOther );
   }

   /**
Index: scenery/js/display/Display.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/display/Display.js	(revision 9c6549efc6ce9b9affc1df835e13f6983b87ede6)
+++ scenery/js/display/Display.js	(date<PHONE_NUMBER>361)
@@ -50,6 +50,7 @@
  * @author Jonathan Olson<EMAIL_ADDRESS>  */

+import BooleanProperty from '../../../axon/js/BooleanProperty.js';
 import Emitter from '../../../axon/js/Emitter.js';
 import Property from '../../../axon/js/Property.js';
 import stepTimer from '../../../axon/js/stepTimer.js';
@@ -330,10 +331,6 @@

     if ( this._accessible ) {

-      // make the PDOM invisible in the browser - it has some width and is shifted off screen so that AT can read the
-      // formatting tags, see https://github.com/phetsims/scenery/issues/730
-      // SceneryStyle.addRule( '.accessibility, .accessibility * { position: relative; left: -1000px; top: 0; width: 250px; height: 0; clip: rect(0,0,0,0); pointerEvents: none }' );
-
       // @private {Node}
       this._focusRootNode = new Node();

@@ -812,26 +809,26 @@

   /**
    * Get the root accessible DOM element which represents this display and provides semantics for assistive
-   * technology.
+   * technology. If this Display is not accessible, returns null.
    * @public
    *
-   * @returns {HTMLElement}
+   * @returns {HTMLElement|null}
    */
   getAccessibleDOMElement() {
-    return this._rootAccessibleInstance.peer.primarySibling;
+    return this._accessible ? this._rootAccessibleInstance.peer.primarySibling : null;
   }

   get accessibleDOMElement() { return this.getAccessibleDOMElement(); }

-  /**
-   * Sets whether or not focus highlights are shown for this Display.
-   * @public
-   *
-   * @param {boolean} visible
-   */
-  setFocusHighlightVisible( visible ) {
-    this._focusOverlay.setVisible( visible );
-  }
+  // /**
+  //  * Sets whether or not focus highlights are shown for this Display.
+  //  * @public
+  //  *
+  //  * @param {boolean} visible
+  //  */
+  // setFocusHighlightVisible( visible ) {
+  //   this._focusOverlay.setVisible( visible );
+  // }

   /*
    * Returns the bitmask union of all renderers (canvas/svg/dom/webgl) that are used for display, excluding
@@ -2189,6 +2186,8 @@
   phetioReadOnly: true
 } );

+Display.focusHighlightsVisibleProperty = new BooleanProperty( false );
+
 // @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.
 // See https://github.com/phetsims/scenery/issues/802 and https://github.com/phetsims/vibe/issues/32 for more
Index: sun/js/buttons/PushButtonInteractionStateProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/buttons/PushButtonInteractionStateProperty.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/buttons/PushButtonInteractionStateProperty.js	(date<PHONE_NUMBER>774)
@@ -22,9 +22,9 @@
     }, options );

     super(
-      [ buttonModel.overProperty, buttonModel.looksPressedProperty ],
-      ( over, looksPressed ) => {
-        return over && !looksPressed ? ButtonInteractionState.OVER :
+      [ buttonModel.overProperty, buttonModel.looksOverProperty, buttonModel.looksPressedProperty ],
+      ( over, looksOver, looksPressed ) => {
+        return looksOver && !looksPressed ? ButtonInteractionState.OVER :
                over && looksPressed ? ButtonInteractionState.PRESSED :
                ButtonInteractionState.IDLE;
       },
Index: sun/js/buttons/StickyToggleButtonInteractionStateProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/buttons/StickyToggleButtonInteractionStateProperty.js	(revision 57ca44eb9624ac1c547b3b2ecc754531824bf648)
+++ sun/js/buttons/StickyToggleButtonInteractionStateProperty.js	(date<PHONE_NUMBER>373)
@@ -21,10 +21,10 @@
     const options = { valueType: ButtonInteractionState };

     super(
-      [ buttonModel.overProperty, buttonModel.looksPressedProperty, buttonModel.valueProperty ],
-      ( over, looksPressed, propertyValue ) => {
+      [ buttonModel.overProperty, buttonModel.looksOverProperty, buttonModel.looksPressedProperty, buttonModel.valueProperty ],
+      ( over, looksOver, looksPressed, propertyValue ) => {
         const isValueDown = ( propertyValue === buttonModel.valueDown );
-        return over && !( looksPressed || isValueDown ) ? ButtonInteractionState.OVER :
+        return looksOver && !( looksPressed || isValueDown ) ? ButtonInteractionState.OVER :
                over && ( looksPressed || isValueDown ) ? ButtonInteractionState.PRESSED :
                isValueDown ? ButtonInteractionState.PRESSED :
                ButtonInteractionState.IDLE;

Add a static focusHighlightsVisibleProperty to Display (or somewhere global), rather than an instance variable on FocusOverlay.

I think this should be controllable "per display", what if there was a secondary display that you always wanted the focus to highlight for. I don't have a really good idea beyond that though.

looksOverProperty

I know that's the naming direction we went with the "looksPressedProperty", but perhaps we should just rename this to "looksHighlightedProperty", triggered on focus and over. That is likely a larger rename, but I feel like since "over" is a scenery event, it really has more intrinsic meaning, and we shouldn't overload that further if possible.

Add a static focusHighlightsVisibleProperty to Display (or somewhere global), rather than an instance variable on FocusOverlay.

I think this should be controllable "per display", what if there was a secondary display that you always wanted the focus to highlight for. I don't have a really good idea beyond that though.

looksOverProperty

I know that's the naming direction we went with the "looksPressedProperty", but perhaps we should just rename this to "looksHighlightedProperty", triggered on focus and over. That is likely a larger rename, but I feel like since "over" is a scenery event, it really has more intrinsic meaning, and we shouldn't overload that further if possible.

Agreed with @zepumph above.
Also, the PressListener change seems like it would leak memory. But also I don't know how you'd handle a PressListener being in multiple Displays (which is possible).

Agreed with @zepumph above.
Also, the PressListener change seems like it would leak memory. But also I don't know how you'd handle a PressListener being in multiple Displays (which is possible).

@jessegreenberg, @jonathanolson, and I discussed this problem. We decided that it was best to keep a focusHighlightVisibleProperty per Display instance, even though we agree with having a single, static Display.focusProperty to follow how DOM focus works.
We think that in PressListener, instead of using a global on Display to see if the focusHighlight is visible, we can get the Display instance from the Display.focusProperty, but we only need to monitor it while focused, so we can add a listener on focus, and then remove it on blur or dispose.
We then also need to visit all other focus listeners to make sure they only do visual "hover" highlighting logic (or other stuff etc) when the focusHighlight is visible.

This logic should be factored out
A new issue should be created to try to improve on this pattern, with a priority based on how prevalent and gross the pattern looks what all is said and done here.

@jessegreenberg has to work on a few other things in the coming days, so I will look into this more this afternoon.

@jessegreenberg, @jonathanolson, and I discussed this problem. We decided that it was best to keep a focusHighlightVisibleProperty per Display instance, even though we agree with having a single, static Display.focusProperty to follow how DOM focus works.
We think that in PressListener, instead of using a global on Display to see if the focusHighlight is visible, we can get the Display instance from the Display.focusProperty, but we only need to monitor it while focused, so we can add a listener on focus, and then remove it on blur or dispose.
We then also need to visit all other focus listeners to make sure they only do visual "hover" highlighting logic (or other stuff etc) when the focusHighlight is visible.

This logic should be factored out
A new issue should be created to try to improve on this pattern, with a priority based on how prevalent and gross the pattern looks what all is said and done here.

@jessegreenberg has to work on a few other things in the coming days, so I will look into this more this afternoon.

I'll continue on this now.

I'll continue on this now.

In the most recent patch I see, this is the display input listener:

      this.display.addInputListener( {
        down: event => {

          // in the self-voicing prototype we want the focus highlight to remain with
          // mouse/touch presses
          if ( !phet.chipper.queryParameters.supportsSelfVoicing ) {

            // An AT might have sent a down event outside of the display, if this happened we will not remove focus.
            if ( this.display.bounds.containsPoint( event.pointer.point ) ) {

              // no need to do this work unless some element in the simulation has focus
              if ( Display.focusedNode ) {
                if ( event.trail.nodes.includes( Display.focusedNode ) ) {
                  this.display.setFocusHighlightVisible( false );
                }
                else {
                  Display.focus = null;
                }
              }
            }
          }
        },
        focus: event => {
          this.display.setFocusHighlightVisible( true );
        }
      } );

But @jessegreenberg I thought we changed this substantially. I feel like this patch is missing some important changes. Do you still have them in your working copy?

In the most recent patch I see, this is the display input listener:

      this.display.addInputListener( {
        down: event => {

          // in the self-voicing prototype we want the focus highlight to remain with
          // mouse/touch presses
          if ( !phet.chipper.queryParameters.supportsSelfVoicing ) {

            // An AT might have sent a down event outside of the display, if this happened we will not remove focus.
            if ( this.display.bounds.containsPoint( event.pointer.point ) ) {

              // no need to do this work unless some element in the simulation has focus
              if ( Display.focusedNode ) {
                if ( event.trail.nodes.includes( Display.focusedNode ) ) {
                  this.display.setFocusHighlightVisible( false );
                }
                else {
                  Display.focus = null;
                }
              }
            }
          }
        },
        focus: event => {
          this.display.setFocusHighlightVisible( true );
        }
      } );

But @jessegreenberg I thought we changed this substantially. I feel like this patch is missing some important changes. Do you still have them in your working copy?

[ ] This patch felt a bit different, and since I'm just entering part-way through, I'd like to divide up commits as much as possible. I'm not currently thinking we need this in the main changeset. I'll apply later:

Index: js/display/Display.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/display/Display.js b/js/display/Display.js
--- a/js/display/Display.js	(revision d27ac19414d1d249c58edb6291baef6f733285a5)
+++ b/js/display/Display.js	(date<PHONE_NUMBER>570)
@@ -812,13 +812,13 @@

   /**
    * Get the root accessible DOM element which represents this display and provides semantics for assistive
-   * technology.
+   * technology. If this Display is not accessible, returns null.
    * @public
    *
-   * @returns {HTMLElement}
+   * @returns {HTMLElement|null}
    */
   getAccessibleDOMElement() {
-    return this._rootAccessibleInstance.peer.primarySibling;
+    return this._accessible ? this._rootAccessibleInstance.peer.primarySibling : null;
   }

   get accessibleDOMElement() { return this.getAccessibleDOMElement(); }

[ ] This patch felt a bit different, and since I'm just entering part-way through, I'd like to divide up commits as much as possible. I'm not currently thinking we need this in the main changeset. I'll apply later:

Index: js/display/Display.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/display/Display.js b/js/display/Display.js
--- a/js/display/Display.js	(revision d27ac19414d1d249c58edb6291baef6f733285a5)
+++ b/js/display/Display.js	(date<PHONE_NUMBER>570)
@@ -812,13 +812,13 @@

   /**
    * Get the root accessible DOM element which represents this display and provides semantics for assistive
-   * technology.
+   * technology. If this Display is not accessible, returns null.
    * @public
    *
-   * @returns {HTMLElement}
+   * @returns {HTMLElement|null}
    */
   getAccessibleDOMElement() {
-    return this._rootAccessibleInstance.peer.primarySibling;
+    return this._accessible ? this._rootAccessibleInstance.peer.primarySibling : null;
   }

   get accessibleDOMElement() { return this.getAccessibleDOMElement(); }

I'm having good progress on my local copy, but I want to take notes of what is next:

[ ] most usages of .overProperty should likely look at .looksOverProperty. This applies to ButtonModel and PressListener, but we need to be careful in checking for usages.

I'm having good progress on my local copy, but I want to take notes of what is next:

[ ] most usages of .overProperty should likely look at .looksOverProperty. This applies to ButtonModel and PressListener, but we need to be careful in checking for usages.

Summary of work so far:

Rewriting of "focus rules" in sim.js in how we listener to the display events to determine if we should blur focus, or toggle highlights visible.
Creation of PressListener.looksOverProperty, to combine over OR (focus and display shows focus highlights).
Move Display.setFocusHighlightsVisible() to be a Property, passed in as an option to FocusOverlay so that there isn't too much unneeded duplication.
Go through all usages of focus listeners and event.isFromPDOM() to combine PDOM and "main" code paths. Most of the time the PDOM specific logic was just controlling focus, and now we can do that always (since Sim.js will control if we see the focus highlgihts)
Remove isPDOMClicking(), unneeded.
IMPORTANT: The main piece that we hadn't covered yet that I ran into in this issue, is that is wasn't enough to just have "looksOverProperty integrated into interactionStateProperties, the other side of this is that PressListener.overProperty is no longer true when focused, and so I needed to add ButtonModel.focusedProperty, and then to update supporting logic throughout sun to make sure that you can press when over OR focused. This has felt like a deficiency for some time now, and I'm glad to have clarified it here.
From the above, note that in interactionStateProperties, looksOverProperty is used to determine "over" state, but a combo of "over || focused" is used for the pressed logic. This is the same exact thing for the actual behavior check to fire, like in PushButtonModel:86

This is a really huge huge commit. It basically is a result of the entirety of @jessegreenberg's and my a11y time for the week (at least a couple of days of it). I don't like doing this all as one batch, but I also hate the idea of forgetting details over the weekend and trying to handle it further as a patch. I think the best path forward is to mark this as (very) blocking for all sims, and then @jessegreenberg and I can work on this Monday morning. I did thorough testing with fuzzBoard, normal fuzzing, and manual mouse/keyboard interaction on many components. I'm hoping we can fix this up quickly if there are any problems. I'm feeling about 90% confident in the change set (though 100% in the direction).
Next steps to me is a thorough review, of all components (that will take some time I think), and to cover any issues CT demonstrates.

Summary of work so far:

Rewriting of "focus rules" in sim.js in how we listener to the display events to determine if we should blur focus, or toggle highlights visible.
Creation of PressListener.looksOverProperty, to combine over OR (focus and display shows focus highlights).
Move Display.setFocusHighlightsVisible() to be a Property, passed in as an option to FocusOverlay so that there isn't too much unneeded duplication.
Go through all usages of focus listeners and event.isFromPDOM() to combine PDOM and "main" code paths. Most of the time the PDOM specific logic was just controlling focus, and now we can do that always (since Sim.js will control if we see the focus highlgihts)
Remove isPDOMClicking(), unneeded.
IMPORTANT: The main piece that we hadn't covered yet that I ran into in this issue, is that is wasn't enough to just have "looksOverProperty integrated into interactionStateProperties, the other side of this is that PressListener.overProperty is no longer true when focused, and so I needed to add ButtonModel.focusedProperty, and then to update supporting logic throughout sun to make sure that you can press when over OR focused. This has felt like a deficiency for some time now, and I'm glad to have clarified it here.
From the above, note that in interactionStateProperties, looksOverProperty is used to determine "over" state, but a combo of "over || focused" is used for the pressed logic. This is the same exact thing for the actual behavior check to fire, like in PushButtonModel:86

This is a really huge huge commit. It basically is a result of the entirety of @jessegreenberg's and my a11y time for the week (at least a couple of days of it). I don't like doing this all as one batch, but I also hate the idea of forgetting details over the weekend and trying to handle it further as a patch. I think the best path forward is to mark this as (very) blocking for all sims, and then @jessegreenberg and I can work on this Monday morning. I did thorough testing with fuzzBoard, normal fuzzing, and manual mouse/keyboard interaction on many components. I'm hoping we can fix this up quickly if there are any problems. I'm feeling about 90% confident in the change set (though 100% in the direction).
Next steps to me is a thorough review, of all components (that will take some time I think), and to cover any issues CT demonstrates.

@samreid this review should block GAO RC.

@samreid this review should block GAO RC.

Great work @zepumph, thanks for getting this to a commit point.

The main piece that we hadn't covered yet that I ran into in this issue, is that is wasn't enough to just have "looksOverProperty integrated into interactionStateProperties, the other side of this is that...

I think this may be caused by the logic change in the setting of looksOverProperty. In master it is now looksOverProperty and overProperty are set with
    this.isOverProperty.value = ( this.overPointers.length > 0 && !pointerAttachedToOther );
    this.looksOverProperty.value = this.isOverProperty.value ||
                                   ( this.isFocusedProperty.value && !!this.display && this.display.focusHighlightsVisibleProperty.value );

An earlier patch had
     this.isOverProperty.value = this.isFocusedProperty.value || ( this.overPointers.length > 0 && !pointerAttachedToOther );
     this.looksOverProperty.value = ( this.isFocusedProperty.value && Display.focusHighlightsVisibleProperty.get() ) || ( this.overPointers.length > 0 && !pointerAttachedToOther );

There is duplication, but this may make isFocusedProperty in ButtonModel and the next bullet point not needed.
A review comment was added to ButtonModel

const looksPressedProperties = this.listeners.map( listener => listener.looksPressedProperty );

    looksPressedProperties.push( this.downProperty ); // MK is not sure why this is needed.

The reason is in https://github.com/phetsims/ph-scale/issues/71#issuecomment-426571289, I will add a comment explaining.

Great work @zepumph, thanks for getting this to a commit point.

The main piece that we hadn't covered yet that I ran into in this issue, is that is wasn't enough to just have "looksOverProperty integrated into interactionStateProperties, the other side of this is that...

I think this may be caused by the logic change in the setting of looksOverProperty. In master it is now looksOverProperty and overProperty are set with
    this.isOverProperty.value = ( this.overPointers.length > 0 && !pointerAttachedToOther );
    this.looksOverProperty.value = this.isOverProperty.value ||
                                   ( this.isFocusedProperty.value && !!this.display && this.display.focusHighlightsVisibleProperty.value );

An earlier patch had
     this.isOverProperty.value = this.isFocusedProperty.value || ( this.overPointers.length > 0 && !pointerAttachedToOther );
     this.looksOverProperty.value = ( this.isFocusedProperty.value && Display.focusHighlightsVisibleProperty.get() ) || ( this.overPointers.length > 0 && !pointerAttachedToOther );

There is duplication, but this may make isFocusedProperty in ButtonModel and the next bullet point not needed.
A review comment was added to ButtonModel

const looksPressedProperties = this.listeners.map( listener => listener.looksPressedProperty );

    looksPressedProperties.push( this.downProperty ); // MK is not sure why this is needed.

The reason is in https://github.com/phetsims/ph-scale/issues/71#issuecomment-426571289, I will add a comment explaining.

@zepumph and I discussed the addition of isFocusedProperty in ButtonModel and agreed that it is the better way. It clarifies what isOverProperty means in PressListener and that is worth the additional code in sun.
We are good to close this issue!

@zepumph and I discussed the addition of isFocusedProperty in ButtonModel and agreed that it is the better way. It clarifies what isOverProperty means in PressListener and that is worth the additional code in sun.
We are good to close this issue!


Bug: Opening two identical pages in a row, but with a different DataContext, is blocked

Fixed by 42cd01a commit