Fix counter visualization not working on Safari #5236

daniellangnet · 2020-10-25T21:05:50Z

What type of PR is this? (check all applicable)

Bug Fix

Description

In the current master branch, the counter visualization doesn't work in Safari at all (both desktop and mobile) and stays completely empty. This is because of a change that was introduced in this PR.

This fixes the issue (while still keeping the max width/height for the query screen of the above PR working).

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

kravets-levko · 2020-10-25T21:32:25Z

@daniellangnet Which Safari version do you use? 'cause that's what I see in Safari on our master:

Upd.: same for the issue with dashboard container height: it's totally okay in my Safari. I'm using 14.0

daniellangnet · 2020-10-25T21:34:18Z

@kravets-levko I'm on Safari 14. It was okay in the query editor, but not when added to a dashboard (see screenshot)

kravets-levko · 2020-10-25T21:40:04Z

Got it. Seems both here and for dashboard root there's the same issue: for some reason, if there's a container with position: relative and child with height: 100% Safari fails to set a proper child height, which is totally not correct behavior.

Upd.: position: relative doesn't work with flex-grow: 1 🙂 but it should

daniellangnet · 2020-10-25T21:45:26Z

Agreed. Although I'm not positive that a child with position: absolute is supposed to increase the height of an empty parent with position: relative. Could just be an implementation choice in Chrome / Firefox

Update: Oh Apple... the new IE.

kravets-levko · 2020-10-25T21:52:18Z

