superset icon indicating copy to clipboard operation
superset copied to clipboard

fix: add listener to repaint on visibility change for canvas

Open eschutho opened this issue 1 year ago • 5 comments

SUMMARY

We're seeing with the latest release of Chrome, some canvas elements on the dashboard will disappear, and only reappear on mouseover. I believe that there was an experimental feature in Chrome that saved memory by removing canvas elements when the browser tab wasn't active, and it just went GA.

Echart's library zrender is responsible for animating the canvas on mouseover which is making the canvas visible again, but we need another solution to repaint the canvas when the document is visible again.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

https://github.com/apache/superset/assets/5186919/450cfd71-e5ee-4796-9e6f-eb46ae5424fd

After:

https://github.com/apache/superset/assets/5186919/5dab80b9-657f-4a71-b47c-f74e6312f3db

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • [ ] Has associated issue:
  • [ ] Required feature flags:
  • [ ] Changes UI
  • [ ] Includes DB Migration (follow approval process in SIP-59)
    • [ ] Migration is atomic, supports rollback & is backwards-compatible
    • [ ] Confirm DB migration upgrade and downgrade tested
    • [ ] Runtime estimates and downtime expectations provided
  • [ ] Introduces new feature or API
  • [ ] Removes existing feature or API

eschutho avatar May 18 '24 00:05 eschutho

/testenv up

yousoph avatar May 19 '24 02:05 yousoph

@yousoph Ephemeral environment spinning up at http://35.85.45.127:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

github-actions[bot] avatar May 19 '24 02:05 github-actions[bot]

This appears to fix the behavior in the Dashboard app, but not in explore (?) should we do this lower level in the Chart component or in some other chart-wrapping component that's common to both Explore and Dashboard ?

mistercrunch avatar May 20 '24 16:05 mistercrunch

@mistercrunch it doesn't seem to affect explore or individual charts, but only dashboards. One issue with adding it to explore is that we need to assign an event listener to the window, and only one event listener is allowed at a time, or rather each time we create a new event listener, it will overwrite the last one. This change uses the already existing visibilitychange event listener used for logging.

eschutho avatar May 20 '24 20:05 eschutho

Oh interesting, I'm positive I've seen the behavior in explore too. I'm on Mobile now so I can't tell reproduce... I think it was big number, blank after switching tab, reappears on hover.

mistercrunch avatar May 20 '24 23:05 mistercrunch

Ok, took a couple more tries, but I was just able to repro on explore. I can add another event listener there, but outside of the slice so that we don't wind up with multiple event listeners. Talking with a few folks, it sounds like we may want to follow up with explore in a separate PR.

eschutho avatar May 21 '24 16:05 eschutho

Ephemeral environment shutdown and build artifacts deleted.

github-actions[bot] avatar May 21 '24 16:05 github-actions[bot]

@eschutho Thank you for providing this fix. It looks like they disabled the feature that was causing the problem in 125.0.6422.76 which is already available. Can we revert this PR to avoid introducing logic that fixes a dependency problem?

michael-s-molina avatar May 23 '24 11:05 michael-s-molina