search

Taylor-CCB-Group / MDV

GNU General Public License v3.0
9 stars 6 forks source link

ScatterPlot zoom needs fixing #78

Closed xinaesthete closed 4 months ago

xinaesthete commented 4 months ago

This got lost in the noise somewhere while applying biome linting etc.

There was at one point an issue with events not working properly in popouts. This was at one point (recently) fixed by being more careful about use of arrow functions & this, but the current version of the code I have in pjt-dev has an empty object for this.handlers.pan_or_zoom in WGL2DI.js:1383, meaning that mouse-wheel zoom doesn't work even in non-popout chart.

xinaesthete commented 4 months ago

Failed to reproduce the above conditions immediately after writing that... pan_or_zoom being empty did not prevent wheel zoom. It works before popping out, breaks when popped out and isn't repaired by popping back in.

Something must be wrong in the process of Chart.changeBaseDocument(). The element this.div_container should be moved along with its parents, but something is going wrong.

xinaesthete commented 4 months ago

Seems to be Chrome-specific.

martinSergeant commented 4 months ago

On my branch its only just started doing this and only in chrome. Can be fixed by passing passive:true as options when adding the wheel event listener. I have noticed a few anomalies in chrome recently - canvas had become really slow when drawing many items but adding willReadFrequently:true as an option when getting the context seemed to fix it

xinaesthete commented 4 months ago

Thanks @martinSergeant - only just noticed your comment, and am looking into it now.

I don't find an user-appreciable difference passing {passive: true} (or false) - but I've now noticed that I'm getting an error

Unable to preventDefault inside passive event listener invocation.

That appears even without the options object, {passive: false} gets rid of it. Chrome reports it as an error, Firefox as a warning, and Safari doesn't seem to care (as of this writing).

xinaesthete commented 4 months ago

I think ultimately the approach I take to this will be to refactor scatter-plots to use deck.gl - the WGL2DI zoom code interacts badly with the way macos trackpad synthesises events for momentum, and I think there'll be some other benefits as well, I've been procrastinating about this for ages.

martinSergeant commented 4 months ago

On windows in chrome, the wheel event works when it is popped out , but does not work when popped back in again (although will still work if popped out again) When the listener is constructed with passive: true (by default in my chrome setup passive is false) this behaviour does not happen but preventDefault throws an error hence scrolling of the parent element happens as well Removing and re-adding the handler in changeBaseDocument does fix the problem but is a bit of a hack