Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: define options.passive as enumerable for compatibility #520

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

idiotWu
Copy link
Owner

@idiotWu idiotWu commented Mar 22, 2023

Fixes #519
CC @ajiho

Description

In the default-passive-event package, they copy the original event listener options using Object.assign({}, options). However, since we define the options.passive as non-enumerable property, this property will not be accessed in their monkey patch, and as a result, our code will assume that the browser does not support passive events according to the following detection procedure:

function getOptions(): typeof eventListenerOptions {
if (eventListenerOptions !== undefined) {
return eventListenerOptions;
}
let supportPassiveEvent = false;
try {
const noop = () => {};
const options = Object.defineProperty({}, 'passive', {
get() {
supportPassiveEvent = true;
},
});
window.addEventListener('testPassive', noop, options);
window.removeEventListener('testPassive', noop, options);
} catch (e) {}
eventListenerOptions = supportPassiveEvent ? { passive: false } as EventListenerOptions : false;
return eventListenerOptions;
}

This modification defines options.passive as enumerable to ensure compatibility with the default-passive-event package.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Fixes #519:
This modification ensures compatibility with the `default-passive-event`
package.
@sadeghbarati
Copy link
Collaborator

Hi @idiotWu

I think enumerable is not needed, or it should be a separate utility function

https://github.com/Modernizr/Modernizr/blob/86ebb0447a8f698b22320b2bb560968050d47cf7/feature-detects/dom/passiveeventlisteners.js

@idiotWu
Copy link
Owner Author

idiotWu commented Mar 22, 2023

@sadeghbarati
enumerable: true here is only for compatibility with other packages like default-passive-event, and it does no harm to our code.

@idiotWu idiotWu merged commit 8f060fe into develop Mar 23, 2023
@idiotWu idiotWu deleted the fix-passive-event branch March 23, 2023 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to preventDefault inside passive event listener invocation.
2 participants