Print directly from Edit/Share/New pages

calculuschild commented 4 months ago

Implements #2742 Supersedes #3464 , as it solves the same issue and removes the need for /print endpoint entirely.

Appears to work fine with legacy/V3, cross-page variables, etc. All functional features in the brewRenderer are printed correctly.

Open Tasks:

[x] When scrolled to the bottom of a long (7+ page) document, after printing, scrolling back up reveals that the top few pages have disappeared. Resizing the page, etc. makes them reappear. Seems to be due to the overflow-y: unset; line in the @media print CSS somehow?

[x] Fix hotkey printing when iframe is focused https://github.com/naturalcrit/homebrewery/pull/3491#issuecomment-2126182602
[x] Need to fully remove the /print page and associated routing code.
[x] Printing via hotkey from iframe focus should only work on Edit/Share/New

5e-Cleric commented 4 months ago

When scrolled to the bottom of a long (7+ page) document, after printing, scrolling back up reveals that the top few pages have disappeared. Resizing the page, etc. makes them reappear. Seems to be due to the overflow-y: unset; line in the @media print CSS somehow?

That doesn't make any sense, @media print is very clear at what it does

calculuschild commented 4 months ago

Exactly. It doesnt make any sense. And yet, if you try it, you will see that removing that line of CSS prevents the issue.

G-Ambatte commented 4 months ago

I don't seem to be able to reproduce the issue, either on the deployment or in my local build. Can you share a link to the test doc on the deployment for further experimentation please?

calculuschild commented 4 months ago

I don't seem to be able to reproduce the issue, either on the deployment or in my local build. Can you share a link to the test doc on the deployment for further experimentation please?

It's just a blank document with 7 pages.

\page

\page

\page

\page

\page

\page

Scroll to the bottom. Print. Close print dialog. Scroll to the top. The top 2 pages are invisible.

Gazook89 commented 4 months ago

Doesn't happen for me, in chrome or firefox. Tested with just the blank pages and with additional stuff, in the deployment. I can try checking out to local if necessary

calculuschild commented 4 months ago

Hmmmm. Well if it's just me.... Let me just try clearing my cache and everything. Maybe Chrome is just acting up. I will try to record a video as well if it's still there.

G-Ambatte commented 4 months ago

We might want to catch CTRL-P from the iFrame.

When Brew Renderer is focused and CTRL-P pressed:

on Share Page:
on Edit Page:

Not 100% on how we might achieve that yet, though - potentially a future improvement if it proves too difficult for this iteration.

calculuschild commented 4 months ago

I tested the issue again just now with a clean install of Chrome on two different machines. It occurs for me in local and in the deployment, on both Chrome and Edge, but not Firefox. Occurs on all of /new, /share, and /edit.

Test brew: https://homebrewery-pr-3491.herokuapp.com/share/2qpSNU5IzHjO

https://github.com/naturalcrit/homebrewery/assets/5878534/fc804f1f-eb14-46f1-aa97-09db0047f5c9

G-Ambatte commented 4 months ago

WTF

It didn't happen It didn't happen ...I changed page size to Letter from A4, and it happened

calculuschild commented 4 months ago

We might want to catch CTRL-P from the iFrame.

I think I have a solution for this.

G-Ambatte commented 4 months ago

WTF

It didn't happen It didn't happen ...I changed page size to Letter from A4, and it happened

...and now it's not doing it any more. This makes diagnostics considerably more difficult.

EDIT: Okay, I seem to have a reasonably reliable issue reproduction method.

Scroll the BrewRenderer preview to the last page (7).
Click GET PDF.
Change page size to A4, then to Letter.
Complete the Print event (click Save > enter filename > OK, or click Cancel).
Scroll up the BrewRenderer preview.
Observe that pages at the beginning of the document are not rendered.

Pressing CTRL+P from the Editor does NOT exhibit the same behaviour.

