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 tooltips not visible for NVD3 charts on Firefox (#7822) #7929

Merged
merged 1 commit into from
Aug 9, 2019

Conversation

schoel-bis
Copy link
Contributor

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

The bug was introduced by #7102

Using position:absolute on body gives document.documentElement a height of 0 which propagates to clientHeight on Firefox. This leads to a wrong calculation of the tooltip position in NVD3.

The solution proposed here is to use min-height: 100vh instead of the current technique for stretching the body element to the full window height, thus keeping body and html together.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

Access an NVD3 based chart in Firefox and hover with mouse.

ADDITIONAL INFORMATION

  • Has associated issue: Fixes Line chart tooltips broken on Firefox #7822
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

@etr2460
Copy link
Member

etr2460 commented Jul 26, 2019

Could you add screenshots to show the before/after of the change? Thanks!

@schoel-bis
Copy link
Contributor Author

Sure.

Before (note the highlighted dots in the chart on the left from hovering):

Bildschirmfoto von 2019-07-26 20-03-19

After:

Bildschirmfoto von 2019-07-26 20-03-58

@mistercrunch
Copy link
Member

You may have to rebase because of Travis issues. Also can you confirm this doesn't break Chrome?

@codecov-io
Copy link

codecov-io commented Jul 28, 2019

Codecov Report

Merging #7929 into master will increase coverage by 0.37%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7929      +/-   ##
==========================================
+ Coverage   65.59%   65.97%   +0.37%     
==========================================
  Files         469      468       -1     
  Lines       22401    22297     -104     
  Branches     2432     2429       -3     
==========================================
+ Hits        14694    14710      +16     
+ Misses       7586     7466     -120     
  Partials      121      121
Impacted Files Coverage Δ
superset/assets/src/messageToasts/actions/index.js 79.16% <0%> (-12.5%) ⬇️
superset/cli.py 37.16% <0%> (-0.18%) ⬇️
...src/explore/components/controls/MetricsControl.jsx 79.72% <0%> (-0.14%) ⬇️
superset/views/core.py 71.15% <0%> (-0.07%) ⬇️
superset/tasks/cache.py 73.68% <0%> (ø) ⬆️
superset/config.py 94.04% <0%> (ø) ⬆️
superset/examples/helpers.py 100% <0%> (ø) ⬆️
superset/common/tags.py
superset/viz.py 71.77% <0%> (+0.02%) ⬆️
superset/connectors/sqla/models.py 83.5% <0%> (+0.02%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8ca078...81eb344. Read the comment docs.

@schoel-bis
Copy link
Contributor Author

Rebase done. I cannot spot any visual differences in Chrome – nor in Firefox apart from the tooltips being back. I don't have access to IE/Edge or Safari but according to caniuse the vh CSS unit is supported since IE9.

@mistercrunch
Copy link
Member

I don't think the rebase worked properly, your PR now has commits that shouldn't be there...

@schoel-bis schoel-bis force-pushed the fix-firefox-tooltips branch from cf708ab to 65782d1 Compare July 29, 2019 07:18
@schoel-bis schoel-bis force-pushed the fix-firefox-tooltips branch from 65782d1 to 81eb344 Compare August 5, 2019 08:30
Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me then. Seems like CI was flaky though, so i'd recommend rebasing to kick off ci again

This bug was introduced by apache#7102

Using `position:absolute` on body gives `document.documentElement` a height of 0 which propagates to `clientHeight` on Firefox. This leads to a wrong calculation of the tooltip position in NVD3.

The solution proposed here is to use `min-height: 100vh` instead of the current technique for stretching the body element to the full window height, thus keeping body and html together.
@schoel-bis schoel-bis force-pushed the fix-firefox-tooltips branch from 81eb344 to 5c795b4 Compare August 7, 2019 11:42
@etr2460
Copy link
Member

etr2460 commented Aug 9, 2019

@mistercrunch I think this is good to merge

@mistercrunch mistercrunch merged commit 6df2a71 into apache:master Aug 9, 2019
etr2460 pushed a commit to etr2460/incubator-superset that referenced this pull request Aug 30, 2019
@graceguo-supercat
Copy link

@schoel-bis This PR has to be reverted from Master branch. Please see details in #8147.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Line chart tooltips broken on Firefox
5 participants