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

Fallback Data Vis images don’t display on tablet #432

Closed
bazzadp opened this issue Nov 10, 2019 · 15 comments · Fixed by #511
Closed

Fallback Data Vis images don’t display on tablet #432

bazzadp opened this issue Nov 10, 2019 · 15 comments · Fixed by #511
Assignees
Labels
Projects
Milestone

Comments

@bazzadp
Copy link
Contributor

@bazzadp bazzadp commented Nov 10, 2019

On iPad we don’t see the Google Sheets images (presumably because of same reason as iPhone: #369) but the fall back images do not show either whereas they do on iPhone.

CSS need a little tweaking to change the breakpoint?

@rviscomi

This comment has been minimized.

Copy link
Member

@rviscomi rviscomi commented Nov 10, 2019

IIUC, this is working as intended. We need images to appear on small viewports because the 600px interactive charts would otherwise overflow. On a tablet with a wider screen, the wide interactive charts still fit. WDYT?

@rviscomi rviscomi added this to TODO in Web Almanac via automation Nov 10, 2019
@rviscomi rviscomi added this to the SHIP IT! milestone Nov 10, 2019
@bazzadp

This comment has been minimized.

Copy link
Contributor Author

@bazzadp bazzadp commented Nov 10, 2019

Except that iPadOS at least doesn't render them. Not sure about Android.

@rviscomi

This comment has been minimized.

Copy link
Member

@rviscomi rviscomi commented Nov 10, 2019

Because of this issue "Canvas area exceeds the maximum limit (width * height > 16777216)"?

Really surprised.

Is this an issue for all charts or only very large ones? The default dimensions are width="600" height="371"for an area of 222,600. So I'm confused why those would be blocked.

@rviscomi rviscomi added bug and removed enhancement labels Nov 10, 2019
@bazzadp

This comment has been minimized.

Copy link
Contributor Author

@bazzadp bazzadp commented Nov 10, 2019

Because of this issue "Canvas area exceeds the maximum limit (width * height > 16777216)"?

Yup confirmed by hooking iPad up to Mac and using Web Inspector. Same error.

Is this an issue for all charts or only very large ones? The default dimensions are width="600" height="371"for an area of 222,600. So I'm confused why those would be blocked.

All charts. Even my simple Pie Chart in HTTP/2 doesn't render. Your Performance chapter is looking pretty bare...

@rviscomi rviscomi added the ASAP label Nov 10, 2019
@rviscomi

This comment has been minimized.

Copy link
Member

@rviscomi rviscomi commented Nov 10, 2019

Ok in that case this is a launch blocker. Super strange. The canvas size limit is 75x larger than the area for most of these charts so I have no idea why they'd be blocked.

If we can't figure this out we can display: none all iframes and show the static images to everyone.

@rviscomi rviscomi moved this from TODO to Needs Attention in Web Almanac Nov 10, 2019
@bazzadp

This comment has been minimized.

Copy link
Contributor Author

@bazzadp bazzadp commented Nov 10, 2019

If we can't figure this out we can display: none all iframes and show the static images to everyone.

That would be a shame as they are nicer. However maybe it's better to do that until we fix #428 anyway?

@rviscomi

This comment has been minimized.

Copy link
Member

@rviscomi rviscomi commented Nov 10, 2019

Oof this is a really tough call. I'll think about it. Open to other suggestions.

@bkardell

This comment has been minimized.

Copy link
Contributor

@bkardell bkardell commented Nov 11, 2019

Android, chrome, Moto g7, I can not see any of them, fallback even on the markup chapter. The only ones that show up are the static images, most are missing

@bazzadp

This comment has been minimized.

Copy link
Contributor Author

@bazzadp bazzadp commented Nov 11, 2019

Android, chrome, Moto g7, I can not see any of them, fallback even on the markup chapter. The only ones that show up are the static images, most are missing

Raised #462 for this, as even missing on mobile so should be there for that.

@bazzadp

This comment has been minimized.

Copy link
Contributor Author

@bazzadp bazzadp commented Nov 13, 2019

Because of this issue "Canvas area exceeds the maximum limit (width * height > 16777216)"?

Really surprised.

Is this an issue for all charts or only very large ones? The default dimensions are width="600" height="371"for an area of 222,600. So I'm confused why those would be blocked.

OK some more progress on this. If you look at any of the Google Sheet images in Google Chrome, they are like this:

<canvas class="docs-charts-component-canvas" width="1200" height="742" style="width: 600px; height: 371px;"></canvas>

So yes you are restricting to 600 by 371 however that's just a style - the main canvas is 1200 by 742.

In iOS, with a Retina Screen it's even worse as Google Sheets seems to recognise this and up the pixel count:

<canvas class="docs-charts-component-canvas" width="6000" height="3710" style="width: 600px; height: 371px;"></canvas>

So that's 6000 by 3710 = 22,260,000 which is indeed bigger than the 16,777,216 max limit for mobile Safari :-(

Unfortunately there is no fix as Google Sheets do not allow you to set the Canvas size - only the style size 😞. Google Sheets really should fix this and feature detect and display the appropriate canvas size.

In the meantime adding the following to src/templates/base.html seems to work:

  <body>
    {% block content %}{% endblock %}

    <script nonce="{{ csp_nonce() }}">

      //iPad, with a pixel ratio of 2, have problems displaying Google Sheets
      //See https://github.com/HTTPArchive/almanac.httparchive.org/issues/432
      //Macs with a pixel ratio of 3, don't so let's check for 2 only.
      if (window.devicePixelRatio = 2) {

        //Hide the figure iframes
        var all_fig_iframes = document.querySelectorAll('figure iframe');
        all_fig_iframes.forEach(function(fig_frame) {
          fig_frame.style.display = "none";
        });
        
        //Remove the fig-mobile class from links/imho to prevent it being hidden in desktop in this scenario
        var all_fig_imgs = document.querySelectorAll('.fig-mobile');
        all_fig_imgs.forEach(function(fig_img) {
          fig_img.classList.remove("fig-mobile");
        });
      }
  </script>
  </body>

Tested on iPad and iPhone.

@rviscomi / @mikegeyser what's your thoughts?

Ideally I'd like to test for max canvas size and only apply this when 16M but there is no API to get a maximum canvas size unfortunately. So best I can think of is to stop it for Pixel Ratio of 2 and some users (ChromeOS users with nice display) might suffer, but even then get the images.

@rviscomi

This comment has been minimized.

Copy link
Member

@rviscomi rviscomi commented Nov 13, 2019

Something like this should be possible in CSS with a media query. I think it's a reasonable approach.

@rviscomi rviscomi moved this from Needs Attention to In Progress in Web Almanac Nov 13, 2019
@rviscomi rviscomi removed the ASAP label Nov 13, 2019
@rviscomi rviscomi modified the milestones: SHIP IT!, Après Ski Nov 13, 2019
@bazzadp

This comment has been minimized.

Copy link
Contributor Author

@bazzadp bazzadp commented Nov 13, 2019

Actually discovered a problem - Chrome on my MacOS is reporting a pixelDensity of 2 as well, not 3 as I thought. So we'd turn it off for all Chrome users who can handle this :-(

Any ideas how to reliably detect iOS?

Or can we create a Canvas object of 6000 by 3710 on this page and if that fails then fall back if that fails?

More info, including how to detect here: https://github.com/jhildenbiddle/canvas-size#test-results

@bazzadp

This comment has been minimized.

Copy link
Contributor Author

@bazzadp bazzadp commented Nov 13, 2019

BTW it would be nice to couple this with #428 and if either large canvas is not supported or Google Sheets is blocked, then fallback.

Or alternatively change the default to images, and fall forward if both large Canvas and Google Sheets connectivity is supported if that's easier to detect! Especially with lazy loading now, which should mean most images are not loaded. Progressive enhancement and all that...

I'm gonna have to call it a night there but anyone else is free to pick this up and continue my investigations (especially if their JavaScript is better than mine - which won't be difficult!). Happy to help test both items locally in the morning if anyone does come up with anything and sticks it on a branch.

@rviscomi

This comment has been minimized.

Copy link
Member

@rviscomi rviscomi commented Nov 13, 2019

Thanks @bazzadp. I agree that it'd be much better to disable the interactive chart only to users who can't support it. Detecting this reliably is the biggest blocker, but we should investigate our options.

To summarize the various data viz issues:

  • the Sheets iframe may be blocked unexpectedly at the network level (#428)
  • the canvas in the Sheets iframe may be blocked at the UA level if the canvas is too big (this issue)
  • in the general case, the image and iframe are wastefully loaded regardless of device type (#413 (comment))
  • in the general case, the images and iframes are loaded before scrolled into view (fixed by #351)
@bazzadp

This comment has been minimized.

Copy link
Contributor Author

@bazzadp bazzadp commented Nov 15, 2019

BTW I think this should be in SHIP-IT milestone and not Apres-Ski. A non-trivial number of users will not be getting visuals. Hence why I've prioritised this.

@rviscomi rviscomi removed this from the Après Ski milestone Nov 15, 2019
@rviscomi rviscomi added this to the SHIP IT! milestone Nov 15, 2019
@bazzadp bazzadp closed this in #511 Nov 25, 2019
Web Almanac automation moved this from In Progress to Done Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Web Almanac
  
Done
3 participants
You can’t perform that action at this time.