Merge branch 'main' of https://github.com/phetsims/scenery into keybo…

…ard-drag-listener-wip

js/accessibility/FocusDisplayedController.ts

@@ -1,4 +1,4 @@
-// Copyright 2021-2023, University of Colorado Boulder
+// Copyright 2021-2024, University of Colorado Boulder
 
 /**
  * Responsible for setting the provided focusProperty to null when the Focused node either
@@ -22,11 +22,12 @@ class FocusDisplayedController {
   private visibilityTracker: TrailVisibilityTracker | null = null;
 
   // When there is value, we will watch and update when there are changes to the displayed state of the Focus trail.
-  private focusProperty: TProperty<Focus | null> | null;
+  private focusProperty: TProperty<Focus | null>;
 
   // Bound functions that are called when the displayed state of the Node changes.
   private readonly boundVisibilityListener: () => void;
   private readonly boundInstancesChangedListener: ( instance: Instance ) => void;
+  private readonly boundNodeDisposedListener: () => void;
 
   // Handles changes to focus, adding or removing listeners
   private readonly boundFocusListener: ( focus: Focus | null ) => void;
@@ -36,6 +37,7 @@ class FocusDisplayedController {
 
     this.boundVisibilityListener = this.handleTrailVisibilityChange.bind( this );
     this.boundInstancesChangedListener = this.handleInstancesChange.bind( this );
+    this.boundNodeDisposedListener = this.handleNodeDisposed.bind( this );
 
     this.boundFocusListener = this.handleFocusChange.bind( this );
     this.focusProperty.link( this.boundFocusListener );
@@ -58,7 +60,7 @@ class FocusDisplayedController {
    */
   private handleTrailVisibilityChange(): void {
     if ( this.visibilityTracker && !this.visibilityTracker.trailVisibleProperty.value ) {
-      this.focusProperty!.value = null;
+      this.focusProperty.value = null;
     }
   }
 
