Page MenuHomePhabricator

Download all page images for printing
Closed, ResolvedPublic5 Estimated Story Points

Description

Problem

With the recent work on T179914, we would like to download all lazily loaded images at print time so they appear as pictures instead of greyed out boxes.

Design

Step1: show the download action

land.png (1×750 px, 186 KB)

Step 2: download action will trigger standard spinner in place of the icon

loading.png (1×750 px, 187 KB)

Step 3: PDF with images in the print mode

pdf.png (1×750 px, 166 KB)

Note: ignore the fact that this mock is on iOS, that is just convenient to produce --

For non-js browsers

This feature of fetching images will not available on non-js browsers as of. they will recieve pdf's without the images that are not in the DOM
It just works.
Non js browsers will have already loaded images due to our fallbacks e.g. noscript tag or minimum js.

Developer notes

As discussed previously, concerning the issues of printing lazy loaded images with a print button, we have full control of loading images before print. Baha previously did a POC demonstrating this to be possible.

To support this, we'll need to make some changes to the Skin class inside MobileFrontend.

Open questions before sign-off

  • is the 3seconds timeout enough to load most of images on production servers for mobile devices?

Acceptance criteria

  • Skin class should have a function that allows the loading of images. This works like the loadImages class however drops the isElementCloseToViewport check. In order to keep this DRY a refactor of loadImages is likely - either to add a parameter or to add an additional method loadAllImages

Testing plan

Using mobile mode on beta cluster or http://reading-web-staging.wmflabs.org/ visit articles with many images (ex: Barack Obama).

  • onClick it tries to load all lazy-loaded articles (check the network tab)
  • the print preview/printed article should contain all images, not only the one from the top)
  • spamming button brings only one print window
  • try to print, dismiss print dialog, button should be in "download" state and it should be possible to trigger print action once again
  • once all images are loaded, click should bring the print dialog immediately

Please use mobile devices using cellular connection to verify that 3s delay is more than enough to download most of the images.

Sample code

$('#content').find( '.lazy-image-placeholder' ).toArray().forEach((placeholder)=> {
  var $placeholder = $( placeholder ),
      width = $placeholder.attr( 'data-width' ),
      height = $placeholder.attr( 'data-height' ),
      $downloadingImage = $( '<img>' ).attr( {
        'class': $placeholder.attr( 'data-class' ),
        width: width,
        height: height,
        src: $placeholder.attr( 'data-src' ),
        alt: $placeholder.attr( 'data-alt' ),
        style: $placeholder.attr( 'style' ),
        srcset: $placeholder.attr( 'data-srcset' )
      } );
      $downloadingImage.addClass( 'image-lazy-loaded' );
      $placeholder.replaceWith( $downloadingImage );
});
// async timeout allows images to begin downloading before print occurs.
setTimeout(() => {
window.print();
},1000)

Open questions

  • If loading images is taking considerable time e.g. say 5 seconds, it's not clear whether we should show an intermediate display - e.g. turn the download button into a spinner or show some kind of dialog to the user informing them we are preparing the document for printing or

Event Timeline

ovasileva renamed this task from Investigate how to download all page images for printing to [Spike] Investigate how to download all page images for printing.Nov 9 2017, 10:31 AM
ovasileva triaged this task as High priority.
ovasileva moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.
Jdlrobson renamed this task from [Spike] Investigate how to download all page images for printing to Download all page images for printing.Nov 9 2017, 7:54 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson added subscribers: Nirzar, Jdlrobson.

No spike needed here. We just need some thoughts from @Nirzar on how to deal with the async nature of preparing the page for print (if any).

@Jdlrobson I agree. I will update the description with desired flow. i now remember we said, when we have the download action we can actually trigger fetching of all images prior to enter cmd+P

Non js browsers will have already loaded images due to our fallbacks e.g. noscript tag or minimum js.

The problem is the solution!

giphy-downsized (30).gif (200×250 px, 544 KB)

Change 391710 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Provide capabilities in Skin to load all images in page

https://gerrit.wikimedia.org/r/391710

Change 391711 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Load all images during print action

https://gerrit.wikimedia.org/r/391711

Change 391710 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Provide capabilities in Skin to load all images in page

https://gerrit.wikimedia.org/r/391710

Change 391711 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Load all images during print action

https://gerrit.wikimedia.org/r/391711

is there any place I can test this? I tried on beta cluster and staging and I can't see the spinner or the images, asking so I can impact on T181261

