1648064 - loading="lazy" images are not printed unless the were scrolled to

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P1)

Description

[Peter Molnar]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:77.0) Gecko/20100101 Firefox/77.0

Steps to reproduce:

Open a site which has images with native lazy loading (using loading="lazy" on the img elements) out of the immediately visible area and send the page to print.

Actual results:

Only the images which have been scrolled to are printed.

Expected results:

This is tricky question: Firefox is doing exactly what the user asked to do. However, because the rest of the content, which is not under loading=lazy is printed, the images with loading=lazy should also be printer - at least this would be the logical outcome.

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: loading="lazy" images are not printed

Comment 2

[Alice0775 White]

Attachment: testcase

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Yeah, this is a bit tricky, because for printing we create a clone of the image... And obviously the image hasn't loaded yet by then if lazy-loading... :(

Comment 5

[C. Martin]

The screenshot functionality has the same problem when choosing to screenshot the whole page.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I'll poke at the printing issue. Screenshots probably need a different fix though. Henrik, are you the right person to take a look for screenshots? It seems screenshots for the whole page should trigger lazy image loads and wait for them to finish.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

I'll poke at the printing issue. Screenshots probably need a different fix though. Henrik, are you the right person to take a look for screenshots? It seems screenshots for the whole page should trigger lazy image loads and wait for them to finish.

We have various screenshot code usage in our tree. Personally I only implemented screenshots for Marionette and the Remote Protocol. So it would be more a question for Matt who implemented drawSnapshot().

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1648064 - Fix usage of printsettings in printpreview_helper. r=jwatt

For some reason that I don't want to dig into after a whole day of
debugging, nsDocumentViewer::GetGlobalPrintSettings always returns a new
object. Make sure to get a hand on the initial settings objects and use
that. That way we can remove the XXX comment, and use the print
settings as expected, which is useful because I'm going to add a test
whose reference is using background-color, and I need to change the
settings for us to print backgrounds.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1648064 - Fix usage of printsettings in printpreview_helper. r=jwatt

For some reason that I don't want to dig into after a whole day of
debugging, nsDocumentViewer::GetGlobalPrintSettings always returns a new
object. Make sure to get a hand on the initial settings objects and use
that. That way we can remove the XXX comment, and use the print
settings as expected, which is useful because I'm going to add a test
whose reference is using background-color, and I need to change the
settings for us to print backgrounds.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1648064 - Don't ignore load blockers for print job resource loading. r=jwatt

I don't think it's correct. It was added in bug 468568, but with no
clear reason other than "it fixes the tests".

Tests are passing with this change, and without it one of the tests I'm
about to add fails, because image loads for <picture> happen async (see
ImageLoadTask).

Depends on D81776

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1648064 - Fix two issues with notifications for media feature value changes. r=#style

We need to notify unconditionally because even if we didn't have
stylesheets, we could have responsive content which needs to change
source.

We need to notify the document even if the pres shell is not
initialized, as it might be our last chance to notify the responsive
content. This happens for printing, and makes my srcset test fail.

Depends on D81777

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1648064 - Switch DOM images to work like CSS images for the purposes of printing. r=tnikkel!,smaug!

Make them perform the image load (if needed), instead of copying the
image requests from the original document.

This is needed for CSS for stuff like:

@media print {
#foo::before {
content: url(bar.png);
}
}

And so on. For images, we should do this as well. Nothing prevents you
from doing:

<picture>
<source srcset="print.png" media="print">
<source srcset="screen.png" media="not print">
<img>
</picture>

And that should in theory work. It works after this patch, and I added a
test for that.

This patch is a bit bigger than I'd like, but I didn't find a more
reasonable way to split it up.

Making static docs able to do image loads is most of the patch and is
mostly straight-forward. This allows to remove the hacky "change the
loading document" thing that CSS images do, which is just working around
the CSP of the print document.

I need to enable background colors in printpreview_helper so as to be
able to have a reference page for all the different image types.

Depends on D81778

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1648064 - Disable lazy loading for print documents. r=hiro

This, along with the previous patches, allow lazy-loaded images to show
up in print, even if they haven't been loaded otherwise.

