NoriginMedia / react-spatial-navigation

DEPRECATED. HOC-based Spatial Navigation. NEW Hooks version is available here: https://github.com/NoriginMedia/norigin-spatial-navigation
MIT License
226 stars 64 forks source link

The third parameter to `addEventListener` #37

Closed yarikleto closed 4 years ago

yarikleto commented 5 years ago

An old browser (Firefox 5) throws an exception if addEventListener doesn't have the third parameter (false). I use the old Smart TV (Orsay)

MDN: https://developer.mozilla.org/ru/docs/Web/API/EventTarget/addEventListener

TODO: I can add a new field to the init (private of initNavigation) method, like listenerOptions (or useCapture) and pass it to

init({
    debug: debug = false,
    visualDebug: visualDebug = false,
    nativeMode: nativeMode = false,
    throttle: throttle = 0,
    listenerOptions = false
  } = {}) {
   // Rest code
  }

window.addEventListener('keyup', this.keyUpEventListener, listenerOptions);
window.addEventListener('keydown', this.keyDownEventListener, listenerOptions);

What do you think about it?

asgvard commented 5 years ago

Hi,

The third parameter should be optional? At least according to MDN. Not sure why it throws an exception in this case. But perhaps you can add this if this is an issue. Just maybe to name it like "keyListenerOptions", because from the init method prospective it's not clear which listeners is it related to (even though it's only keys).

yarikleto commented 5 years ago

For old browsers, the third parameter is required. If the addEventListener and removeEventListener (sometimes) don't have the third parameter, a browser throws an exception.

shirakaba commented 4 years ago

@alfimois If we're going to provide keyListenerOptions for the sake of backwards compatibility, it shouldn't strictly be implemented as a boolean (that would better be called keyListenerUseCapture). It would be better to implement it as an options object (for the sake of newer browsers) that falls back to boolean if necessary (for older browsers):

https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Safely_detecting_option_support

In older versions of the DOM specification, the third parameter of addEventListener() was a Boolean value indicating whether or not to use capture. Over time, it became clear that more options were needed. Rather than adding more parameters to the function (complicating things enormously when dealing with optional values), the third parameter was changed to an object which can contain various properties defining the values of options to configure the process of removing the event listener.

Because older browsers (as well as some not-too-old browsers) still assume the third parameter is a Boolean, you need to build your code to handle this scenario intelligently. You can do this by using feature detection for each of the options you're interested in.

I don't know what the default (backwards-compatible) options for each property on a keyListenerOptions object would be, however 😕

shirakaba commented 4 years ago

@alfimois Nonetheless, in case you need it, I've created a branch from v2.9.2 that implements keyListenerUseCapture: https://github.com/shirakaba/react-spatial-navigation/tree/shirakaba/listener-use-capture. I won't be maintaining this branch to keep it up-to-date with upstream updates, however.

You can install it with:

npm install --save "git+ssh://git@github.com:shirakaba/react-spatial-navigation.git#shirakaba/listener-use-capture"

# or with yarn:

yarn add "git+ssh://git@github.com:shirakaba/react-spatial-navigation.git#shirakaba/listener-use-capture"
yarikleto commented 4 years ago

Oh, thanks:D