Per description the beta cluster. I've also added to http://reading-web-staging.wmflabs.org/ in case that is more useful.

Looks good on staging with the Barack Obama article. I also tried a few other articles and a couple of platforms (devices).

Per description the beta cluster.

I tried it on beta earlier and it didn't work. could test on staging now.

Looking good on staging! this will improve overall experience a lot!

one minor issue to document, not sure if we can solve it but just good to know.
If the user hits back while the browser print is generating the pdf, the spinner is shown and is not spinning. I guess we don't have an event for that to reset the download button state.

A question - @Jdlrobson - how does the spinner currently work? As in are we:

  1. showing the spinner
  2. downloading all images
  3. hiding the spinner
  4. calling the browser print

or

  1. showing the spinner
  2. downloading all images
  3. calling the browser print

As in, what happens to the spinner in the time between getting all the images and displaying the print options? If it's the latter, we can probably close T181261: Provide system feedback while the Android opens up the print dialog using download article action

I'm seeing the second behaviour on staging.... yay!?

@ovasileva - the current spinner logic is:

  • after button click show spinner and start downloading images
  • after 3 seconds or when all images are downloaded (which happens first)
  • call browser print by window.print() call
  • hide the spinner

EDIT: Changed the logic after yesterdays patch: https://gerrit.wikimedia.org/r/#/c/393651

Hey there, the browser print does not show up on my Nexus 5 (Android 6.0.1) on Chrome app v62.0.3202.84.
Video of the behavior: https://youtu.be/wyltb331nrE

FWIW It is working on my Pixel (8.0.0) on the same Chrome browser version.

@RHo what happens if you try to print via the browser's file menu? e.g. without the button. It looks like for some reason the print dialog fails to load without explanation 🤔

hi @Jdlrobson - I just realized my Chrome app was still open from when the download button on mweb prod crashed the print function (noted in T181261). I forced restarted Chrome and it seems to be working now on staging.
Back on the mweb version, I am able to reproduce the error whereby the print download will crash after being invoked more than once. I get a "Print spooler" error, and subsequently the print dialog no longer shows again with no further error messaging.

image.png (1×1 px, 218 KB)

If the user hits back while the browser print is generating the pdf, the spinner is shown and is not spinning. I guess we don't have an event for that to reset the download button state.

The spinner is a gif and will only stop spinning if the browser itself freezes. If you try to scroll down in the browser window it's probably not happening.

I am able to reproduce the error whereby the print download will crash after being invoked more than once

When you say invoked more than once do you mean via multiple clicks to the button in quick succession or after opening the print dialog clicking again? The former is going to be tricky to protect against, unless we temporarily disable the print button for a certain time period after clicking.

When you say invoked more than once do you mean via multiple clicks to the button in quick succession or after opening the print dialog clicking again? The former is going to be tricky to protect against, unless we temporarily disable the print button for a certain time period after clicking.

I meant the latter – i.e., after the print dialog is opened, I go back to the article, then press the print button on the same article again, or go to a different article and press the print button on that. It seems to freeze when I go back to the article from the print dialog (using the device back button) when the PDF is still being generated in that Print dialog view. Here's another video: https://youtu.be/b0TuFTWb6z0

Weird. Given the spinner freezes it looks like the printer service is
hanging :(

phuedx added a subscriber: ABorbaWMF.

Almost done! @ABorbaWMF - for posterity, could you give us a list of the devices and chrome versions you tested on?

After discussing for a bit with @Tbayer, perhaps we can split this off into a separate task and test on every version of Android. @ABorbaWMF - do we have access to the past, say, 5 years of android versions for testing?

Device List
Nexus 5 - Chrome 60
Nexus 6 - Chrome 61
Nexus 9 -Chrome 59
Galaxy s7 - Chrome 61
Galaxy s5 - Chrome 60
Nexus 4 - Chrome 62 (real device)
Pixel - Chrome 62 (real device)

@ovasileva I don't have a direct way of getting that info. We could look at past testcase results, but I don't think they go that far back.

@ABorbaWMF - thank you! I think we have a pretty decent spread of versions here:

Nexus 4 (2012) - Android 4.2 - 5.1
Nexus 5 (2013) - Android 4.4 - 6.0
Galaxy s5 - Android 4.4 - 5.1
Nexus 6, 9 (2014) - Android 5.0 - 7.1
Pixel (2016) - Android 7.1 - 8.0

Seems like it works. Thanks all!