Depends on D81779

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1648064 - Make print preview documents wait properly for the document to be loaded. r=smaug

Otherwise my test fails intermittently on CI. We need to block on all
the load blockers because stuff like responsive images doesn't fire the
load directly but they do that as a micro task (blocking the load
event).

Depends on D81780

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1648064 - Make static clones have the CSP of the original doc. r=smaug

Depends on D81989

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1648064 - Make service workers of the original document intercept the static document's requests. r=smaug!,asuth!

Depends on D81997

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ab7e34d7df3
Fix usage of printsettings in printpreview_helper. r=jwatt

Comment 18

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/5ab7e34d7df3

Comment 19

[Phabricator Automation]

Comment on attachment 9160510 [details]
Bug 1648064 - Fix two issues with notifications for media feature value changes. r=#style

Revision D81778 was moved to bug 1648839. Setting attachment 9160510 [details] to obsolete.

Comment 20

[Matt Woodrow (:mattwoodrow)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #7)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

I'll poke at the printing issue. Screenshots probably need a different fix though. Henrik, are you the right person to take a look for screenshots? It seems screenshots for the whole page should trigger lazy image loads and wait for them to finish.

We have various screenshot code usage in our tree. Personally I only implemented screenshots for Marionette and the Remote Protocol. So it would be more a question for Matt who implemented drawSnapshot().

Hiro, do you know what would be required to force loading of all images for a snapshot?

All our various screenshot/snapshot code goes through nsLayoutUtils::PaintFrame, which will sync decode images, but I guess won't load them.

The screenshots browser functionality is currently using CanvasRenderingContext2D::DrawWindow, which is synchronous, and won't work if we need to asynchronously wait for image loads to start.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #20)

Hiro, do you know what would be required to force loading of all images for a snapshot?

Lazy loading images totally rely on the DOMIntersectionObserver implementation, which means it relies on ancestor's scroll ports basically (and it also requires to be triggered by a refresh driver tick).

All our various screenshot/snapshot code goes through nsLayoutUtils::PaintFrame, which will sync decode images, but I guess won't load them.

Right, it won't trigger loading, as far as I can tell.

The screenshots browser functionality is currently using CanvasRenderingContext2D::DrawWindow, which is synchronous, and won't work if we need to asynchronously wait for image loads to start.

Without waiting for asynchronous loads, lazy loading images will not appear there. For printing/print preview, what Emilio tries to do in this bug is to disable lazy loading images in static cloned documents in D81780, so that it starts being loaded in the first place (thus, it blocks document load, that's my understanding). So to me for sceenshots, we need to trigger loading lazy loading images and make sure that all lazy loading images have been loaded, then we need to call PaintFrame? To do that, we need to add a chrome only API to load lazy loading images forcibly. That said, it also affects the original document, so I am not sure it's acceptable or not.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Filed bug 1650677 for the screenshot issue.

Comment 23

[j.j.]

Open Chromium bug
https://bugs.chromium.org/p/chromium/issues/detail?id=875403

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1648064 - Remove some includes from nsPrintJob.h. r=jwatt

I had needed this before to export that header, and it's no longer
needed, but it seems useful anyways.

Depends on D81989

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dee61778b7b0
Remove some includes from nsPrintJob.h. r=jwatt

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1648064 - Allow for some fuzziness in the tests. r=jwatt

Before my patches the background images in table-background-print didn't
seem to load on time.

I tried to make the images* tests non-fuzzy, but the <svg:image> really
ruined the deal. A bit of a shame that they fall into a subpixel
boundary because of page margins. So allow a bit of fuzz there rather
than spending another few days trying :)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fed6928b53d
Switch DOM images to work like CSS images for the purposes of printing. r=tnikkel,smaug
https://hg.mozilla.org/integration/autoland/rev/f735d4f6b21f
Disable lazy loading for print documents. r=hiro
https://hg.mozilla.org/integration/autoland/rev/4ec12eb9788c
Make print preview documents wait properly for the document to be loaded. r=smaug
https://hg.mozilla.org/integration/autoland/rev/2598e2706bd0
Make static clones have the CSP of the original doc. r=smaug
https://hg.mozilla.org/integration/autoland/rev/189401f7b6e8
Make service workers of the original document intercept the static document's requests. r=smaug,asuth
https://hg.mozilla.org/integration/autoland/rev/8ac892c60eda
Allow for some fuzziness in the tests. r=jwatt

Comment 28

[Narcis Beleuzu [:NarcisB]]

Backed out for wp failures on test_printpreview.xhtml

Backout link: https://hg.mozilla.org/integration/autoland/rev/1600e73bdd901c5ac4b831c43e29f73d561aa816
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310739922&repo=autoland&lineNumber=876

Comment 29

[Narcis Beleuzu [:NarcisB]]

Please also check:

• xpchsell failure on test_async_notification_404.js -> https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310739035&repo=autoland&lineNumber=8661
• mochitest failure on test_printpreview.xhtml -> https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310738452&repo=autoland&lineNumber=9520

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1648064 - Disable the URL in the header in printpreview_helper. r=nordzilla

It seems win7 sometimes displaces it slightly. We've hit this in the
past and it's not the point of the test so just get rid of it.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af2e77ea3ee0
Switch DOM images to work like CSS images for the purposes of printing. r=tnikkel,smaug
https://hg.mozilla.org/integration/autoland/rev/b38b31524349
Disable lazy loading for print documents. r=hiro
https://hg.mozilla.org/integration/autoland/rev/136eaa3a0068
Make print preview documents wait properly for the document to be loaded. r=smaug
https://hg.mozilla.org/integration/autoland/rev/73ef86087279
Make static clones have the CSP of the original doc. r=smaug
https://hg.mozilla.org/integration/autoland/rev/b229904070e1
Make service workers of the original document intercept the static document's requests. r=smaug,asuth
https://hg.mozilla.org/integration/autoland/rev/f53428dd4ae3
Allow for some fuzziness in the tests. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/ad46f8c31bb0
Disable the URL in the header in printpreview_helper. r=nordzilla

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc30baaf3115
Some minor fuzz fix.

Comment 33

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/af2e77ea3ee0
https://hg.mozilla.org/mozilla-central/rev/b38b31524349
https://hg.mozilla.org/mozilla-central/rev/136eaa3a0068
https://hg.mozilla.org/mozilla-central/rev/73ef86087279
https://hg.mozilla.org/mozilla-central/rev/b229904070e1
https://hg.mozilla.org/mozilla-central/rev/f53428dd4ae3
https://hg.mozilla.org/mozilla-central/rev/ad46f8c31bb0
https://hg.mozilla.org/mozilla-central/rev/cc30baaf3115

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Hi Emilio, I'm sorta wondering what our best path forward is here for ESR78. Do we have any idea of how widespread lazy-loading usage is? I'm not crazy about uplifting all these patches, especially with the various crash follow-ups. Could we easily disable lazy-loading otherwise?

Comment 35

[Emilio Cobos Álvarez (:emilio)]

Yes, this is not the kind of patch I'd be confident uplifting to ESR. Lazyloading is not too uncommon I guess. It can be disabled if needed via dom.image-lazy-loading.enabled.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Attachment: Bug 1648064 - Disable lazyloading on ESR78. r=hiro

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9168902 [details]
Bug 1648064 - Disable lazyloading on ESR78. r=hiro

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Disables a feature which causes lazy-loaded images to not be printed if they haven't been scrolled to yet.
• User impact if declined: Unexpected printing results, missing images
• Fix Landed on Version: N/A
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just prefs off the regressing feature and returns us to previous behavior users on ESR68 would have already had. I've also verified via Try that the attached testcase prints as expected both with and without this patch (image isn't in the printed PDF unless I've previously scrolled to it in a vanilla build, image is there without scrolling to it with the patched build).
• String or UUID changes made by this patch:

Comment 38

[Julien Cristau [:jcristau]]

Comment on attachment 9168902 [details]
Bug 1648064 - Disable lazyloading on ESR78. r=hiro

approved for 78.2esr, thanks

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/517c384fff74

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9168902 [details]
Bug 1648064 - Disable lazyloading on ESR78. r=hiro

Clearing the approval flag to get this off the needs-uplift radar.