/dash-preview: Fix window size

tarzzz commented 4 years ago

Detailed explanation in: https://github.com/plotly/streambed/issues/13458#issuecomment-534274326

tarzzz commented 4 years ago

@antoinerg or @etpinard Please review.

antoinerg commented 4 years ago

y/streambed#13458 (comment)

If I understand correctly, the idea is to have the size of the viewport match the size of the page so that when printing to PDF, figures are already appropriately sized.

For this to work properly, we need to know the DPI of the screen. Right now, it is hardcoded to 96: https://github.com/plotly/orca/blob/1d56807e1146aa5a28d81ad46183b6f45346e96a/src/component/plotly-dash-preview/constants.js#L1-L2 This will not always be the case. I imagine that xvfb defaults to 96 so this is why this fix works in the Docker image. We should probably specify this DPI value in case the default changes in the future. It should also be noted that the current fix will not work for desktop users with a high-density display.

Finally, although I understand this needs to be fixed urgently, I am getting a bit uneasy about merging things that aren't tested on CI.

antoinerg commented 4 years ago

Actually, I think it is because we need to round off window size as well:

If that's the case, we should also round off the window size if the user specifies pageSize as a string: https://github.com/plotly/orca/blob/5c1f71d6d8ce046ec167997b722228820117f999/src/component/plotly-dash-preview/parse.js#L39-L40

antoinerg commented 4 years ago

I don't think commit f2a2bad is necessary. It would be much cleaner and DRY to round off the numbers on line 59 just after the following block just before passing them to the renderer: https://github.com/plotly/orca/blob/574241a3a9019c9c1580cf469c47857aaedd1ca6/src/component/plotly-dash-preview/parse.js#L39-L60

antoinerg commented 4 years ago

Okay so I can see that with your latest commit we go from a PDF with a figure not filling the page (before.pdf) to one that does (after.pdf).

antoinerg commented 4 years ago

This is related to (another) issue: https://github.com/plotly/dash-snapshots/issues/62 . Looking at parse.js, it seems to me that if we pass body.pageSize as a string, we won't get the right result. It is a bit unfortunate in my opinion that we have two ways of specifying the size of the page (via body.pageSize and via body.pdf_options.pageSize) but we'll have to live with it.

Please test that for the 4 possible input body:

body.pageSize is a string
body.pageSize is an object with width and height
body.pdf_options.pageSize is a string
body.pdf_options.pageSize is an object with width and height we end up with a result.browserSize and a result.pdfOptions.pageSize that are equivalent (ie. same size). Finally, add a test to lock down the current behavior when both body.pageSize and body.pdf_options.pageSize are specified. Which one wins?

cc @tarzzz @ycaokris

tarzzz commented 4 years ago

I have changed the base to master because we don't know yet if it needs to go to 3.3 release..

antoinerg commented 4 years ago

@tarzzz By the way, I can confirm that this component mishandles 2 of the 4 scenarios described in https://github.com/plotly/orca/pull/250#issuecomment-535323595

antoinerg commented 4 years ago

With commit https://github.com/plotly/orca/pull/250/commits/0943a790019855649f6d9d223395c603fd94f1e0 I introduced new tests that enabled me to discover bugs in the way we used to parge pageSize.

It fixes:

[x] handling of pageSize defined as a string
[x] handling of pdf_options.pageSize defined as an object with width and height in microns
[x] It also guarantees that the browser size and the PDF page size are the same (assuming a DPI of 96) to ensure that no page reflow is necessary (essentially a workaround for issue https://github.com/plotly/plotly.js/issues/1275)

antoinerg commented 4 years ago

@etpinard I'm taking over this PR so I might need you to review it.

antoinerg commented 4 years ago

I have changed the base to master because we don't know yet if it needs to go to 3.3 release.

If this doesn't go into 3.3, this means that snapshots will have figures that are not sized appropriately. I am pretty sure that @chriddyp and @ycaokris want this in. I will let them confirm whether this is the case or not.

cc @tarzzz

ycaokris commented 4 years ago

I have changed the base to master because we don't know yet if it needs to go to 3.3 release.

If this doesn't go into 3.3, this means that snapshots will have figures that are not sized appropriately. I am pretty sure that @chriddyp and @ycaokris want this in. I will let them confirm whether this is the case or not.

cc @tarzzz

I'd like to know what's further steps we need to do in order to merge this into 3.3 release. If we're able to get it fixed before 3.3 due we could possibly skip temporary match for 3.2.3 https://github.com/plotly/dash-snapshots/pull/64

etpinard commented 4 years ago

:dancer: thanks for taking care of this!

I made one non-blocking https://github.com/plotly/orca/pull/250#discussion_r329718833

plotly / orca

/dash-preview: Fix window size #250