bbc / peaks.js

JavaScript UI component for interacting with audio waveforms
https://waveform.prototyping.bbc.co.uk
GNU Lesser General Public License v3.0
3.16k stars 277 forks source link

Marking event listeners as passive (or not) to get rid of a violation warning #459

Closed suterma closed 1 year ago

suterma commented 1 year ago

To optimize scrolling performance, passive event listeners can be declared with an option. Currently, the declaration is missing, and a warning is displayed in recent Google Chrome browsers. See the below example from the https://candeogi.github.io/peaks-vue-demo/ by Giovanni Candeo.

Screenshot from 2022-08-22 22-58-53

To get rid of the warning, the appropriate value for the option should be chosen when the event listener is added.

Example (from https://github.com/slipmatio/control-knob/blob/main/src/ControlKnob.vue#L278)

  element.addEventListener('wheel', wheelListener, { passive: true /*or false, depending on requirements*/ })

For peaks.js I would guess that touch or mouse events should be passive in this sense, in case the intention was to NOT scroll the page, but the waveform.

chrisn commented 1 year ago

Thank you for mentioning this. The warnings come from Konva, and related issues have been raised there: https://github.com/konvajs/konva/issues/260, https://github.com/konvajs/konva/issues/1382. In some cases Peaks.js does allow the page to scroll, this is discussed in https://github.com/bbc/peaks.js/issues/454.

suterma commented 1 year ago

I now have raised the issue directly on the konvajs repo and will therefore close this issue here.

chrisn commented 1 year ago

Thanks. There are some places where Peaks.js calls window.addEventListener that may be worth looking at: https://github.com/bbc/peaks.js/blob/master/src/mouse-drag-handler.js#L94

suterma commented 1 year ago

Ok, then it might be a good idea to keep this open too. I'll leave it to you as the maintainer to proceed with this issue. I am currently not able to contribute code in a meaningful way here.