search

plotly / plotly.js

Open-source JavaScript charting library behind Plotly and Dash
https://plotly.com/javascript/
MIT License
16.78k stars 1.84k forks source link

Interactive portions of plot do not render correctly in a child window #702

Closed nielsenb-jf closed 1 month ago

nielsenb-jf commented 8 years ago

When using Plotly to render in a child window, the hover functionality for plots seems to break completely. Any enabled buttons will render in the incorrect location, but seem to function correctly.

This CodePen shows the issue.

The attached screenshot demonstrates the issue as well.

I have reproduced in the latest stable Firefox and Chrome.

screenshot

etpinard commented 8 years ago

Thanks for report.

Interesting. I'm not sure what could cause that.

nielsenb-jf commented 8 years ago

Everything renders correctly if you copy the injected styles from the parent window to the child. See this CodePen for a demonstration.

Things break if you try to interact with the plot by clicking on a data point or by zooming by clicking and selecting a zoom area.

etpinard commented 8 years ago

@nielsenb-jf Interesting. Thanks for letting us know.

nielsenb-jf commented 8 years ago

I have a branch in my fork that demonstrates fixing interactivity when rendering to a child window. Basically, it renders the drag cover to the document of the child window, instead of always to the parent window where Plotly was instantiated.

Looking at the code more, Plotly makes lots of assumptions about which document / window to render to. I'm not sure if it would be worth a pull request for only fixing this one issue, I'm sure there are others I haven't found yet.

Would there be interest in trying to make Plotly more window / document agnostic? It does potentially enable some interesting multi-window webapps with Plotly. This is a big enough undertaking that I'd rather not start it if there's no upstream interest in incorporating it.

If yes, what should be done about the issue of injecting stylesheets? Should they be injected to the document containing the original element given to Plotly? Is that even feasible?

What about getting the correct parent window? document.defaultView would be the 'modern' way to do it, but would limit support of IE versions before 9. Is that an issue? I've also found a polyfill of sorts on StackOverflow.

etpinard commented 8 years ago

@nielsenb-jf thanks so much for your efforts.

that demonstrates fixing interactivity when rendering to a child window. Basically, it renders the drag cover to the document of the child window, instead of always to the parent window where Plotly was instantiated.

Fantastic!

Would there be interest in trying to make Plotly more window / document agnostic? It does potentially enable some interesting multi-window webapps with Plotly.

Yes. There is interest. But, this is not a priority by any means (explaining why no plotly team member has looked at this issue here in details).

what should be done about the issue of injecting stylesheets? Should they be injected to the document containing the original element given to Plotly? Is that even feasible?

I'm not exactly sure how to go about this (I personally don't know much about child windows). We currently inject our stylesheets using Lib.addStyleRule here. Maybe there's a way to make it work on child windows? Alternatively, we could perhaps expose a method on Plotly design to correctly populate child windows and have users that want to use plotly.js in child windows call that method upon window.open.

but would limit support of IE versions before 9. Is that an issue?

We don't officially support IE < 9 at the moment, so no, this is not an issue.

nielsenb-jf commented 8 years ago

I renamed my branch to 'child_window_fixes' to better describe the goal.

I tried testing as many plot types as I could, specifically looking for ones that make assumptions about the current window or document. The only other one that showed issues that I found were ones using range slider / selectors. The issue was the same as for other plots, the component actually managing the selection events was being attached to the parent, instead of to the child.

I also corrected an issue with restyle on plots in a child window not correctly reflecting the child window size.

So the question gets to be, should we clear up as many assumptions as possible, even if they don't cause issues? Or should things that are okay with happening in the parent window stay the way they are now? For example, the 'tester' element is currently appended to the parent window, not the child. This doesn't seem to be an issue, and I could see arguments for either way being "correct".

Yes. There is interest. But, this is not a priority by any means (explaining why no plotly team member has looked at this issue here in details).

I completely understand its not a priority for the team. It doesn't seem to be a priority for other users either since I seem to be the first person to stumble on it. It just seems to me that since Plotly takes a component as an argument, it shouldn't matter where that component is. Either way, I'm willing to try to make this work on my own with guidance when necessary.

I'm not exactly sure how to go about this (I personally don't know much about child windows). We currently inject our stylesheets using Lib.addStyleRule here.

I'm not either. I definitely need to look into this more. Any further advice you have would be greatly appreciated!

We don't officially support IE < 9 at the moment, so no, this is not an issue.

Good. nielsenb-jf/plotly.js@f10706dd45bc372239ae39184d4af2fc75375b49 wouldn't work with it.

etpinard commented 8 years ago

Thanks again for your efforts!

Or should things that are okay with happening in the parent window stay the way they are now?

I would vote :+1: for this.

Plotly takes a component as an argument, it shouldn't matter where that component is.

Yes. No doubt about it.

So I guess this leave us to figure out what to do with our style sheets.

Perhaps we should move the Lib.addStyleRule calls to the main Plotly.plot routine (with a check to not replicate the rules in <style>) so that every plotly.js graph is guaranteed to be accompanied with the correct style sheets.

@nielsenb-jf Would you be interested in trying that out?

nielsenb-jf commented 8 years ago

New branch called child_css that incorporates the previous changes, as well as injecting styles in Plotly.plot. Everything I've tested seems to work.

I changed pull_css to generate a plotcss.js file that does nothing but export the style rules. Those rules are then injected by the new plotcss_injector. The injector will not inject a rule if there is already a matching rule in one of the target document's stylesheets. This prevents redundant rules when plot is called multiple times on a document.