@daniellangnet Add height: 0 here (but please check in Chrome & FF if you can, or I'll check tomorrow):

redash/client/app/components/dashboards/dashboard-grid.less

Lines 89 to 92 in 6f9e79c

    
           > .visualization-renderer-wrapper { 
        
             flex-grow: 1; 
        
             position: relative; 
        
           }

daniellangnet · 2020-10-25T22:03:42Z

@kravets-levko I just added height: 0 as requested (although I'm not really sure I understand the purpose of it). Also confirming that I tested this in both Chrome and Firefox, latest versions respectively.

kravets-levko · 2020-10-25T22:04:42Z

client/app/components/dashboards/dashboard-grid.less

@@ -89,6 +89,7 @@
    > .visualization-renderer-wrapper {
      flex-grow: 1;
      position: relative;
+      height: 0;


Please try just this fix - for me it worked without others (also, I tried in Chrome a minute ago - it was fine too). If it works - let's not touch other styles.

Only works in connection with this other change for me: https://github.com/getredash/redash/pull/5236/files#diff-4e9dd1634afb5937186e1247809856826568d835e2167e33be776e08ae6ffc94L6

Everything else we could get rid of though

kravets-levko · 2020-10-25T22:09:17Z

I'm not really sure I understand the purpose of it

I'm also not sure why it works. I found a mention of this fix on some forum, and somebody said that that's some bug in Safari - in some cases it fail to compute real element's height when it's offset parent has an auto height set in either way. Setting it explicitly to some value fixes the issue, and in combination with flexbox will result in a proper layout. I just tried it, and looks it really works, so...

P.S. I think, nobody really understands how CSS works 🙂 We all just use it like a magic

daniellangnet · 2020-10-25T22:11:10Z

@kravets-levko haha, now I know what you mean! I tried it but that change alone doesn't fix it for me. The height: 0 only works if I also remove the position: relative from .counter-visualization-container

kravets-levko · 2020-10-25T22:19:17Z

The height: 0 only works if I also remove the position: relative from .counter-visualization-container

Oh Apple... the new IE.

🤦‍♂️

P.S. I never liked any browser-specific bugs and workarounds for them. I'll check "my" solution once more tomorrow, I hope it's possible to make it work in all browsers (and have some logical explanation).

daniellangnet · 2020-10-25T22:36:29Z

Sounds good. Thank you @kravets-levko. FWIW, I think my original solution without the height: 0 made sense to me from a CSS perspective.

From what I understand, the issue was this:

counter-visualization-container is the container that was used for the height calculation in JavaScript
counter-visualization-content is the child container that is set to position: absolute so that it can expand to it's full height, before we apply the scale() to keep it contained
if we add position: relative to counter-visualization-container, then this container will only have it's own height, because the absolutely positioned counter-visualization-content inside would NOT push it's height
this actually sounds like correct behavior to me, it just appears that Chrome & Firefox chose to implement it differently regardless which is why it worked there
so if we remove the position: relative from counter-visualization-container then the reference container is one level higher up, therefore allowing the container to wrap itself around around its child, up to the maximum that is allowed by its own flex-container

I reverted my commit back to what it originally was, because I totally agree with you on your dislike for browser-specific workarounds in CSS 😃 🙌

dengc367 · 2020-11-23T08:34:15Z

me too. the counter visualization can not shown in my safari, which version is 14.0.1.

guidopetri · 2023-08-20T22:02:04Z

@daniellangnet , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

justinclift · 2023-08-22T21:56:40Z

The merge conflict for this PR has been solved. Does anyone know if the problem it fixes is still occurring with our current Redash builds and modern Safari though?

I'd prefer not to merge this if the problem no longer exists. 😄

codecov · 2023-08-22T22:08:28Z

Codecov Report

Merging #5236 (c34beff) into master (4107265) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5236   +/-   ##
=======================================
  Coverage   60.70%   60.70%           
=======================================
  Files         154      154           
  Lines       12624    12624           
  Branches     1716     1716           
=======================================
  Hits         7663     7663           
  Misses       4735     4735           
  Partials      226      226

daniellangnet mentioned this pull request Oct 25, 2020

Added KPI visualization #4640

Open

1 task

kravets-levko self-requested a review October 25, 2020 21:27

kravets-levko self-assigned this Oct 25, 2020

kravets-levko added Frontend Visualizations Visualizations: Counter labels Oct 25, 2020

Fix counter visualization not working on Safari

4971d3e

daniellangnet force-pushed the fix-counter-safari branch from a27e790 to 4971d3e Compare October 25, 2020 22:02

kravets-levko reviewed Oct 25, 2020

View reviewed changes

Fix counter visualization not working on Safari

cf255ba

daniellangnet force-pushed the fix-counter-safari branch from ae5a624 to cf255ba Compare October 25, 2020 22:39

guidopetri added the 1-week-review label Aug 20, 2023

Merge branch 'master' into fix-counter-safari

c34beff

guidopetri removed the 1-week-review label Aug 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix counter visualization not working on Safari #5236

Fix counter visualization not working on Safari #5236

daniellangnet commented Oct 25, 2020

kravets-levko commented Oct 25, 2020 •

edited

Loading

daniellangnet commented Oct 25, 2020

kravets-levko commented Oct 25, 2020 •

edited

Loading

daniellangnet commented Oct 25, 2020 •

edited

Loading

kravets-levko commented Oct 25, 2020

daniellangnet commented Oct 25, 2020 •

edited

Loading

kravets-levko Oct 25, 2020

daniellangnet Oct 25, 2020

kravets-levko commented Oct 25, 2020

daniellangnet commented Oct 25, 2020

kravets-levko commented Oct 25, 2020

daniellangnet commented Oct 25, 2020 •

edited

Loading

dengc367 commented Nov 23, 2020

guidopetri commented Aug 20, 2023

justinclift commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023

Fix counter visualization not working on Safari #5236

Are you sure you want to change the base?

Fix counter visualization not working on Safari #5236

Conversation

daniellangnet commented Oct 25, 2020

What type of PR is this? (check all applicable)

Description

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

kravets-levko commented Oct 25, 2020 • edited Loading

daniellangnet commented Oct 25, 2020

kravets-levko commented Oct 25, 2020 • edited Loading

daniellangnet commented Oct 25, 2020 • edited Loading

kravets-levko commented Oct 25, 2020

daniellangnet commented Oct 25, 2020 • edited Loading

kravets-levko Oct 25, 2020

Choose a reason for hiding this comment

daniellangnet Oct 25, 2020

Choose a reason for hiding this comment

kravets-levko commented Oct 25, 2020

daniellangnet commented Oct 25, 2020

kravets-levko commented Oct 25, 2020

daniellangnet commented Oct 25, 2020 • edited Loading

dengc367 commented Nov 23, 2020

guidopetri commented Aug 20, 2023

justinclift commented Aug 22, 2023 • edited Loading

codecov bot commented Aug 22, 2023

Codecov Report

kravets-levko commented Oct 25, 2020 •

edited

Loading

kravets-levko commented Oct 25, 2020 •

edited

Loading

daniellangnet commented Oct 25, 2020 •

edited

Loading

daniellangnet commented Oct 25, 2020 •

edited

Loading

daniellangnet commented Oct 25, 2020 •

edited

Loading

justinclift commented Aug 22, 2023 •

edited

Loading