Skip to content

Commit

Permalink
Initial work for removing Events .on()/.onStatic()/.off()/.offStatic(…
Browse files Browse the repository at this point in the history
…), see #490
  • Loading branch information
jonathanolson committed Apr 9, 2020
1 parent 928e260 commit 4a1fe16
Show file tree
Hide file tree
Showing 38 changed files with 517 additions and 519 deletions.
4 changes: 2 additions & 2 deletions js/accessibility/pdom/PDOMDisplaysInfo.js
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ export default inherit( Object, PDOMDisplaysInfo, {
}
}

this.node.trigger0( 'accessibleDisplays' );
this.node.accessibleDisplaysEmitter.emit();
}

sceneryLog && sceneryLog.PDOMDisplaysInfo && sceneryLog.pop();
Expand Down Expand Up @@ -306,7 +306,7 @@ export default inherit( Object, PDOMDisplaysInfo, {
}
}

this.node.trigger0( 'accessibleDisplays' );
this.node.accessibleDisplaysEmitter.emit();
}

sceneryLog && sceneryLog.PDOMDisplaysInfo && sceneryLog.pop();
Expand Down
1 change: 1 addition & 0 deletions js/accessibility/pdom/PDOMInputTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ QUnit.test( 'focusin/focusout (focus/blur)', assert => {
} );

a.focus();
debugger;
assert.ok( aGotFocus, 'a should have been focused' );
assert.ok( !aLostFocus, 'a should not blur' );

Expand Down
9 changes: 3 additions & 6 deletions js/accessibility/pdom/PDOMInstance.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
* @author Jonathan Olson <[email protected]>
*/

