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

fix: add listener to repaint on visibility change for canvas #28568

Merged
merged 1 commit into from
May 21, 2024

Conversation

eschutho
Copy link
Member

@eschutho eschutho commented May 18, 2024

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:

Screen.Recording.2024-05-17.at.5.14.44.PM.mov

After:

Screen.Recording.2024-05-17.at.5.15.49.PM.mov

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 eschutho marked this pull request as ready for review May 18, 2024 00:21
@dosubot dosubot bot added browser:chrome Related to the Chrome browser change:frontend Requires changing the frontend dashboard Namespace | Anything related to the Dashboard labels May 18, 2024
@yousoph
Copy link
Member

yousoph commented May 19, 2024

/testenv up

Copy link
Contributor

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

@mistercrunch
Copy link
Member

mistercrunch commented May 20, 2024

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 ?

@eschutho
Copy link
Member Author

@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.

@mistercrunch
Copy link
Member

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.

@eschutho
Copy link
Member Author

eschutho commented May 21, 2024

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 eschutho merged commit 62a0336 into master May 21, 2024
38 of 56 checks passed
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@michael-s-molina
Copy link
Member

michael-s-molina commented May 23, 2024

@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?

@john-bodley john-bodley deleted the elizabeth/repaint-convas branch May 23, 2024 17:20
Vitor-Avila pushed a commit to Vitor-Avila/superset that referenced this pull request May 28, 2024
EnxDev pushed a commit to EnxDev/superset that referenced this pull request May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser:chrome Related to the Chrome browser change:frontend Requires changing the frontend dashboard Namespace | Anything related to the Dashboard dependencies:npm preset-io size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants