Child window revert

[etpinard]

Fixes #826 and attempts to solve #822

Reverts #806 and @nielsenb-jf 's #764

As mentioned in #827 accessing style sheets programmatically can caused some security errors. While workaround do exist (see here), I prefer not throwing in a somewhat piece of code until we test rigorously on several browsers.

Looking forward, we should perhaps try to support plotting in child windows differently maybe by exposing Plotly.addStyles method that folks wanting to plot in child windows would be expected to call.

That said, some parts of #764 should be incorporated in a future release such as the more general gd._document that we should use to display Lib.notifier among other things.

[mdtusz]

💃

[nielsenb-jf]

Making a callable Plotly.addStyles would be a good compromise.

Alternatively, we could rely on the gd._plotCSSLoaded member we added, and assume if it's false, the style rules hadn't been added and inject, without any checking of the present stylesheets. After all, the original code didn't check for duplicates.

Third option would be to catch the security exception, assume that if it's thrown, the style sheet is from a different domain, and thus doesn't need to be checked for duplicates anyway.

Not sure what is going on with #822 though, I can't reproduce it.

[etpinard]

Alternatively, we could rely on the gd._plotCSSLoaded member we added, and assume if it's false, the style rules hadn't been added and inject, without any checking of the present stylesheets. After all, the original code didn't check for duplicates.

That's what I thought about too. But that would mean that pages with multiple plotly graph div would get duplicate style rule ad infinitum.

A hacky alternative would be to store _plotCSSLoaded in gd._document meaning that we would mutate the window.document object - but I'm not sure I want to go through with that.

[nielsenb-jf]

A hacky alternative would be to store _plotCSSLoaded in gd._document meaning that we would mutate the window.document object - but I'm not sure I want to go through with that.

Really, you're already mutating the document by adding the styles, one additional variable isn't the worst thing. I'm still trying to think up something better though...

[nielsenb-jf]

Could add a child div to the graph div with a specific id. Add a specific style for that id to the plotcss rules. Check to see if the child gets styled. If it does, bail, if it doesn't, inject styles. Then delete the child. It's a little hacky, but I like it better than tacking a variable onto the parent document.

In regards to #822, I think we'd need to switch back to the old way of just injecting an entirely new stylesheet, appending to a stylesheet that may or may not have come from a different domain seems to be fragile...

[etpinard]

Really, you're already mutating the document by adding the styles

Good point. It might be worth exploring. I've made an attempt on the plocss-hotfix branch.

Could add a child div to the graph div with a specific id.

That's an interesting solution. How would it work?

In regards to #822,

Looks like this issue is now fixed in 1.16.1.

[nielsenb-jf]

That's an interesting solution. How would it work?

Good question! Not entirely sure, I'll attempt to put a demo together when I get a chance.

Looks like this issue is now fixed in 1.16.1.

I would expect it to, it's using the old way of just creating an entirely new stylesheet to append to. The code you reverted attempted to append to an existing stylesheet if one was present. I suspect #822 is caused by attempting to append to a stylesheet that came from a different domain. With the benefit of hindsight, I think the old way was actually the better way.

[nielsenb-jf]

That's an interesting solution. How would it work?

Good question! Not entirely sure, I'll attempt to put a demo together when I get a chance.

Works something like this. I don't think the test style I added is compiled quite right, but other then that, it all works.

[etpinard]

@nielsenb-jf your patch on css_div_check is pretty clever 🍻 . But I'm not convinced we should pursue this option. Main reason: this new piece will be run on every Plotly.plot call which may lead to performance drops.

After the revert in 1.16.1, I'm a reluctant to try a automatically inject styles in child windows. I can't honestly think of a clean solution. I would prefer not resorting to a hacky fix for this issue because one small bug here can lead to plotly.js being 100% broken as we saw last week.

I'd vote for adding a top-level (and well-documented 😄 ) method on Plotly to inject styles in child windows on demand. I'd call it Plotly.injectStyles. I hope you don't mind.

[nielsenb-jf]

@etpinard to some degree, I understand the reluctance, it seems to be a niche feature. But on the other hand, plotly already does plenty to the document it's instantiated in, both by injecting styles, and appending a tester div. Drawing a line here seems arbitrary, but it's probably as good of a line in the sand as any.

The issue we saw in Firefox (#826) comes from reading styles from stylesheets loaded from a different domain, which is not allowed in Firefox. #822 comes from appending to a stylesheet loaded from a different domain, which is not allowed in Chrome. I can produce neither error when style injection is done to a new stylesheet (preventing #822, essentially the same way styles are currently injected), and no reading of stylesheets is done (preventing #826, done either by using a style test div or a state variable on the document). It's for all intents and purposes the same thing you're doing currently, just with either a state variable or an additional tester div.

As for a Plotly.injectStyles method, it would only be half a solution. We also need a reference in the graph div to the parent window and some of those related fixes. Both combined would be a perfectly fine fix as well.

[nielsenb-jf]

Follow up I've been meaning to ask since I started working on this. Why are the styles added programmatically anyway? Most JavaScript libraries that require styles just ask the user to include a compiled stylesheet in their document. Then there's no worry about loading it twice. It would also eliminate the code around generating the plotcss file and for injecting it.

[etpinard]

Why are the styles added programmatically anyway?

Very good question. For folks importing plotly.js in a <script>, you're right it doesn't make much of a difference to add a <style> too.

But, for (npm) users bundling plotly.js, adding a <style> tag can be a burden.

Most JavaScript libraries that require styles

From my experiences, that would most old JS libraries [...]