@@ -68,10 +70,19 @@ class FocusDisplayedController {
    */
   private handleInstancesChange( instance: Instance ): void {
     if ( instance.node && instance.node.instances.length === 0 ) {
-      this.focusProperty!.value = null;
+      this.focusProperty.value = null;
     }
   }
 
+  /**
+   * While this focus-clear is mostly covered by listening for instance changes, there is an intermediate state between
+   * when a Node is disposed, and when the Instance tree is updated to reflect that disposal (during updateDisplay()).
+   * This function handles that atypical case (pretty much impossible to get to in PhET sims except during fuzzing).
+   */
+  private handleNodeDisposed(): void {
+    this.focusProperty.value = null;
+  }
+
   /**
    * Add listeners that watch when the Displayed state of the Node with Focus has changed,
    * including visibility of the trail and attachment to a scene graph.
@@ -84,7 +95,11 @@ class FocusDisplayedController {
     this.visibilityTracker.addListener( this.boundVisibilityListener );
 
     this.node = focus.trail.lastNode();
+
+    // This is potentially dangerous to listen to generally, but in this case it is safe because the state we change
+    // will only affect a separate display's state, not this one.
     this.node.changedInstanceEmitter.addListener( this.boundInstancesChangedListener );
+    this.node.disposeEmitter.addListener( this.boundNodeDisposedListener );
   }
 
   /**
@@ -99,6 +114,7 @@ class FocusDisplayedController {
     }
     if ( this.node ) {
       this.node.changedInstanceEmitter.removeListener( this.boundInstancesChangedListener );
+      this.node.disposeEmitter.removeListener( this.boundNodeDisposedListener );
       this.node = null;
     }
   }
@@ -107,11 +123,10 @@ class FocusDisplayedController {
 
     // this disposes the TrailVisibilityTracker and removes any listeners on the Node
     this.removeDisplayedListeners();
-    this.focusProperty!.unlink( this.boundFocusListener );
+    this.focusProperty.unlink( this.boundFocusListener );
 
     this.node = null;
     this.visibilityTracker = null;
-    this.focusProperty = null;
   }
 }
 

js/accessibility/voicing/InteractiveHighlighting.ts

@@ -105,6 +105,9 @@ const InteractiveHighlighting = memoize( <SuperType extends Constructor<Node>>(
       };
 
       this._changedInstanceListener = this.onChangedInstance.bind( this );
+
+      // This is potentially dangerous to listen to generally, but in this case it is safe because the state we change
+      // will only affect a separate display's state, not this one.
       this.changedInstanceEmitter.addListener( this._changedInstanceListener );
 
       this._interactiveHighlightingEnabledListener = this._onInteractiveHighlightingEnabledChange.bind( this );
@@ -427,14 +430,11 @@ const InteractiveHighlighting = memoize( <SuperType extends Constructor<Node>>(
           const focus = display.focusManager.pointerFocusProperty.value;
           const locked = !!display.focusManager.lockedPointerFocusProperty.value;
 
-          // Workaround for https://github.com/phetsims/density-buoyancy-common/issues/97. We should not try to
-          // highlight lock if the focus is disposed, but it won't be cleaned up until instances change upon next updateDisplay().
-          const focusIsNotDisposedWorkaround = !focus?.trail.lastNode().isDisposed;
-
           // Focus should generally be defined when pointer enters the Node, but it may be null in cases of
           // cancel or interrupt. Don't attempt to lock if the FocusManager already has a locked highlight (especially
           // important for gracefully handling multitouch).
-          if ( focus && !locked && focusIsNotDisposedWorkaround ) {
+          if ( focus && !locked ) {
+            assert && assert( !focus.trail.lastNode().isDisposed, 'Focus should not be set to a disposed Node' );
 
             // Set the lockedPointerFocusProperty with a copy of the Focus (as deep as possible) because we want
             // to keep a reference to the old Trail while pointerFocusProperty changes.

js/accessibility/voicing/Voicing.ts

@@ -1,4 +1,4 @@
-// Copyright 2021-2023, University of Colorado Boulder
+// Copyright 2021-2024, University of Colorado Boulder
 
 /**
  * A trait for Node that supports the Voicing feature, under accessibility. Allows you to define responses for the Node
@@ -114,7 +114,8 @@ const Voicing = <SuperType extends Constructor<Node>>( Type: SuperType ) => { //
     private _voicingFocusListener!: SceneryListenerFunction<FocusEvent> | null;
 
     // Indicates whether this Node can speak. A Node can speak if self and all of its ancestors are visible and
-    // voicingVisible.
+    // voicingVisible. This is private because its value depends on the state of the Instance tree. Listening to this
+    // to change the scene graph state can be incredibly dangerous and buggy, see https://github.com/phetsims/scenery/issues/1615
     private _voicingCanSpeakProperty!: TinyProperty<boolean>;
 
     // A counter that keeps track of visible and voicingVisible Instances of this Node.
@@ -166,6 +167,9 @@ const Voicing = <SuperType extends Constructor<Node>>( Type: SuperType ) => { //
       this._voicingCanSpeakCount = 0;
 
       this._boundInstancesChangedListener = this.addOrRemoveInstanceListeners.bind( this );
+
+      // This is potentially dangerous to listen to generally, but in this case it is safe because the state we change
+      // will only affect how we voice (part of the audio view), and not part of this display's scene graph.
       this.changedInstanceEmitter.addListener( this._boundInstancesChangedListener );
 
       this._speakContentOnFocusListener = {
@@ -488,16 +492,6 @@ const Voicing = <SuperType extends Constructor<Node>>( Type: SuperType ) => { //
       return this._voicingUtterance;
     }
 
-    /**
-     * Get the Property indicating that this Voicing Node can speak. True when this Voicing Node and all of its
-     * ancestors are visible and voicingVisible.
-     */
-    public getVoicingCanSpeakProperty(): TinyProperty<boolean> {
-      return this._voicingCanSpeakProperty;
-    }
-
-    public get voicingCanSpeakProperty() { return this.getVoicingCanSpeakProperty(); }
-
     /**
      * Called whenever this Node is focused.
      */
@@ -675,8 +669,11 @@ Voicing.alertUtterance = ( utterance: Utterance ) => {
  */
 Voicing.registerUtteranceToVoicingNode = ( utterance: Utterance, voicingNode: VoicingNode ) => {
   const existingCanAnnounceProperties = utterance.voicingCanAnnounceProperties;
-  if ( !existingCanAnnounceProperties.includes( voicingNode.voicingCanSpeakProperty ) ) {
-    utterance.voicingCanAnnounceProperties = existingCanAnnounceProperties.concat( [ voicingNode.voicingCanSpeakProperty ] );
+
+  // @ts-expect-error Accessing a private member because this is meant to be "private to the file".
+  const voicingCanSpeakProperty = voicingNode._voicingCanSpeakProperty;
+  if ( !existingCanAnnounceProperties.includes( voicingCanSpeakProperty ) ) {
+    utterance.voicingCanAnnounceProperties = existingCanAnnounceProperties.concat( [ voicingCanSpeakProperty ] );
   }
 };
 
@@ -686,7 +683,10 @@ Voicing.registerUtteranceToVoicingNode = ( utterance: Utterance, voicingNode: Vo
  */
 Voicing.unregisterUtteranceToVoicingNode = ( utterance: Utterance, voicingNode: VoicingNode ) => {
   const existingCanAnnounceProperties = utterance.voicingCanAnnounceProperties;
-  const index = existingCanAnnounceProperties.indexOf( voicingNode.voicingCanSpeakProperty );
+
+  // @ts-expect-error Accessing a private member because this is meant to be "private to the file".
+  const voicingCanSpeakProperty = voicingNode._voicingCanSpeakProperty;
+  const index = existingCanAnnounceProperties.indexOf( voicingCanSpeakProperty );
   assert && assert( index > -1, 'voicingNode.voicingCanSpeakProperty is not on the Utterance, was it not registered?' );
   utterance.voicingCanAnnounceProperties = existingCanAnnounceProperties.splice( index, 1 );
 };

js/accessibility/voicing/voicingManager.ts

@@ -1,4 +1,4 @@
-// Copyright 2020-2023, University of Colorado Boulder
+// Copyright 2020-2024, University of Colorado Boulder
 
 /**
  * Uses the Web Speech API to produce speech from the browser. This is a prototype, DO NOT USE IN PRODUCTION CODE.
@@ -12,7 +12,7 @@
 import SpeechSynthesisAnnouncer, { SpeechSynthesisAnnouncerOptions, SpeechSynthesisInitializeOptions } from '../../../../utterance-queue/js/SpeechSynthesisAnnouncer.js';
 import { globalKeyStateTracker, KeyboardUtils, scenery } from '../../imports.js';
 import optionize, { EmptySelfOptions } from '../../../../phet-core/js/optionize.js';
-import TEmitter from '../../../../axon/js/TEmitter.js';
+import { TReadOnlyEmitter } from '../../../../axon/js/TEmitter.js';
 
 type SelfOptions = EmptySelfOptions;
 type VoicingManagerOptions = SelfOptions & SpeechSynthesisAnnouncerOptions;
@@ -35,7 +35,7 @@ class VoicingManager extends SpeechSynthesisAnnouncer {
   /**
    * The initialization with some additional scenery-specific work for voicingManager.
    */
-  public override initialize( userGestureEmitter: TEmitter, options?: SpeechSynthesisInitializeOptions ): void {
+  public override initialize( userGestureEmitter: TReadOnlyEmitter, options?: SpeechSynthesisInitializeOptions ): void {
     super.initialize( userGestureEmitter, options );
 
     // The control key will stop the synth from speaking if there is an active utterance. This key was decided because

js/display/Display.ts

@@ -94,6 +94,10 @@ type SelfOptions = {
   // What cursor is used when no other cursor is specified
   defaultCursor?: string;
 
+  // Forces SVG elements to be refreshed every frame, which can force repainting and detect (or potentially in some
+  // cases work around) SVG rendering browser bugs. See https://github.com/phetsims/scenery/issues/1507
+  forceSVGRefresh?: boolean;
+
   // Initial background color
   backgroundColor?: Color | string | null;
 
@@ -205,6 +209,7 @@ export default class Display {
   public _aggressiveContextRecreation: boolean;
   public _allowBackingScaleAntialiasing: boolean;
   public _allowLayerFitting: boolean;
+  public _forceSVGRefresh: boolean;
 
   private readonly _allowWebGL: boolean;
   private readonly _allowCSSHacks: boolean;
@@ -304,6 +309,13 @@ export default class Display {
   private perfDrawableOldIntervalCount?: number;
   private perfDrawableNewIntervalCount?: number;
 
+  // (scenery-internal) When fired, forces an SVG refresh, to try to work around issues
+  // like https://github.com/phetsims/scenery/issues/1507
+  public readonly _refreshSVGEmitter = new Emitter();
+
+  // If true, we will refresh the SVG elements on the next frame
+  private _refreshSVGPending = false;
+
   /**
    * Constructs a Display that will show the rootNode and its subtree in a visual state. Default options provided below
    *
@@ -332,6 +344,8 @@ export default class Display {
 
       allowLayerFitting: false,
 
+      forceSVGRefresh: false,
+
       // {string} - What cursor is used when no other cursor is specified
       defaultCursor: 'default',
 
@@ -439,6 +453,7 @@ export default class Display {
     this._aggressiveContextRecreation = options.aggressiveContextRecreation;
     this._allowBackingScaleAntialiasing = options.allowBackingScaleAntialiasing;
     this._allowLayerFitting = options.allowLayerFitting;
+    this._forceSVGRefresh = options.forceSVGRefresh;
     this._overlays = [];
     this._pointerOverlay = null;
     this._pointerAreaOverlay = null;
@@ -702,6 +717,12 @@ export default class Display {
 
     PDOMTree.auditPDOMDisplays( this.rootNode );
 
+    if ( this._forceSVGRefresh || this._refreshSVGPending ) {
+      this._refreshSVGPending = false;
+
+      this.refreshSVG();
+    }
+
     sceneryLog && sceneryLog.Display && sceneryLog.pop();
   }
 
@@ -2075,6 +2096,23 @@ export default class Display {
     return ( instance && instance.trail ) ? instance.trail : null;
   }
 
+  /**
+   * Forces SVG elements to have their visual contents refreshed, by changing state in a non-visually-apparent way.
+   * It should trick browsers into re-rendering the SVG elements.
+   *
+   * See https://github.com/phetsims/scenery/issues/1507
+   */
+  public refreshSVG(): void {
+    this._refreshSVGEmitter.emit();
+  }
+
+  /**
+   * Similar to refreshSVG (see docs above), but will do so on the next frame.
+   */
+  public refreshSVGOnNextFrame(): void {
+    this._refreshSVGPending = true;
+  }
+
   /**
    * Releases references.
    *

js/display/SVGBlock.js

@@ -1,4 +1,4 @@
-// Copyright 2013-2023, University of Colorado Boulder
+// Copyright 2013-2024, University of Colorado Boulder
 
 /**
  * Handles a visual SVG layer of drawables.
@@ -8,6 +8,7 @@
 
 import cleanArray from '../../../phet-core/js/cleanArray.js';
 import Poolable from '../../../phet-core/js/Poolable.js';
+import dotRandom from '../../../dot/js/dotRandom.js';
 import { CountMap, FittedBlock, scenery, SVGGroup, svgns, Utils } from '../imports.js';
 
 class SVGBlock extends FittedBlock {
@@ -83,6 +84,29 @@ class SVGBlock extends FittedBlock {
       this.domElement = this.svg;
     }
 
+    // Forces SVG elements to be refreshed every frame, which can force repainting and detect (or potentially in some
+    // cases work around) SVG rendering browser bugs. See https://github.com/phetsims/scenery/issues/1507
+    // @private {function} - Forces a color change on the 0x0 rect
+    this.forceRefreshListener = () => {
+      // Lazily add this, so we're not incurring any performance penalties until we actually need it
+      if ( !this.workaroundRect ) {
+        const workaroundGroup = document.createElementNS( svgns, 'g' );
+        this.svg.appendChild( workaroundGroup );
+
+        this.workaroundRect = document.createElementNS( svgns, 'rect' );
+        this.workaroundRect.setAttribute( 'width', '0' );
+        this.workaroundRect.setAttribute( 'height', '0' );
+        this.workaroundRect.setAttribute( 'fill', 'none' );
+        workaroundGroup.appendChild( this.workaroundRect );
+      }
+
+      const red = dotRandom.nextIntBetween( 0, 255 );
+      const green = dotRandom.nextIntBetween( 0, 255 );
+      const blue = dotRandom.nextIntBetween( 0, 255 );
+      this.workaroundRect.setAttribute( 'fill', `rgba(${red},${green},${blue},0.02)` );
+    };
+    this.display._refreshSVGEmitter.addListener( this.forceRefreshListener );
+
     // reset what layer fitting can do
     Utils.prepareForTransform( this.svg ); // Apply CSS needed for future CSS transforms to work properly.
 
@@ -367,6 +391,8 @@ class SVGBlock extends FittedBlock {
 
     this.paintCountMap.clear();
 
+    this.display._refreshSVGEmitter.removeListener( this.forceRefreshListener );
+
     this.baseTransformGroup.removeChild( this.rootGroup.svgGroup );
     this.rootGroup.dispose();
     this.rootGroup = null;