I went through and added content to each page (literally, only the page number: 1, \page, 2, \page, etc.) Using this reproduction method, I can see that while the third page is present, it has no content, while pages 1 and 2 are not rendered at all. I suspect that for some reason, after the print event, the BrewRenderer preview window is cleared and only pages within 3 of the initial page are rendered, and this does not update as the preview window is scrolled, despite state.viewablePageNumber updating.

5e-Cleric commented 4 months ago

Okay, who made a pact with a fey? who was it?

calculuschild commented 4 months ago

Ok, so.... after lots of fiddling, I think I have a workaround, but it's dumb.

It appears that triggering the print dialog forces a reflow of the brewRenderer, and somehow it is using the @media print criteria for determining if an element is off the page for painting. Triggering a re-render in React isn't enough, because all that does is update the DOM, and the DOM hasn't changed.

If I add these lines to the printing step, it basically forces a repaint of the DOM, which works, but is really dumb.

window.frames['BrewRenderer'].contentWindow.print();
let node = window.frames['BrewRenderer'].contentDocument.getElementsByClassName('brewRenderer').item(0);
node.style.display='none';
node.offsetHeight; // accessing this is enough to trigger a reflow
node.style.display='';

5e-Cleric commented 4 months ago

It is using the @media print criteria for determining if an element is off the page for painting.

How can you know that? and are we sure about it?

There might be an alternative to the @media print

what if we apply css right before printing via js and then remove it later?

calculuschild commented 4 months ago

How can you know that? and are we sure about it?

I mean, at some point its just a best guess based on what I've seen. We know for sure is that it is specifically the overflow-y: unset (or overflow-y: visible) in the @media print dialog leading to the issue, but not specifically why. And we know that a DOM repaint will restore the missing pages.

We also know that opening the print dialog triggers a repaint of the brewrenderer in the background. In the video below, I have Chrome set to flash green any component that gets repainted. You can see the brewRenderer element flashes green behind the print dialog when it first opens, but it does not flash green when the dialog closes. So we can also guess that the pages are disappearing when the print dialog first opens.

https://github.com/naturalcrit/homebrewery/assets/5878534/c780c7b5-f9f8-46e1-b1da-e4b2122f2cc5

Here my new code that adds a repaint added after the print dialog closes. You can see another green flash when I close the dialog:

https://github.com/naturalcrit/homebrewery/assets/5878534/4d6f08df-77e3-4063-a07a-49559a79a2f5

calculuschild commented 4 months ago

what if we apply css right before printing via js and then remove it later?

I tried this as well. This does mostly the same thing as my solution, since since removing the CSS causes a repaint when the dialog closes. But unfortunately it adds two downsides:

1) While the print dialog is up, the webpage in the background looks like the print output 2) This in turn causes you to lose your scroll position because the elements have shifted around

calculuschild commented 4 months ago

Ok, I think the two major issues are fixed. Missing pages are handled and CTRL-P is now captured in the iframe.

G-Ambatte commented 4 months ago

Ok, I think the two major issues are fixed. Missing pages are handled and CTRL-P is now captured in the iframe.

Confirmed; my reproduction method no longer produces the issue.

5e-Cleric commented 4 months ago

Using this PR, users can now print all pages, we might want to block printing in account page.

Changelog, Home page, user page, they are all printable.

5e-Cleric commented 4 months ago

Shouldn't this PR delete the print page altogether apart from adding this function? Or are we keeping it for the other mentioned reasons?

calculuschild commented 4 months ago

Shouldn't this PR delete the print page altogether apart from adding this function? Or are we keeping it for the other mentioned reasons?

Look up at the original post. It's already one of the open tasks.

calculuschild commented 4 months ago

Using this PR, users can now print all pages, we might want to block printing in account page.

I just made a tweak so hotkey printing when focused on the brewRenderer iFrame only generates the full PDF on Share/Edit/New.

The other pages now just fall back to the default Chrome print action, unchanged from their current behavior on the live site. I don't think we need to entirely block all forms of printing.

calculuschild commented 4 months ago

Changed the function to printCurrentBrew() so it isn't confused with "page" components.

naturalcrit / homebrewery

Print directly from Edit/Share/New pages #3491

Open Tasks: