denverfoundation / storybase

The code behind Floodlight
http://floodlightproject.org
MIT License
11 stars 7 forks source link

Embed code from Google Charts not rendering in Story Builder #187

Closed jwirfs-brock closed 11 years ago

jwirfs-brock commented 12 years ago

Jon was trying to embed an interactive Google Chart into a story (that he originally built using the Django admin). He found that the embed code wasn't rendering, but he could publish an image of the chart. I recreated this issue in the story builder.

Here is the link to the Google Chart: https://docs.google.com/spreadsheet/ccc?key=0AhWnhIC_xoBpdGRoUWRFNmxfaGlRSUxwY1dXSkJhVmc#gid=4

You can grab the embed code by selecting "Publish chart" and "Interactive chart" as the publish format. Here's the embed code:

<script type="text/javascript" src="//ajax.googleapis.com/ajax/static/modules/gviz/1.0/chart.js"> {"dataSourceUrl":"//docs.google.com/spreadsheet/tq?key=0AhWnhIC_xoBpdGRoUWRFNmxfaGlRSUxwY1dXSkJhVmc&transpose=0&headers=1&range=A2%3AD26&gid=0&pub=1","options":{"vAxes":[{"title":"Travel Time in Minutes","useFormatFromData":true,"viewWindowMode":"pretty","gridlines":{"count":"10"},"viewWindow":{}},{"useFormatFromData":true,"viewWindowMode":"pretty","viewWindow":{}}],"title":"Travel Time to Denver Metro Area Public Soccer Fields from the Holly Square","booleanRole":"certainty","height":717,"animation":{"duration":0},"width":1682,"hAxis":{"titleTextStyle":{"color":"#222","italic":true,"fontSize":"12"},"title":"Denver Area Soccer Field","useFormatFromData":true,"viewWindowMode":"pretty","viewWindow":{}},"isStacked":false},"state":{},"chartType":"ColumnChart","chartName":"Chart 1"} </script>

...and a screenshot of how you grab it. In the story builder, I selected the asset type "map" because it had a field for embed code. (We have no "chart" or "graph" asset type, and the image asset type doesn't have an embed code field. I'm not sure if this should be a separate issue.)

After pasting in the embed code and clicking "Save changes", here is how the asset appeared within the story builder (on the left): screenshot

When I clicked to edit the asset, the embed code was retained/editable.

ghing commented 12 years ago

Note that when (pre)viewing the story in the viewer, the chart shows up.

The embedding strategy used by Google Charts doesn't play well with the builder because it dynamically adds an element to the DOM. I'll have to play around with this a little to see if I can change the rendering strategy to make this work. Frankly, I don't think Google Charts' embedding implementation is very good, and it reflects that the service sits in a weird place between a developer API and an end user service. The embedding strategy really reflects how the Google Chart/Visualization API seems primarily geared at people trying to visualize data from an application.

This embedding strategy also requires that the user enter a SCRIPT html tag. We need to be very, very careful about this because it opens the site to cross-site scripting (XSS) attacks if we just let users enter in arbitrary SCRIPT tags.

The most straightforward way to make this work is to have a whitelist of domains that we allow iframes and script tags to be sourced from.

One reason oEmbed exists is to make it easier to let users embed content and making it easier for an application to determine the safety of embedded content.

We should add a chart type.

jwirfs-brock commented 12 years ago

Agreed that we should add a chart/graph type.

Having a whitelist of domains that we allow script and iframes tags sounds like a good idea. I'll start taking a look at some of the other common sites we want to support (Tableau Public, Many Eyes, etc.) along with examples of how they do embeds.

If you feel strongly about Google Charts, should we discourage people from using it? From the user standpoint, Google Charts is easy to use and many people already have Google accounts, which is why we are encouraging people in the workshops to go that route. When creating visualizations from Fusion Tables (which don't yet have as many options as the charts/visualization API, I think), the embed code is an iframe tag. We can encourage people to use Fusion Tables as opposed to Google Spreadsheets/Google Charts when possible, if that makes sense.

ghing commented 12 years ago

I think we should support what people like to use. Iframes are less problematic for showing in the builder, but should still be whitelisted. I'm just saying that Google Charts doesn't make it easy for our use case and this should advise us of how not to make the Data Engine embedding work. I guess my feeling is that if something is shareable, it should be more persistent and have its own URI.

Frankly, my dream solution would be to make a companion site that lets users share their visualizations in a consistent way instead of having to wrap all of this functionality into Floodlight/Storybase.

I've created #189 as a feature request for the chart/graph type. This is easy to do. The hard part is just deciding which labels and fields should show up. I'm thinking it should function like a map, allowing the upload of a static image, embedded HTML or a URL to an image/oembedable resource. Can you add your thoughts there.

ghing commented 12 years ago

@jwirfs-brock Can you point me to training materials around Google Charts? One thing, as exemplified by that particular embed is that it's super huge, and doesn't automatically scale. I just want to get a feel for whether users have a sense of how big things will be when they export them out of Google Spreadsheets.

jwirfs-brock commented 12 years ago

Embed code examples are here: https://docs.google.com/spreadsheet/ccc?key=0As2exFJJWyJqdHQ2QThsTkdBRnBJdkJ6cGJXamN6VFE#gid=0

ghing commented 12 years ago

@jwirfs-brock I'm still thinking about the best way to handle this. I just wanted to note that infogram embeds using an IFRAME and it works quite nicely in the builder.

ghing commented 12 years ago

Note to self: maybe I can create an iframe and attach the script tag to it?

ghing commented 11 years ago

@tekhneco reported another instance of this as #735. In this case the embed code that wouldn't render is:

<script type="text/javascript" src="//ajax.googleapis.com/ajax/static/modules/gviz/1.0/chart.js"> {"dataSourceUrl":"//docs.google.com/a/tekhne.co/spreadsheet/tq?key=0AmK32wFXtJAQdFdTWUtiVTZpcUliSlF2bUtYREk0d2c&transpose=0&headers=0&range=A5%3AK11&gid=0&pub=1","options":{"titleTextStyle":{"bold":true,"color":"#000","fontSize":16},"vAxes":[{"useFormatFromData":true,"title":"Types of apples","minValue":null,"viewWindowMode":null,"viewWindow":null,"maxValue":null},{"useFormatFromData":true}],"booleanRole":"certainty","title":"U.S. per capita use of selected, commercially produced, fresh, and processing apples, 1976 to date","animation":{"duration":500},"legend":"right","hAxis":{"useFormatFromData":true,"title":"Pounds, farm eight","minValue":0,"viewWindowMode":"explicit","logScale":false,"viewWindow":{"min":0,"max":400},"maxValue":400},"tooltip":{},"isStacked":true,"width":450,"height":320},"state":{},"view":{},"isDefaultVisualization":true,"chartType":"BarChart","chartName":"Chart 1"} </script>
patternleaf commented 11 years ago

Google's chart script was getting confused by being loaded in an unorthodox way. We've addressed this via a more general approach of sandboxing in an iframe any html asset that has a script tag.

One issue with this approach is appropriately sizing the iframe to match its contents. ae17f78 has a solution for this while integrating the solution for #469 which has a similar pattern of autosizing needing to happen programmatically after asset load.

ae17f78 has been tested in FF, Safari, Chrome, and IE8. @ghing would you mind sanity-checking that commit? Note it does not have any of the fixture-based tests you put together.

re: the problem of deferring iframe loading, it prints a vanilla src attribute on the backend. To "defer" it, the JS immediately aborts the load after rendering. This is kind of ugly as it does provoke an http request for the iframe content, but Firebug is reporting 0 bytes received on that request ... if you have other thoughts on this, let me know.

ghing commented 11 years ago

@patternleaf, I'm going to take a look at ae17f78. Can you document the testing that you did, even if its really quick and dirty/informal, just so we can replicate it in the future and so I think of any cases that you missed.

patternleaf commented 11 years ago

@ghing, Testing diagrammed below. This is good—found a case I missed and noticed that browser resizing isn't working as desired.

ghing commented 11 years ago

I just thought of another test case: in the builder, remove the asset from the container and re-add it using drag + drop from the asset drawer.

patternleaf commented 11 years ago

@ghing roger that.

patternleaf commented 11 years ago

Yeah: asset in asset drawer is subject to the same error as original. But if #770 is solved by use of an icon, that may provide a solution to this issue too, correct?

ghing commented 11 years ago

Yes. I was thinking more of making sure it displayed correctly in the container after it was added. But I guess you have to wait for the #770 fix to test that?

patternleaf commented 11 years ago

@ghing I can probably hack something to test without waiting for #770.

patternleaf commented 11 years ago

Update on this:

  1. Dragging from the asset drawer seems to work correctly in that the asset is re-created faithfully in the container.
  2. I'll give up on having the figure container collapse to the size of its asset for this issue. (To clarify, in http://floodlightproject.org/stories/officer-hollis-peace-memorial/viewer/#sections/e7bab059de7443978364915bd65282e5 for example, you'll notice that the figure outline fills the entire 50% space available in the 2-up layout. I had code which collapsed this to the width of the asset, which seems desirable, but making this work with responsive resizing was looking troublesome; I'd say this should be taken up in a separate issue.)
  3. Still to fix: captions are not resizing with current code (see, eg, http://dev2.floodlightproject.org/stories/officer-hollis-peace-memorial/viewer/#sections/e7bab059de7443978364915bd65282e5 at the time of this writing)

Once 3 is fixed I'm going to run another round of tests.

patternleaf commented 11 years ago

I've tested the iframe-wrapping scheme with the following kinds of script-based embeds:

Also I tried a couple of assets that had either a google map or an alternate form of google chart. Both of those included full HTML markup. These forms broke because our scheme wraps the asset body with <html><head></head><body>... without checking whether the asset already contains a head or body.

Other problems noted:

patternleaf commented 11 years ago

Status update. With current code on dev2, this is just about ready to be closed. The update will fix google chart embeds showing up in the builder. Google chart embeds are the only embedded script-based widgets currently known to exist in the wild. Nonetheless we have done some testing on other embed types. Some of those will break in new and exciting ways, but it turns out that they were pretty broken before this, too. Here are the results of testing a few embeds on dev vs dev2 (Firefox only):

no 187 fixes (dev)

187 branch (dev2)

We're going to open a separate issue for those embeds, or maybe separate issues for each. Also a list of first-class embeddable widgets that will likely need support is in the offing.

ghing commented 11 years ago

@patternleaf Can you make sure you include the case for this issue reported by @jwirfs-brock as part #803 in your testing? Basically, does the caption/border resizing work for slow-loading images, or is it run before the image has a detectable size, resulting in the image not being shown?

patternleaf commented 11 years ago

does the caption/border resizing work for slow-loading images, or is it run before the image has a detectable size, resulting in the image not being shown?

Ugh. Yeah. It looks like I need to re-introduce the imagesLoaded plugin. I think this was on account of some confusion on my part between implementing responsive images and the necessity of that plugin.