Mute event listener logging #2251
Conversation
|
Ah, I see - so basically this is just attaching the event in a different way that allows us to tell the browser "yes, we know this isn't passive, stop bugging us"? I like the |
src/plots/cartesian/dragbox.js
Outdated
| @@ -434,9 +434,14 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) { | |||
|
|
|||
| // everything but the corners gets wheel zoom | |||
| if(ns.length * ew.length !== 1) { | |||
| // still seems to be some confusion about onwheel vs onmousewheel... | |||
| if(dragger.onwheel !== undefined) dragger.onwheel = zoomWheel; | |||
| else if(dragger.onmousewheel !== undefined) dragger.onmousewheel = zoomWheel; | |||
There was a problem hiding this comment.
Can we keep that onmousewheel handler in case we need it in older browsers?
There was a problem hiding this comment.
I think the only difference between what @dfcreative has here and what we had before is that now we always attach to either wheel or mousewheel, whereas before we could potentially attach to neither if the browser doesn't support either one (I had to play with this - you may already know this but what's going on is if the browser supports wheel, then element.onwheel = null before any handler is attached, not undefined). And even if there is a case where the browser really doesn't support either, there shouldn't be any problem attaching to a nonexistent event.
|
@etpinard resolved |
src/plots/cartesian/dragbox.js
Outdated
| } | ||
| element._onwheel = handler; | ||
|
|
||
| element.addEventListener(wheelEventName, handler, {passive: false}); |
There was a problem hiding this comment.
Unfortunately, this might break older browsers, see:
I couldn't find a list of which older browsers does not support {passive: true|false}, but we should probably play it safe here.
There was a problem hiding this comment.
Good catch @etpinard - do we need a has-passive ala https://github.com/dfcreative/has-hover? Or even better, a module that would figure this out once and then export the correct 3rd arg to mark the listener active or passive:
var eventOpts = require('event-opts');
element.addEventListener(wheelEventName, handler, eventOpts.active); // false or {passive: false}
element.addEventListener(wheelEventName, handler, eventOpts.passive); // false or {passive: true}There was a problem hiding this comment.
There is a polyfill default-passive-events for that. We could use detection logic from there in a way @alexcjohnson suggests.
There was a problem hiding this comment.
@etpinard @alexcjohnson a32a21a introduces eventListenerOptionsSupported method to detect proper way to attach events.
To make code cleaner it is possible to introduce an event binding function or use package, like emmy or similar.
|
Nicely done. Merge away (when you get the chance) 💃 |
|
Dima must be travelling today. Merging this thing so that it's part of |
Fixes #1795