Use correct parent document when necessary: fixes 702 #2 #758

nielsenb-jf · 2016-07-20T18:57:06Z

This pull request fixes the issues noted in issue 702. Essentially, if Plotly was provided an element in a different window or tab, the plot would not be styled correctly and interactivity (hovers, drag zoom, etc.) would not work.

The following are the big changes to fix this issue:

Graph divs now keep a reference to their 'parent' document in the `_document' property.
Styles are now injected to the parent document on the first call to Plotly.plot.
Anything that draws over a document for interactivity purposes (eg. dragcovers), now uses the _document reference to ensure it is drawn in the correct document.
Notifiers now require a graph div as the first argument so that notifications are rendered in the correct place

This fixes the issue with the dragcover not being appended to the correct child when the plot is rendered on a window other than the parent.

This fixes the issue with range slider events being tied to the parent window, as opposed to the child, when rendering to a div in a child window.

This fixes the assumption that the plot should be sized based on the window Plotly was instantiated in, instead of the window Plotly is drawing the plot in.

The previous behavior was to inject CSS into the document the Plotly JS is loaded in. This causes plots rendered in a different document to not be styled correctly. Now, the CSS is injected into the correct document during Plotly.plot.

files. Also change the built plotcss file to export the rules, not inject them.

- so that plotting code - which relies on marker.line.width to set the effective 'bargap' does not error out - broken since plotly#124

This fixes the issue with the dragcover not being appended to the correct child when the plot is rendered on a window other than the parent.

- before dd2251d bar for non-numeric size were excluded from the calcdata, which led to wrong bar widths in traces with gaps. However, this made setPositions stack non-numeric bars.

…AF never stops; 2) scene2d.destroy should destroy the traces (cherry picked from commit 8f6f2dd)

…ce dispose loop; null -> delete

…rencing of the GL context or its own canvas

The _plotCSSLoaded property is set to true after injecting the CSS.

…ed in. This reference is used where necessary to ensure functionality when plotting to a window other than the one Plotly was instantiated in.

'inserts styles on initial plot' - Deletes all plotly CSS in the document (in case other tests have run previously), does a plot, and verifies the CSS has been injected. 'inserts styles in a child window document' - Creates a plot in a child window, and makes sure the CSS has been injected. The child window is then closed, no cleanup required. 'does not insert duplicate styles' - Deletes all plotly CSS in the document, plots to the graph div twice, and ensures the required Plotly CSS only occurs once.

This fixes the issue with notifications not appearing in the correct window when plotting to a child window. The notification is now appended to the body of the document the graph div is a child of.

… into child_window_fixes

etpinard · 2016-07-20T19:00:44Z

Hmm. Something went wrong with your git rebase. I'll pull down your branch and rebase the commits when I get the time this week.

nielsenb-jf · 2016-07-20T19:03:20Z

Is it griping about "Commit 212d5f... being a merge, but no -m option given"?

If you figure out how I broke that, I'd love to hear it so I can avoid this again.

etpinard · 2016-07-21T16:36:59Z

@nielsenb-jf I squashed together all commits in your child_window_fixes branch and push to a new branch squash-child-window. I proposed closing this PR and opening a new one off my squash-child-window branch. This would lead to some good and bad news:

Good news: you'll get one authoring commit
Bad news: you'll only get one authoring commit

But one thing is sure: merging 115 commits of git frustration ⏫ won't happen.

Alternatively, you can also try:

fetching the squash-child-window branch unto your fork
run git reset HEAD^ to reset the BIG squashing commit 4cb2c7c
git add and git commit your work into multiple commits
and then git push -f unto your child_window_fixes branch

Thanks again for your hard work!

nielsenb-jf · 2016-07-21T16:46:06Z

I'm on board for one "BIG ASS CHILD WINDOW COMMIT". Thanks for your help!

New PR

nielsenb-jf and others added 30 commits July 12, 2016 15:05

Ensure the dragcover is rendered on the proper element.

5d7904e

This fixes the issue with the dragcover not being appended to the correct child when the plot is rendered on a window other than the parent.

Be consistent with use of ownerDocument.

d69b72d

Merge branch 'master' into coverslip_fix

02ac18f

Ensure range sliders use the correct child document / window.

f10706d

This fixes the issue with range slider events being tied to the parent window, as opposed to the child, when rendering to a div in a child window.

Properly resize plot based on plot parent window.

0bec933

This fixes the assumption that the plot should be sized based on the window Plotly was instantiated in, instead of the window Plotly is drawing the plot in.

Merge branch 'master' into child_window_fixes

bac79fe

Don't hardcode CSS in plotcss_injector, instead use the generated style

5f57f08

files. Also change the built plotcss file to export the rules, not inject them.

Merge branch 'master' into child_window_fixes

a1204ec

Fix formatting.

e19f8d8

bump karma to 1.1.0

5109efc

bump eslint to 3.0.0

4defbab

bar: set marker.line.width dflt to 0

4a04474

- so that plotting code - which relies on marker.line.width to set the effective 'bargap' does not error out - broken since plotly#124

test: add bargap 0 mock

eec4a20

test-runner: check if baseline img exist before px comparison

d0ad30f

Ensure the dragcover is rendered on the proper element.

855a339

This fixes the issue with the dragcover not being appended to the correct child when the plot is rendered on a window other than the parent.

Be consistent with use of ownerDocument.

aa1c3c4

bar: skip over bars with non-numeric size in setPositions

10c751b

- before dd2251d bar for non-numeric size were excluded from the calcdata, which led to wrong bar widths in traces with gaps. However, this made setPositions stack non-numeric bars.

test: add stack with gaps mock

89c7c79

test-assets: add msg-extra argument to is-close array matchers

198390c

test-assets: don't try to compute dist between non-numeric elements

275b110

lint: flatten bar test suite

36216f9

test: add bar calc / setPositions tests

8bea10a

fix issue with bar releative mode when first bucket is 0 or string

a2ec12d

lint: bar setPositions comments + spaces

ae45238

test: add bar setPositions test cases for relative of pos 0 / -0

6d0539a

Memory leak fix candidates: 1) purge must destroy scene2d otherwise r…

36ff540

…AF never stops; 2) scene2d.destroy should destroy the traces (cherry picked from commit 8f6f2dd)

Lint

4c1fc94

Call cleanPlot from newPlot directly; Changed the location of the tra…

57ce709

…ce dispose loop; null -> delete

Adding test cases that check that Plotly.newPlot indeed causes derefe…

56f026d

…rencing of the GL context or its own canvas

nielsenb-jf added 15 commits July 20, 2016 13:26

Move addStyleRule code to injectStyles, delete lib.addStyleRule.

ae9b2ba

Only injectStyles if they haven't already been injected.

eb536d4

The _plotCSSLoaded property is set to true after injecting the CSS.

Graph div now keeps a _document reference to the document it's render…

f03b7b3

…ed in. This reference is used where necessary to ensure functionality when plotting to a window other than the one Plotly was instantiated in.

Merge branch 'master' into child_window_fixes

bc07d60

Make sure graph div _document property is set for drag element tests.

2f25141

Move some common code to a helpers file so they can be used in tests.

b0e9c0c

Use CSS helper functions.

1efd493

Move CSS injection tests to plt_css_test.js.

000104e

Use @src and @build macros.

413b731

Notifier now requires a graph div as the first argument.

d668340

This fixes the issue with notifications not appearing in the correct window when plotting to a child window. The notification is now appended to the body of the document the graph div is a child of.

Cleanup _document and _plotCSSLoaded in plots.purge.

2e8a1ec

Merge branch 'notifier_fixes' into child_window_fixes

f8794bc

Merge branch 'master' into child_window_fixes

5c0fa8d

Merge branch 'child_window_fixes' of github.com:nielsenb-jf/plotly.js…

114c13d

… into child_window_fixes

nielsenb-jf mentioned this pull request Jul 20, 2016

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

Closed

etpinard added feature something new status: in progress bug something broken labels Jul 20, 2016

nielsenb-jf mentioned this pull request Jul 21, 2016

Single commit of child window fixes #763

Closed

nielsenb-jf closed this Jul 21, 2016

nielsenb-jf mentioned this pull request Jul 21, 2016

Plotting in child window #764

Merged

etpinard added type: duplicate and removed status: in progress bug something broken feature something new labels Jul 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use correct parent document when necessary: fixes 702 #2 #758

Use correct parent document when necessary: fixes 702 #2 #758

nielsenb-jf commented Jul 20, 2016

etpinard commented Jul 20, 2016

nielsenb-jf commented Jul 20, 2016

etpinard commented Jul 21, 2016

nielsenb-jf commented Jul 21, 2016

Use correct parent document when necessary: fixes 702 #2 #758

Use correct parent document when necessary: fixes 702 #2 #758

Conversation

nielsenb-jf commented Jul 20, 2016

etpinard commented Jul 20, 2016

nielsenb-jf commented Jul 20, 2016

etpinard commented Jul 21, 2016

nielsenb-jf commented Jul 21, 2016