import Events from '../../../../axon/js/Events.js';
import cleanArray from '../../../../phet-core/js/cleanArray.js';
import inherit from '../../../../phet-core/js/inherit.js';
import platform from '../../../../phet-core/js/platform.js';
Expand All @@ -54,7 +53,7 @@ function PDOMInstance( parent, display, trail ) {

scenery.register( 'PDOMInstance', PDOMInstance );

inherit( Events, PDOMInstance, {
inherit( Object, PDOMInstance, {

/**
* Initializes an PDOMInstance, implements construction for pooling.
Expand All @@ -66,8 +65,6 @@ inherit( Events, PDOMInstance, {
* @returns {PDOMInstance} - Returns 'this' reference, for chaining
*/
initializeAccessibleInstance: function( parent, display, trail ) {
Events.call( this ); // TODO: is Events worth mixing in by default? Will we need to listen to events?

assert && assert( !this.id || this.isDisposed, 'If we previously existed, we need to have been disposed' );

// unique ID
Expand Down Expand Up @@ -150,7 +147,7 @@ inherit( Events, PDOMInstance, {
}

const listener = this.checkAccessibleDisplayVisibility.bind( this, i - parentTrail.length );
relativeNode.onStatic( 'accessibleDisplays', listener );
relativeNode.accessibleDisplaysEmitter.addListener( listener );
this.relativeListeners.push( listener );
}

Expand Down Expand Up @@ -492,7 +489,7 @@ inherit( Events, PDOMInstance, {
PDOMUtils.removeElements( this.parent.peer.primarySibling, this.peer.topLevelElements );

for ( let i = 0; i < this.relativeNodes.length; i++ ) {
this.relativeNodes[ i ].offStatic( 'accessibleDisplays', this.relativeListeners[ i ] );
this.relativeNodes[ i ].accessibleDisplaysEmitter.removeListener( this.relativeListeners[ i ] );
}
}

Expand Down
4 changes: 2 additions & 2 deletions js/accessibility/pdom/PDOMPeer.js
Original file line number Diff line number Diff line change
Expand Up @@ -673,8 +673,8 @@ inherit( Object, PDOMPeer, {
* @returns {boolean}
*/
isFocused: function() {
const visualFocusTrail = phet.scenery.PDOMInstance.guessVisualTrail( this.trail, this.display.rootNode );
return phet.scenery.Display.focusProperty.value && phet.scenery.Display.focusProperty.value.trail.equals( visualFocusTrail );
const visualFocusTrail = scenery.PDOMInstance.guessVisualTrail( this.trail, this.display.rootNode );
return scenery.Display.focusProperty.value && scenery.Display.focusProperty.value.trail.equals( visualFocusTrail );
},

/**
Expand Down
7 changes: 3 additions & 4 deletions js/accessibility/pdom/ParallelDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -1443,7 +1443,7 @@ const ParallelDOM = {
focusHighlight.visible = this.focused;
}

this.trigger( 'focusHighlightChanged' );
this.focusHighlightChangedEmitter.emit();
}
},
set focusHighlight( focusHighlight ) { this.setFocusHighlight( focusHighlight ); },
Expand Down Expand Up @@ -2049,7 +2049,7 @@ const ParallelDOM = {

PDOMTree.accessibleOrderChange( this, oldAccessibleOrder, accessibleOrder );

this.trigger0( 'accessibleOrder' );
this.accessibleOrderEmitter.emit();
}
},
set accessibleOrder( value ) { this.setAccessibleOrder( value ); },
Expand Down Expand Up @@ -2633,8 +2633,7 @@ const ParallelDOM = {
// recompute the heading level for this node if it is using the accessibleHeading API.
this._accessibleHeading && this.computeHeadingLevel();


this.trigger0( 'accessibleContent' );
this.accessibleContentEmitter.emit();
},

/**
Expand Down
12 changes: 6 additions & 6 deletions js/display/BackboneDrawable.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ inherit( Drawable, BackboneDrawable, {
}

this.backboneVisibilityListener = this.backboneVisibilityListener || this.updateBackboneVisibility.bind( this );
this.backboneInstance.onStatic( 'relativeVisibility', this.backboneVisibilityListener );
this.backboneInstance.relativeVisibleEmitter.addListener( this.backboneVisibilityListener );
this.updateBackboneVisibility();
this.visibilityDirty = true;

Expand All @@ -102,8 +102,8 @@ inherit( Drawable, BackboneDrawable, {
const node = instance.node;

this.watchedFilterNodes.push( node );
node.onStatic( 'opacity', this.opacityDirtyListener );
node.onStatic( 'clip', this.clipDirtyListener );
node.opacityProperty.lazyLink( this.opacityDirtyListener );
node.clipAreaProperty.lazyLink( this.clipDirtyListener );
}
}

Expand Down Expand Up @@ -134,11 +134,11 @@ inherit( Drawable, BackboneDrawable, {
while ( this.watchedFilterNodes.length ) {
const node = this.watchedFilterNodes.pop();

node.offStatic( 'opacity', this.opacityDirtyListener );
node.offStatic( 'clip', this.clipDirtyListener );
node.opacityProperty.unlink( this.opacityDirtyListener );
node.clipAreaProperty.unlink( this.clipDirtyListener );
}

this.backboneInstance.offStatic( 'relativeVisibility', this.backboneVisibilityListener );
this.backboneInstance.relativeVisibleEmitter.removeListener( this.backboneVisibilityListener );

// if we need to remove drawables from the blocks, do so
if ( !this.removedDrawables ) {
Expand Down
8 changes: 4 additions & 4 deletions js/display/CanvasBlock.js
Original file line number Diff line number Diff line change
Expand Up @@ -420,8 +420,8 @@ inherit( FittedBlock, CanvasBlock, {
else {
this.filterListenerCountMap[ node.id ] = 1;

node.onStatic( 'opacity', this.opacityDirtyListener );
node.onStatic( 'clip', this.clipDirtyListener );
node.opacityProperty.lazyLink( this.opacityDirtyListener );
node.clipAreaProperty.lazyLink( this.clipDirtyListener );
}
}
},
Expand All @@ -437,8 +437,8 @@ inherit( FittedBlock, CanvasBlock, {
if ( this.filterListenerCountMap[ node.id ] === 0 ) {
delete this.filterListenerCountMap[ node.id ];

node.offStatic( 'clip', this.clipDirtyListener );
node.offStatic( 'opacity', this.opacityDirtyListener );
node.clipAreaProperty.unlink( this.clipDirtyListener );
node.opacityProperty.unlink( this.opacityDirtyListener );
}
}

Expand Down
84 changes: 39 additions & 45 deletions js/display/Display.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,10 @@
*/

import Emitter from '../../../axon/js/Emitter.js';
import Events from '../../../axon/js/Events.js';
import Property from '../../../axon/js/Property.js';
import PropertyIO from '../../../axon/js/PropertyIO.js';
import timer from '../../../axon/js/timer.js';
import TinyProperty from '../../../axon/js/TinyProperty.js';
import Dimension2 from '../../../dot/js/Dimension2.js';
import Matrix3 from '../../../dot/js/Matrix3.js';
import escapeHTML from '../../../phet-core/js/escapeHTML.js';
Expand Down Expand Up @@ -119,9 +119,6 @@ function Display( rootNode, options ) {

//OHTWO TODO: hybrid batching (option to batch until an event like 'up' that might be needed for security issues)

// supertype call to axon.Events (should just initialize a few properties here, notably _eventListeners and _staticEventListeners)
Events.call( this );

options = merge( {
// initial display width
width: ( options && options.container && options.container.clientWidth ) || 640,
Expand Down Expand Up @@ -188,8 +185,11 @@ function Display( rootNode, options ) {

this._allowWebGL = options.allowWebGL;

// The (integral, > 0) dimensions of the Display's DOM element (only updates the DOM element on updateDisplay())
this._size = new Dimension2( this.options.width, this.options.height );
// @public {Property.<Dimension2>} The (integral, > 0) dimensions of the Display's DOM element (only updates the DOM
// element on updateDisplay())
this.sizeProperty = new TinyProperty( new Dimension2( this.options.width, this.options.height ), {
useDeepEquality: true
} );
this._currentSize = new Dimension2( -1, -1 ); // used to check against new size to see what we need to change

this._rootNode = rootNode;
Expand Down Expand Up @@ -533,20 +533,20 @@ inherit( Object, Display, extend( {
updateSize: function() {
let sizeDirty = false;
//OHTWO TODO: if we aren't clipping or setting background colors, can we get away with having a 0x0 container div and using absolutely-positioned children?
if ( this._size.width !== this._currentSize.width ) {
if ( this.size.width !== this._currentSize.width ) {
sizeDirty = true;
this._currentSize.width = this._size.width;
this._domElement.style.width = this._size.width + 'px';
this._currentSize.width = this.size.width;
this._domElement.style.width = this.size.width + 'px';
}
if ( this._size.height !== this._currentSize.height ) {
if ( this.size.height !== this._currentSize.height ) {
sizeDirty = true;
this._currentSize.height = this._size.height;
this._domElement.style.height = this._size.height + 'px';
this._currentSize.height = this.size.height;
this._domElement.style.height = this.size.height + 'px';
}
if ( sizeDirty && !this.options.allowSceneOverflow ) {
// to prevent overflow, we add a CSS clip
//TODO: 0px => 0?
this._domElement.style.clip = 'rect(0px,' + this._size.width + 'px,' + this._size.height + 'px,0px)';
this._domElement.style.clip = 'rect(0px,' + this.size.width + 'px,' + this.size.height + 'px,0px)';
}
},

Expand All @@ -557,12 +557,12 @@ inherit( Object, Display, extend( {

// The dimensions of the Display's DOM element
getSize: function() {
return this._size;
return this.sizeProperty.value;
},
get size() { return this.getSize(); },

getBounds: function() {
return this._size.toBounds();
return this.size.toBounds();
},
get bounds() { return this.getBounds(); },

Expand All @@ -574,11 +574,7 @@ inherit( Object, Display, extend( {
assert && assert( size.height % 1 === 0, 'Display.height should be an integer' );
assert && assert( size.height > 0, 'Display.height should be greater than zero' );

if ( !this._size.equals( size ) ) {
this._size = size;

this.trigger1( 'displaySize', this._size );
}
this.sizeProperty.value = size;
},

setWidthHeight: function( width, height ) {
Expand All @@ -588,7 +584,7 @@ inherit( Object, Display, extend( {

// The width of the Display's DOM element
getWidth: function() {
return this._size.width;
return this.size.width;
},
get width() { return this.getWidth(); },

Expand All @@ -605,7 +601,7 @@ inherit( Object, Display, extend( {

// The height of the Display's DOM element
getHeight: function() {
return this._size.height;
return this.size.height;
},
get height() { return this.getHeight(); },

Expand Down Expand Up @@ -901,8 +897,8 @@ inherit( Object, Display, extend( {
// renders what it can into a Canvas (so far, Canvas and SVG layers work fine)
canvasSnapshot: function( callback ) {
const canvas = document.createElement( 'canvas' );
canvas.width = this._size.width;
canvas.height = this._size.height;
canvas.width = this.size.width;
canvas.height = this.size.height;

const context = canvas.getContext( '2d' );

Expand Down Expand Up @@ -1215,7 +1211,7 @@ inherit( Object, Display, extend( {
let result = '';

result += '<div style="' + headerStyle + '">Display Summary</div>';
result += this._size.toString() + ' frame:' + this._frameId + ' input:' + !!this._input + ' cursor:' + this._lastCursor + '<br/>';
result += this.size.toString() + ' frame:' + this._frameId + ' input:' + !!this._input + ' cursor:' + this._lastCursor + '<br/>';

function nodeCount( node ) {
let count = 1; // for us
Expand Down Expand Up @@ -1769,8 +1765,25 @@ inherit( Object, Display, extend( {

popupRasterization: function() {
this.foreignObjectRasterization( window.open );
},

/**
* Releases references.
* @public
*
* TODO: this dispose function is not complete.
*/
dispose: function() {
if ( this._input ) {
this.detachEvents();
}
this._rootNode.removeRootedDisplay( this );

if ( this._accessible ) {
this._rootAccessibleInstance.dispose();
}
}
}, Events.prototype ), {
} ), {
/**
* Takes a given DOM element, and asynchronously renders it to a string that is a data URL representing an SVG
* file.
Expand Down Expand Up @@ -1866,25 +1879,6 @@ inherit( Object, Display, extend( {
get focusedNode() { return this.getFocusedNode(); }
} );

/**
* Dispose function for Display.
*
* TODO: this dispose function is not complete.
* TODO: Don't require overriding like this. Events prototype and non-standard inheritance forces us right now, but
* ideally we'll stop using Events for Display and this should just work.
* @public
*/
Display.prototype.dispose = function() {
if ( this._input ) {
this.detachEvents();
}
this._rootNode.removeRootedDisplay( this );

if ( this._accessible ) {
this._rootAccessibleInstance.dispose();
}
};

Display.customCursors = {
'scenery-grab-pointer': [ 'grab', '-moz-grab', '-webkit-grab', 'pointer' ],
'scenery-grabbing-pointer': [ 'grabbing', '-moz-grabbing', '-webkit-grabbing', 'pointer' ]
Expand Down
Loading

0 comments on commit 4a1fe16

Please sign in to comment.