Skip to content

Commit

Permalink
Better handling for passiveEvents, adding null so we can use browse…
Browse files Browse the repository at this point in the history
…r defaults. See #770
  • Loading branch information
jonathanolson committed Apr 19, 2018
1 parent 471f3ad commit 3aa96cd
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 12 deletions.
15 changes: 12 additions & 3 deletions js/display/Display.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ define( function( require ) {
var FittedBlockBoundsOverlay = require( 'SCENERY/overlays/FittedBlockBoundsOverlay' );
var FocusIO = require( 'SCENERY/accessibility/FocusIO' );
var FocusOverlay = require( 'SCENERY/overlays/FocusOverlay' );
var platform = require( 'PHET_CORE/platform' );
var PointerAreaOverlay = require( 'SCENERY/overlays/PointerAreaOverlay' );
var PointerOverlay = require( 'SCENERY/overlays/PointerOverlay' );
var SceneryStyle = require( 'SCENERY/util/SceneryStyle' );
Expand Down Expand Up @@ -160,9 +161,17 @@ define( function( require ) {
// See https://github.com/phetsims/scenery/issues/347.
aggressiveContextRecreation: true,

// {boolean} - Whether the `passive` flag should be set when adding and removing DOM event listeners.
// {boolean|null} - Whether the `passive` flag should be set when adding and removing DOM event listeners.
// See https://github.com/phetsims/scenery/issues/770 for more details.
passiveEvents: true
// If it is true or false, that is the value of the passive flag that will be used. If it is null, the default
// behavior of the browser will be used.
//
// Safari doesn't support touch-action: none, so we NEED to not use passive events (which would not allow
// preventDefault to do anything, so drags actually can scroll the sim).
// Chrome also did the same "passive by default", but because we have `touch-action: none` in place, it doesn't
// affect us, and we can potentially get performance improvements by allowing passive events.
// See https://github.com/phetsims/scenery/issues/770 for more information.
passiveEvents: platform.safari ? false : null
}, options );

// TODO: don't store the options, it's an anti-pattern.
Expand Down Expand Up @@ -225,7 +234,7 @@ define( function( require ) {
this._listenToOnlyElement = options.listenToOnlyElement; // TODO: doc
this._batchDOMEvents = options.batchDOMEvents; // TODO: doc
this._assumeFullWindow = options.assumeFullWindow; // TODO: doc
this._passiveEvents = options.passiveEvents; // TODO: doc
this._passiveEvents = options.passiveEvents; // @private {boolean|null}

// @public (scenery-internal) {boolean}
this._aggressiveContextRecreation = options.aggressiveContextRecreation;
Expand Down
20 changes: 12 additions & 8 deletions js/input/BrowserEvents.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ define( function( require ) {
* @param {Display} display
* @param {boolean} attachToWindow - Whether events should be attached to the window. If false, they will be
* attached to the Display's domElement.
* @param {boolean} passiveEvents - The value of the `passive` option for adding/removing DOM event listeners
* @param {boolean|null} passiveEvents - The value of the `passive` option for adding/removing DOM event listeners
*/
addDisplay: function( display, attachToWindow, passiveEvents ) {
assert && assert( display instanceof scenery.Display );
Expand Down Expand Up @@ -55,7 +55,7 @@ define( function( require ) {
*
* @param {Display} display
* @param {boolean} attachToWindow - The value provided to addDisplay
* @param {boolean} passiveEvents - The value of the `passive` option for adding/removing DOM event listeners
* @param {boolean|null} passiveEvents - The value of the `passive` option for adding/removing DOM event listeners
*/
removeDisplay: function( display, attachToWindow, passiveEvents ) {
assert && assert( display instanceof scenery.Display );
Expand Down Expand Up @@ -203,7 +203,7 @@ define( function( require ) {
* Connects event listeners directly to the window.
* @private
*
* @param {boolean} passiveEvents - The value of the `passive` option for adding/removing DOM event listeners
* @param {boolean|null} passiveEvents - The value of the `passive` option for adding/removing DOM event listeners
*/
connectWindowListeners: function( passiveEvents ) {
this.addOrRemoveListeners( window, true, passiveEvents );
Expand All @@ -213,7 +213,7 @@ define( function( require ) {
* Disconnects event listeners from the window.
* @private
*
* @param {boolean} passiveEvents - The value of the `passive` option for adding/removing DOM event listeners
* @param {boolean|null} passiveEvents - The value of the `passive` option for adding/removing DOM event listeners
*/
disconnectWindowListeners: function( passiveEvents ) {
this.addOrRemoveListeners( window, false, passiveEvents );
Expand All @@ -225,11 +225,13 @@ define( function( require ) {
*
* @param {*} element - The element (window or DOM element) to add listeners to.
* @param {boolean} addOrRemove - If true, listeners will be added. If false, listeners will be removed.
* @param {boolean} passiveEvents - The value of the `passive` option for adding/removing DOM event listeners
* @param {boolean|null} passiveEvents - The value of the `passive` option for adding/removing DOM event listeners
* NOTE: if it is passed in as null, the default value for the browser will be
* used.
*/
addOrRemoveListeners: function( element, addOrRemove, passiveEvents ) {
assert && assert( typeof addOrRemove === 'boolean' );
assert && assert( typeof passiveEvents === 'boolean' );
assert && assert( typeof passiveEvents === 'boolean' || passiveEvents === null );

var forWindow = element === window;
assert && assert( !forWindow || ( this.listenersAttachedToWindow > 0 ) === !addOrRemove,
Expand All @@ -250,6 +252,8 @@ define( function( require ) {
// {Array.<string>}
var eventTypes = this.getNonWheelUsedTypes();

var passDirectPassiveFlag = Features.passive && passiveEvents !== null;

for ( var i = 0; i < eventTypes.length; i++ ) {
var type = eventTypes[ i ];

Expand All @@ -258,7 +262,7 @@ define( function( require ) {
if ( forWindow ) {
// Workaround for older browsers needed,
// see https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Improving_scrolling_performance_with_passive_listeners
var documentOptions = Features.passive ? { passive: passiveEvents } : false;
var documentOptions = passDirectPassiveFlag ? { passive: passiveEvents } : false;
document[ method ]( type, noop, documentOptions );
}

Expand All @@ -267,7 +271,7 @@ define( function( require ) {

// Workaround for older browsers needed,
// see https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Improving_scrolling_performance_with_passive_listeners
var mainOptions = Features.passive ? {
var mainOptions = passDirectPassiveFlag ? {
useCapture: false,
passive: passiveEvents
} : false;
Expand Down
2 changes: 1 addition & 1 deletion js/input/Input.js
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ define( function( require ) {
// http://www.html5rocks.com/en/mobile/touchandmouse/ for more information.
// Additionally, IE had some issues with skipping prevent default, see
// https://github.com/phetsims/scenery/issues/464 for mouse handling.
if ( !this.passiveEvents && ( callback !== this.mouseDown || platform.ie || platform.edge ) ) {
if ( !( this.passiveEvents === true ) && ( callback !== this.mouseDown || platform.ie || platform.edge ) ) {
// We cannot prevent a passive event, so don't try
domEvent.preventDefault();
}
Expand Down

0 comments on commit 3aa96cd

Please sign in to comment.