/dash-preview: Fix window size

[tarzzz]

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

[tarzzz]

@antoinerg or @etpinard Please review.

[antoinerg]

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:

orca/src/component/plotly-dash-preview/constants.js

Lines 1 to 2 in 1d56807

	const pixelsInInch = 96
	const micronsInInch = 25400

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]

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:

orca/src/component/plotly-dash-preview/parse.js

Lines 39 to 40 in 5c1f71d

	if (cst.sizeMapping[result.pdfOptions.pageSize]) {
	result.browserSize = cst.sizeMapping[result.pdfOptions.pageSize]

[antoinerg]

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:

orca/src/component/plotly-dash-preview/parse.js

Lines 39 to 60 in 574241a

	if (cst.sizeMapping[result.pdfOptions.pageSize]) {
	result.browserSize = cst.sizeMapping[result.pdfOptions.pageSize]
	} else if (body.pageSize && isPositiveNumeric(body.pageSize.width) &&
	isPositiveNumeric(body.pageSize.height)) {
	result.browserSize = {
	width: Math.ceil(body.pageSize.width * cst.pixelsInMicron),
	height: Math.ceil(body.pageSize.height * cst.pixelsInMicron)
	}
	result.pdfOptions.pageSize = {
	width: Math.ceil(body.pageSize.width),
	height: Math.ceil(body.pageSize.height)
	}
	} else {
	return errorOut(
	400,
	'pageSize must either be A3, A4, A5, Legal, Letter, ' +
	'Tabloid or an Object containing height and width ' +
	'in microns.'
	)
	}
	sendToRenderer(null, result)

[antoinerg]

Is this really necessary? I thought this line: createBrowserWindowOpts['enableLargerThanScreen'] = true would make the result independent of the screen resolution 🤔

[tarzzz]

I was under the same impression, but apparently it doesn't. Should I remove that option, or let it be there?

[antoinerg]

That is unfortunate... I wonder if that is fixed in recent versions of electron (see branch pr-bump-electron-6).

Anyway, changing the resolution of the screen seems like a short-term solution: what if a user needs a bigger browser size than 1024 by 768... We're essentially putting a limit on how big a page we can print. If so, it should be clearly stated somewhere. By the way, what do you mean when you say it "works" (ie. how/what are you testing exactly)?

[antoinerg]

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]

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]

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

[antoinerg]

@tarzzz By the way, I can confirm that this component mishandles 2 of the 4 scenarios described in #250 (comment)

[antoinerg]

With commit 0943a79 I introduced new tests that enabled me to discover bugs in the way we used to parge pageSize.

It fixes:

• handling of pageSize defined as a string
• handling of pdf_options.pageSize defined as an object with width and height in microns
• 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 Charts don't resize appropriately for print view plotly.js#1275)

[antoinerg]

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

[antoinerg]

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]

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]

💃 thanks for taking care of this!

I made one non-blocking #250 (comment)