addStyleRule had to be modified to not keep a reference to the stylesheet being modified. Otherwise, the wrong stylesheet would be modified when plot is called first on the parent window, then on a child, or vice versa. If the document already has stylesheets, it defaults to adding the rule to the first one. I'm not sure if that's the best idea or not, but the only other idea I could come up with is to always modify the last stylesheet, assuming it was the one injected.

etpinard commented 8 years ago

@nielsenb-jf great! Looks like you've got all the pieces together.

Here's a few recommendations before submitting your PR:

gd._document = gd.ownerDocument.defaultView || window.document;

// and then use gd._document afterwards in the plotting code
nielsenb-jf commented 8 years ago

I've got everything but the tests put together in the 'child_window_fixes' branch. Remaining work will happen there.

nielsenb-jf commented 8 years ago

I've added some tests to the child_window_fixes branch.

Are we sure we want the tests in the plot_interact_test suite? It feels a little cleaner to me to have them be their own thing. I could also see them in with the plot_api_test suite since the CSS gets injected in the plot method, which is defined in plot_api. I'll defer to your judgement, it's not like we can't cut/paste the code somewhere else later.

Additionally, notice the buildFullSelector function. It appears both Firefox and Chrome do some processing to CSS selectors (and the rules themselves if you dig into it some) during an insertRule call. The result is equivalent, but it makes it difficult to test rules are inserted. I've done my best to make buildFullSelector format the selector the way Firefox and Chrome seem to, but I can't find any documentation anywhere that guarantees that will stay consistent. If I can find some documentation, I'll add some tests. Note that buildFullSelector gets used in the tests to check all selectors in plotcss were inserted properly, but due to the aforementioned processing of rules, there is no testing to guarantee the rule itself is actually correct.

etpinard commented 8 years ago

@nielsenb-jf Thanks so much!

Are we sure we want the tests in the plot_interact_test suite?

I agree. Your new test cases would look a lot better in a suite of their own. I'd call it plot_css_test.js, but I'll leave it up to you to decide the file name.

Additionally, notice the buildFullSelector function. It appears both Firefox and Chrome do some processing to CSS selectors (and the rules themselves if you dig into it some) during an insertRule call.

Your new css/helpers.js module looks great. Nice and DRY.

I think you're ready to make a PR to the main repo.

Squash those lint/formatting commits and PR away :rocket:

nielsenb-jf commented 8 years ago

I fixed an issue with notifiers not appearing in the correct window, and with the _document and _plotCSSLoaded properties not being cleaned up on purge.

Assuming you don't see anything upsetting in those last commits, I'll squash up a PR.

etpinard commented 8 years ago

Looks great. Sqaush away!

nielsenb-jf commented 8 years ago

So git is being snotty about squashing a couple of the formatting commits due to some of the merging back and fourth I did (whoops). Also, I did a git rebase after opening the pull request...

Anyway, as per the contribution guide, the pull request to master on my fork.

etpinard commented 8 years ago

@nielsenb-jf Great. Now please make a PR to this (main) repo. Thank you very much,

nielsenb-jf commented 8 years ago

Looks likes a new plan of attack is needed (#826, #822). I put some thoughts on #829.

Another option would just be to update the documentation to say the element passed on plot must be in the document Plotly is loaded in... :smile:

IsmaelAlvarez commented 7 years ago

got same kind of problemas w the modebar and the buttons, got a screenshot from safari and chrome, same webpage safari: screen shot 2017-01-17 at 12 13 50 pm chrome: screen shot 2017-01-17 at 12 13 13 pm

ifll commented 7 years ago

Recently the issue #1360 I started has been added here and been closed.

I think I might have found the bug that caused my issue, and I have commented there what I think it could be so you can track what could be that produces this.

stmudie commented 7 years ago

Hi. I've been developing a polymer 2.0 app, and now want to include plotly. However, because of the issue originally reported in #1433 I'm unable to do this without shimming the DOM with the shadyDOM from polymer 1. Has there any work towards resolving these issues? Thanks!

jackparmer commented 3 years ago

This issue has been tagged with NEEDS SPON$OR

A community PR for this feature would certainly be welcome, but our experience is deeper features like this are difficult to complete without the Plotly maintainers leading the effort.

Sponsorship range: $10k-$15k

What Sponsorship includes:

Please include the link to this issue when contacting us to discuss.

rjcorwin commented 2 years ago

FWIW, here's the latest Plotly SASS compiled into a CSS module that can be imported into a LitElement component. Anyone using LitElement and Plotly.js can copy this to into a file in their project and import it using the "Sharing styles" docs.

rjcorwin commented 2 years ago

Much thanks to @darkengines for his suggestion here.

rjcorwin commented 2 years ago

Lastly, here's a LitElement example of using Plotly.js. Placing the call to Plotly in the updated callback is key to making sure that the #myDiv is present in the shadowRoot of the component.

https://gist.github.com/rjsteinert/6ab3728643a15058c37a2502d59959e2

TotallyInformation commented 2 years ago

Seems unlikely that Plotly will ever be very useful as browsers and w3c standards progress, plotly's assumptions about its environment are increasingly unsustainable. Rather sad since some effort has been made to support browser ECMA module use.

gvwilson commented 1 month ago

Hi - this issue has been sitting for a while, so as part of our effort to tidy up our public repositories I'm going to close it. If it's still a concern, we'd be grateful if you could open a new issue (with a short reproducible example if appropriate) so that we can add it to our stack. Cheers - @gvwilson