From 047b5e26d83a37a1e0a63a4f14c6fd435001b99a Mon Sep 17 00:00:00 2001 From: Sam Reid Date: Wed, 6 Mar 2024 14:30:36 -0700 Subject: [PATCH 01/10] Prefer TReadOnlyEmitter, see https://github.com/phetsims/axon/issues/450 --- js/accessibility/voicing/voicingManager.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/js/accessibility/voicing/voicingManager.ts b/js/accessibility/voicing/voicingManager.ts index 3ed6116e8..ee610b595 100644 --- a/js/accessibility/voicing/voicingManager.ts +++ b/js/accessibility/voicing/voicingManager.ts @@ -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 From 06126e0d89324d7578967f46969f7646fc01c102 Mon Sep 17 00:00:00 2001 From: phet-dev Date: Thu, 7 Mar 2024 03:22:00 -0700 Subject: [PATCH 02/10] update copyright dates from daily grunt work --- js/accessibility/FocusDisplayedController.ts | 2 +- js/accessibility/voicing/voicingManager.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/js/accessibility/FocusDisplayedController.ts b/js/accessibility/FocusDisplayedController.ts index dad86dd5e..a603026f6 100644 --- a/js/accessibility/FocusDisplayedController.ts +++ b/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 diff --git a/js/accessibility/voicing/voicingManager.ts b/js/accessibility/voicing/voicingManager.ts index ee610b595..0a042c412 100644 --- a/js/accessibility/voicing/voicingManager.ts +++ b/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. From bfee694e3542ba44fa5bbb7f4ccd556ac038d292 Mon Sep 17 00:00:00 2001 From: Sam Reid Date: Thu, 7 Mar 2024 13:10:01 -0700 Subject: [PATCH 03/10] Move documentation from optionize call site to the declaration of the options type, see https://github.com/phetsims/projectile-data-lab/issues/225 --- js/nodes/Node.ts | 55 +++++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/js/nodes/Node.ts b/js/nodes/Node.ts index 8bf441a88..4a879462c 100644 --- a/js/nodes/Node.ts +++ b/js/nodes/Node.ts @@ -373,11 +373,42 @@ export type NodeOptions = { } & ParallelDOMOptions & NodeTransformOptions; type RasterizedOptions = { + + // {number} - Controls the resolution of the image relative to the local view units. For example, if our Node is + // ~100 view units across (in the local coordinate frame) but you want the image to actually have a ~200-pixel + // resolution, provide resolution:2. + // Defaults to 1.0 resolution?: number; + + // {Bounds2|null} - If provided, it will control the x/y/width/height of the toCanvas call. See toCanvas for + // details on how this controls the rasterization. This is in the "parent" coordinate frame, similar to + // node.bounds. + // Defaults to null sourceBounds?: Bounds2 | null; + + // {boolean} - If true, the localBounds of the result will be set in a way such that it will precisely match + // the visible bounds of the original Node (this). Note that antialiased content (with a much lower resolution) + // may somewhat spill outside these bounds if this is set to true. Usually this is fine and should be the + // recommended option. If sourceBounds are provided, they will restrict the used bounds (so it will just + // represent the bounds of the sliced part of the image). + // Defaults to true useTargetBounds?: boolean; + + // {boolean} - If true, the created Image Node gets wrapped in an extra Node so that it can be transformed + // independently. If there is no need to transform the resulting node, wrap:false can be passed so that no extra + // Node is created. + // Defaults to true wrap?: boolean; + + // {boolean} - If true, it will directly use the element (only works with canvas/webgl renderers) + // instead of converting this into a form that can be used with any renderer. May have slightly better + // performance if svg/dom renderers do not need to be used. + // Defaults to false useCanvas?: boolean; + + // To be passed to the Image node created from the rasterization. See below for options that will override + // what is passed in. + // Defaults to the empty object imageOptions?: ImageOptions; }; @@ -5693,35 +5724,11 @@ class Node extends ParallelDOM { */ public rasterized( providedOptions?: RasterizedOptions ): Node { const options = optionize()( { - // {number} - Controls the resolution of the image relative to the local view units. For example, if our Node is - // ~100 view units across (in the local coordinate frame) but you want the image to actually have a ~200-pixel - // resolution, provide resolution:2. resolution: 1, - - // {Bounds2|null} - If provided, it will control the x/y/width/height of the toCanvas call. See toCanvas for - // details on how this controls the rasterization. This is in the "parent" coordinate frame, similar to - // node.bounds. sourceBounds: null, - - // {boolean} - If true, the localBounds of the result will be set in a way such that it will precisely match - // the visible bounds of the original Node (this). Note that antialiased content (with a much lower resolution) - // may somewhat spill outside these bounds if this is set to true. Usually this is fine and should be the - // recommended option. If sourceBounds are provided, they will restrict the used bounds (so it will just - // represent the bounds of the sliced part of the image). useTargetBounds: true, - - // {boolean} - If true, the created Image Node gets wrapped in an extra Node so that it can be transformed - // independently. If there is no need to transform the resulting node, wrap:false can be passed so that no extra - // Node is created. wrap: true, - - // {boolean} - If true, it will directly use the element (only works with canvas/webgl renderers) - // instead of converting this into a form that can be used with any renderer. May have slightly better - // performance if svg/dom renderers do not need to be used. useCanvas: false, - - // To be passed to the Image node created from the rasterization. See below for options that will override - // what is passed in. imageOptions: {} }, providedOptions ); From 6b80f11189f684d332881d1c3ad0f24e0bd45f2d Mon Sep 17 00:00:00 2001 From: Jonathan Olson Date: Mon, 11 Mar 2024 11:07:49 -0600 Subject: [PATCH 04/10] Adding ?forceSVGRefresh query parameter, see https://github.com/phetsims/scenery/issues/1507 --- js/display/Display.ts | 14 ++++++++++++++ js/display/SVGBlock.js | 27 +++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/js/display/Display.ts b/js/display/Display.ts index b2441dbd4..5786a797f 100644 --- a/js/display/Display.ts +++ b/js/display/Display.ts @@ -68,6 +68,7 @@ import UtteranceQueue from '../../../utterance-queue/js/UtteranceQueue.js'; import { BackboneDrawable, Block, CanvasBlock, CanvasNodeBoundsOverlay, ChangeInterval, Color, DOMBlock, DOMDrawable, Drawable, Features, FittedBlockBoundsOverlay, FocusManager, FullScreen, globalKeyStateTracker, HighlightOverlay, HitAreaOverlay, Input, InputOptions, Instance, KeyboardUtils, Node, PDOMInstance, PDOMSiblingStyle, PDOMTree, PDOMUtils, Pointer, PointerAreaOverlay, PointerOverlay, Renderer, scenery, SceneryEvent, scenerySerialize, SelfDrawable, TInputListener, TOverlay, Trail, Utils, WebGLBlock } from '../imports.js'; import TEmitter from '../../../axon/js/TEmitter.js'; import SafariWorkaroundOverlay from '../overlays/SafariWorkaroundOverlay.js'; +import TinyEmitter from '../../../axon/js/TinyEmitter.js'; type SelfOptions = { // Initial (or override) display width @@ -94,6 +95,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 +210,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 +310,9 @@ export default class Display { private perfDrawableOldIntervalCount?: number; private perfDrawableNewIntervalCount?: number; + // (scenery-internal) + public readonly frameEmitter = new TinyEmitter(); + /** * Constructs a Display that will show the rootNode and its subtree in a visual state. Default options provided below * @@ -332,6 +341,8 @@ export default class Display { allowLayerFitting: false, + forceSVGRefresh: false, + // {string} - What cursor is used when no other cursor is specified defaultCursor: 'default', @@ -439,6 +450,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 +714,8 @@ export default class Display { PDOMTree.auditPDOMDisplays( this.rootNode ); + this.frameEmitter.emit(); + sceneryLog && sceneryLog.Display && sceneryLog.pop(); } diff --git a/js/display/SVGBlock.js b/js/display/SVGBlock.js index cd7202253..e43dabd59 100644 --- a/js/display/SVGBlock.js +++ b/js/display/SVGBlock.js @@ -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,30 @@ 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 + if ( this.display._forceSVGRefresh && !this.forceRefreshListener ) { + + const workaroundGroup = document.createElementNS( svgns, 'g' ); + this.svg.appendChild( workaroundGroup ); + + const workaroundRect = document.createElementNS( svgns, 'rect' ); + workaroundRect.setAttribute( 'width', '0' ); + workaroundRect.setAttribute( 'height', '0' ); + workaroundRect.setAttribute( 'fill', 'none' ); + workaroundGroup.appendChild( workaroundRect ); + + // @private {function} - Forces a color change on the 0x0 rect + this.forceRefreshListener = () => { + const red = dotRandom.nextIntBetween( 0, 255 ); + const green = dotRandom.nextIntBetween( 0, 255 ); + const blue = dotRandom.nextIntBetween( 0, 255 ); + workaroundRect.setAttribute( 'fill', `rgba(${red},${green},${blue},0.02)` ); + }; + } + + this.display._forceSVGRefresh && this.display.frameEmitter.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 +392,8 @@ class SVGBlock extends FittedBlock { this.paintCountMap.clear(); + this.display._forceSVGRefresh && this.display.frameEmitter.removeListener( this.forceRefreshListener ); + this.baseTransformGroup.removeChild( this.rootGroup.svgGroup ); this.rootGroup.dispose(); this.rootGroup = null; From 4d9eaff95b6438b36a615a19beda51f5adf2afb8 Mon Sep 17 00:00:00 2001 From: phet-dev Date: Tue, 12 Mar 2024 03:22:30 -0600 Subject: [PATCH 05/10] update copyright dates from daily grunt work --- js/display/SVGBlock.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/display/SVGBlock.js b/js/display/SVGBlock.js index e43dabd59..d7a514380 100644 --- a/js/display/SVGBlock.js +++ b/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. From 3561b0fff76aa7cce63c26aed66c78b36044c15a Mon Sep 17 00:00:00 2001 From: Jonathan Olson Date: Wed, 13 Mar 2024 10:14:11 -0600 Subject: [PATCH 06/10] Adding Display.refreshSVG / Display.refreshSVGOnNextFrame, improved SVGBlock usage to support lazy creation, and SimDisplay chromium guard to refresh SVG during pan/zoom, see https://github.com/phetsims/scenery/issues/1507 --- js/display/Display.ts | 32 ++++++++++++++++++++++++++++---- js/display/SVGBlock.js | 41 ++++++++++++++++++++--------------------- 2 files changed, 48 insertions(+), 25 deletions(-) diff --git a/js/display/Display.ts b/js/display/Display.ts index 5786a797f..6d4e8e3e2 100644 --- a/js/display/Display.ts +++ b/js/display/Display.ts @@ -68,7 +68,6 @@ import UtteranceQueue from '../../../utterance-queue/js/UtteranceQueue.js'; import { BackboneDrawable, Block, CanvasBlock, CanvasNodeBoundsOverlay, ChangeInterval, Color, DOMBlock, DOMDrawable, Drawable, Features, FittedBlockBoundsOverlay, FocusManager, FullScreen, globalKeyStateTracker, HighlightOverlay, HitAreaOverlay, Input, InputOptions, Instance, KeyboardUtils, Node, PDOMInstance, PDOMSiblingStyle, PDOMTree, PDOMUtils, Pointer, PointerAreaOverlay, PointerOverlay, Renderer, scenery, SceneryEvent, scenerySerialize, SelfDrawable, TInputListener, TOverlay, Trail, Utils, WebGLBlock } from '../imports.js'; import TEmitter from '../../../axon/js/TEmitter.js'; import SafariWorkaroundOverlay from '../overlays/SafariWorkaroundOverlay.js'; -import TinyEmitter from '../../../axon/js/TinyEmitter.js'; type SelfOptions = { // Initial (or override) display width @@ -310,8 +309,12 @@ export default class Display { private perfDrawableOldIntervalCount?: number; private perfDrawableNewIntervalCount?: number; - // (scenery-internal) - public readonly frameEmitter = new TinyEmitter(); + // (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 @@ -714,7 +717,11 @@ export default class Display { PDOMTree.auditPDOMDisplays( this.rootNode ); - this.frameEmitter.emit(); + if ( this._forceSVGRefresh || this._refreshSVGPending ) { + this._refreshSVGPending = false; + + this.refreshSVG(); + } sceneryLog && sceneryLog.Display && sceneryLog.pop(); } @@ -2089,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. * diff --git a/js/display/SVGBlock.js b/js/display/SVGBlock.js index d7a514380..66b0c840a 100644 --- a/js/display/SVGBlock.js +++ b/js/display/SVGBlock.js @@ -86,27 +86,26 @@ class SVGBlock extends FittedBlock { // 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 - if ( this.display._forceSVGRefresh && !this.forceRefreshListener ) { - - const workaroundGroup = document.createElementNS( svgns, 'g' ); - this.svg.appendChild( workaroundGroup ); - - const workaroundRect = document.createElementNS( svgns, 'rect' ); - workaroundRect.setAttribute( 'width', '0' ); - workaroundRect.setAttribute( 'height', '0' ); - workaroundRect.setAttribute( 'fill', 'none' ); - workaroundGroup.appendChild( workaroundRect ); - - // @private {function} - Forces a color change on the 0x0 rect - this.forceRefreshListener = () => { - const red = dotRandom.nextIntBetween( 0, 255 ); - const green = dotRandom.nextIntBetween( 0, 255 ); - const blue = dotRandom.nextIntBetween( 0, 255 ); - workaroundRect.setAttribute( 'fill', `rgba(${red},${green},${blue},0.02)` ); - }; - } + // @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 ); + } - this.display._forceSVGRefresh && this.display.frameEmitter.addListener( this.forceRefreshListener ); + 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. @@ -392,7 +391,7 @@ class SVGBlock extends FittedBlock { this.paintCountMap.clear(); - this.display._forceSVGRefresh && this.display.frameEmitter.removeListener( this.forceRefreshListener ); + this.display._refreshSVGEmitter.removeListener( this.forceRefreshListener ); this.baseTransformGroup.removeChild( this.rootGroup.svgGroup ); this.rootGroup.dispose(); From 2cb5e061080246b4fa28b94cb9d063ff48e9e0f3 Mon Sep 17 00:00:00 2001 From: Michael Kauzmann Date: Thu, 14 Mar 2024 12:30:29 -0600 Subject: [PATCH 07/10] make voicingCanSpeakProperty private, fixes https://github.com/phetsims/scenery/issues/1618 Signed-off-by: Michael Kauzmann --- js/accessibility/voicing/Voicing.ts | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/js/accessibility/voicing/Voicing.ts b/js/accessibility/voicing/Voicing.ts index d013ac368..91edb7b22 100644 --- a/js/accessibility/voicing/Voicing.ts +++ b/js/accessibility/voicing/Voicing.ts @@ -114,7 +114,8 @@ const Voicing = >( Type: SuperType ) => { // private _voicingFocusListener!: SceneryListenerFunction | 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; // A counter that keeps track of visible and voicingVisible Instances of this Node. @@ -488,16 +489,6 @@ const Voicing = >( 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 { - return this._voicingCanSpeakProperty; - } - - public get voicingCanSpeakProperty() { return this.getVoicingCanSpeakProperty(); } - /** * Called whenever this Node is focused. */ @@ -675,8 +666,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 +680,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 ); }; From 41a608311d476156e894ea1f665fcb8b3af6fe19 Mon Sep 17 00:00:00 2001 From: Michael Kauzmann Date: Thu, 14 Mar 2024 12:36:25 -0600 Subject: [PATCH 08/10] clear on disposal instead of using a workaround, https://github.com/phetsims/scenery/issues/1615 https://github.com/phetsims/density-buoyancy-common/issues/97 Signed-off-by: Michael Kauzmann --- js/accessibility/FocusDisplayedController.ts | 24 ++++++++++++++----- .../voicing/InteractiveHighlighting.ts | 7 ++---- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/js/accessibility/FocusDisplayedController.ts b/js/accessibility/FocusDisplayedController.ts index a603026f6..05dbc7ff9 100644 --- a/js/accessibility/FocusDisplayedController.ts +++ b/js/accessibility/FocusDisplayedController.ts @@ -1,4 +1,4 @@ -// Copyright 2021-2024, University of Colorado Boulder +// Copyright 2021-2023, 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 | null; + private focusProperty: TProperty; // 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. @@ -85,6 +96,7 @@ class FocusDisplayedController { this.node = focus.trail.lastNode(); this.node.changedInstanceEmitter.addListener( this.boundInstancesChangedListener ); + this.node.disposeEmitter.addListener( this.boundNodeDisposedListener ); } /** @@ -99,6 +111,7 @@ class FocusDisplayedController { } if ( this.node ) { this.node.changedInstanceEmitter.removeListener( this.boundInstancesChangedListener ); + this.node.disposeEmitter.removeListener( this.boundNodeDisposedListener ); this.node = null; } } @@ -107,11 +120,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; } } diff --git a/js/accessibility/voicing/InteractiveHighlighting.ts b/js/accessibility/voicing/InteractiveHighlighting.ts index 4b3d06356..d4b0b6020 100644 --- a/js/accessibility/voicing/InteractiveHighlighting.ts +++ b/js/accessibility/voicing/InteractiveHighlighting.ts @@ -427,14 +427,11 @@ const InteractiveHighlighting = memoize( >( 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. From 9fccc539944a3cb89028bb8a2bc15cec43a11d75 Mon Sep 17 00:00:00 2001 From: Michael Kauzmann Date: Thu, 14 Mar 2024 12:47:15 -0600 Subject: [PATCH 09/10] update important doc about Node.changedInstanceEmitter, https://github.com/phetsims/scenery/issues/1615 Signed-off-by: Michael Kauzmann --- js/accessibility/FocusDisplayedController.ts | 3 +++ .../voicing/InteractiveHighlighting.ts | 3 +++ js/accessibility/voicing/Voicing.ts | 3 +++ js/nodes/Node.ts | 7 ++++++- js/util/DisplayedProperty.js | 16 ++++++++++------ 5 files changed, 25 insertions(+), 7 deletions(-) diff --git a/js/accessibility/FocusDisplayedController.ts b/js/accessibility/FocusDisplayedController.ts index 05dbc7ff9..b7e730f18 100644 --- a/js/accessibility/FocusDisplayedController.ts +++ b/js/accessibility/FocusDisplayedController.ts @@ -95,6 +95,9 @@ 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 ); } diff --git a/js/accessibility/voicing/InteractiveHighlighting.ts b/js/accessibility/voicing/InteractiveHighlighting.ts index d4b0b6020..0f8ae4def 100644 --- a/js/accessibility/voicing/InteractiveHighlighting.ts +++ b/js/accessibility/voicing/InteractiveHighlighting.ts @@ -105,6 +105,9 @@ const InteractiveHighlighting = memoize( >( }; 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 ); diff --git a/js/accessibility/voicing/Voicing.ts b/js/accessibility/voicing/Voicing.ts index 91edb7b22..356cc937b 100644 --- a/js/accessibility/voicing/Voicing.ts +++ b/js/accessibility/voicing/Voicing.ts @@ -167,6 +167,9 @@ const Voicing = >( 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 = { diff --git a/js/nodes/Node.ts b/js/nodes/Node.ts index 4a879462c..5ed12da26 100644 --- a/js/nodes/Node.ts +++ b/js/nodes/Node.ts @@ -641,7 +641,12 @@ class Node extends ParallelDOM { // Emitted to when we change filters (either opacity or generalized filters) public readonly filterChangeEmitter: TEmitter; - // Fired when an instance is changed (added/removed) + // Fired when an instance is changed (added/removed). CAREFUL!! This is potentially a very dangerous thing to listen + // to. Instances are updated in an asynchronous batch during `updateDisplay()`, and it is very important that display + // updates do not cause changes the scene graph. Thus, this emitter should NEVER trigger a Node's state to change. + // Currently, all usages of this cause into updates to the audio view, or updates to a separate display (used as an + // overlay). Please proceed with caution, and see https://github.com/phetsims/scenery/issues/1615 and + // https://github.com/phetsims/scenery/issues/1620 for details. public readonly changedInstanceEmitter: TEmitter<[ instance: Instance, added: boolean ]>; // Fired when layoutOptions changes diff --git a/js/util/DisplayedProperty.js b/js/util/DisplayedProperty.js index e43279802..058dc30a5 100644 --- a/js/util/DisplayedProperty.js +++ b/js/util/DisplayedProperty.js @@ -1,14 +1,18 @@ // Copyright 2018-2022, University of Colorado Boulder /** - * A property that is true when the node appears on the given display. + * A property that is true when the node appears on the given display. Please exercise extreme care when using this + * class, as it comes with some finicky drawbacks: * - * Note that a node can appear on a display even after it has been removed from the scene graph, if - * Display.updateDisplay has not yet been called since it was removed. So generally this Property will only update + * 1. Note that a node can appear on a display even after it has been removed from the scene graph, if + * Display.updateDisplay() has not yet been called since it was removed. So generally this Property will only update * as a result of Display.updateDisplay() being called. - * - * Be careful to dispose of these, since it WILL result in a permanent memory leak otherwise (Instance objects are - * pooled, and if the listener is not removed, it will stay around forever). + * 2. Given (1), this means that any listeners to this Property will fire during updateDisplay(), and not during normal + * stepping/event handling of the simulation. updateDisplay should NEVER cause the scene graph state to change, so + * listeners to this property should not cause changes to Node state, see https://github.com/phetsims/scenery/issues/1615 + * for details. + * 3. Be careful to dispose of these, since it WILL result in a permanent memory leak otherwise. Instance objects are + * pooled, and if the listener is not removed, it will stay around forever. * * @author Jonathan Olson */ From 131daef9d8f0aa1fb70370ee1fedbd5cc8d873b6 Mon Sep 17 00:00:00 2001 From: phet-dev Date: Fri, 15 Mar 2024 03:23:01 -0600 Subject: [PATCH 10/10] update copyright dates from daily grunt work --- js/accessibility/FocusDisplayedController.ts | 2 +- js/accessibility/voicing/Voicing.ts | 2 +- js/util/DisplayedProperty.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/js/accessibility/FocusDisplayedController.ts b/js/accessibility/FocusDisplayedController.ts index b7e730f18..8c6dc0d1a 100644 --- a/js/accessibility/FocusDisplayedController.ts +++ b/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 diff --git a/js/accessibility/voicing/Voicing.ts b/js/accessibility/voicing/Voicing.ts index 356cc937b..314d1c0df 100644 --- a/js/accessibility/voicing/Voicing.ts +++ b/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 diff --git a/js/util/DisplayedProperty.js b/js/util/DisplayedProperty.js index 058dc30a5..736c95846 100644 --- a/js/util/DisplayedProperty.js +++ b/js/util/DisplayedProperty.js @@ -1,4 +1,4 @@ -// Copyright 2018-2022, University of Colorado Boulder +// Copyright 2018-2024, University of Colorado Boulder /** * A property that is true when the node appears on the given display. Please exercise extreme care when using this