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

Bug: React + Scatterplot subscribe listeners don't register reliably on refresh. #112

Closed
NOT-HAL9000 opened this issue Apr 5, 2023 · 7 comments · Fixed by #114
Closed
Labels
bug Something isn't working

Comments

@NOT-HAL9000
Copy link

NOT-HAL9000 commented Apr 5, 2023

Introduction

I've been trying to create a custom scatterplot hook that I can drag and drop into my project.

I have it all working however I keep running into a weird event listener bug. I have tried various implementations, but they all end up with this same bug.

I don't know if this as a result of my react code, or regl-scatterplot, but I have recreated the bug locally and on stackblitz.

The reproduction code is at the bottom.

Bug Observations

The issue is that when the page is being refreshed,

  1. If the mouse cursor is left outside of the browser content window bounds, the event listeners ARE registered.
  2. If the mouse cursor is quickly moved within the browser content window bounds, the event listeners ARE NOT registered.
  3. If they didn't register, but you subsequently click outside of the browser content window bounds (either selecting a different window and coming back, or re-selecting the tab), they finally register.

Even if the event listeners didn't register, the scatterplot is still interactive (pan, zoom, select).

recording.mp4

Reproduce

The top window in the video was a local implementation, and the bottom window is accessible via the stackblitz link below.

You can find the smallest possible implementation to reproduce the code on stackblitz here. Be aware that you must click the 'Open in New Tab' button on stackblitz to reproduce.

Screen Shot 2023-04-05 at 5 51 10 pm

All the code is in one file and it's only the main page component (to render the canvas) and a hook. It's a very simple implementation.

If you want to re-create the issue locally, comment in the pages/index.js outlines the steps.

1) Install regl-scatterplot via "npm install --save regl regl-scatterplot pub-sub-es"
2) Import useCallback, useEffect, useRef, useState from react
3) Copy 'useScatterplot' hook into the index.js file next to the home component.
4) Use this 'Home' component to register event listeners and render canvas

Let me know if you have any further questions.

@NOT-HAL9000 NOT-HAL9000 changed the title Bug: Scatterplot event listeners don't register if mouse is within screen boundaries on refresh Bug: Scatterplot + React event listeners don't register if mouse is within screen boundaries on refresh Apr 5, 2023
@NOT-HAL9000 NOT-HAL9000 changed the title Bug: Scatterplot + React event listeners don't register if mouse is within screen boundaries on refresh Bug: React + Scatterplot subscribe listeners don't register reliably on refresh. Apr 5, 2023
@NOT-HAL9000
Copy link
Author

NOT-HAL9000 commented Apr 6, 2023

I've narrowed down the problem space a bit. In my example, I would "click" outside of the screen for the events to register. But this is now incorrect, it's when the mouse leaves the canvas space.

I logged from the hover method and realised that it also doesn't get fired when I expect it to.

const hover = (

Moving upstream, I noticed that if I moved the mouse out of the canvas, the mouseLeaveCanvasHandler handler would fire. Then moving it back into the canvas, hover would work.

const mouseLeaveCanvasHandler = () => {

It's looking more and more likely to be related to the isMouseInCanvas flag.

When I logged out from the mouseEnterCanvasHandler handler

const mouseEnterCanvasHandler = () => {

The isMouseInCanvas is fired and logged. Then the event handlers work. No click required.

@NOT-HAL9000
Copy link
Author

I think I have found the issue.

The isMouseInCanvas is initalized as false.

let isMouseInCanvas = false;

If you are able to move the mouse into the canvas before the initalization (it becomes out of sync). The cursor must then trigger the mouseLeaveCanvasHandler followed by the mouseEnterCanvasHandler to get back into sync.

I was able to fix the issue by setting it to true.

let isMouseInCanvas = true;

If the cursor is not in the canvas on startup (isMouseInCanvas is true), afaik it doesn't matter. When the mouse enters the canvas for the first time,mouseEnterCanvasHandler fires anyway and isMouseInCanvas is re-set to true. So it's in sync the whole time.

I think this assumption is safe? I'm not sure if this solution/assumption has any unintended side effects.

Alternatively, you do a check somewhere to find out if the mouse is within the canvas on boot.

@flekschas
Copy link
Owner

Thanks a lot for reporting the bug, putting together the reproducible example, and digging into the code!

You're right the isMouseInCanvas can be an issue when initialized to false. I wonder though if setting it to true on init is better or if that might have other side effects.

Let me have a closer look tonight. Another idea that comes to mind is to determine the state of isMouseInCanvas using something like elementFromPoint() with the mouse position at the time the instance is initialized.

@flekschas flekschas added the bug Something isn't working label Apr 6, 2023
@NOT-HAL9000
Copy link
Author

I guess that I have only tested the isMouseInCanvas: true for a full screen canvas. So it's possible that there may be side effects if you are moving the cursor around the page without having entering the canvas element at all.

You would only have to check and trace mouseMoveHandler to get a better understanding.

const mouseMoveHandler = (event) => {
  if (!isInit || (!isMouseInCanvas && !mouseDown)) return;  // <- HERE

  const currentMousePosition = getRelativeMousePosition(event);
  const mouseMoveDist = dist(...currentMousePosition, ...mouseDownPosition);
  const mouseMovedMin = mouseMoveDist >= lassoMinDist;

  if (isMouseInCanvas && !lassoActive) { // <-  <- HERE
    hover(raycast()); 
  }

  if (lassoActive) {
    event.preventDefault();
    lassoManager.extend(event, true);
  } else if (mouseDown && lassoOnLongPress && mouseMovedMin) {
    lassoManager.hideLongPressIndicator({
      time: lassoLongPressRevertEffectTime,
    });
  }

  if (mouseDownTimeout >= 0 && mouseMovedMin) {
    clearTimeout(mouseDownTimeout);
    mouseDownTimeout = -1;
  }

  if (mouseDown) draw = true;
};

Alternatively, you're right, elementFromPoint could definitely work as well.

@flekschas
Copy link
Owner

I've put together a PR that attempts to fix the issue #114. if you have the chance, could you give it a try?

@NOT-HAL9000
Copy link
Author

lgtm

flekschas added a commit that referenced this issue Apr 8, 2023
@flekschas
Copy link
Owner

@NOT-HAL9000 New version (v1.6.4) with a fix for this bug is out. Thanks again for reporting the issue and taking a stab at it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants