Page MenuHomePhabricator

Regression: Download icon doesn't download images
Closed, ResolvedPublic2 Story Points

Description

While testing a patch from @Niedzielski I noticed that clicking the download icon doesn't lazy load images before the print action.

QA steps

  • Make sure "expand all sections" is disabled in Special:MobileOptions
  • Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Barack_Obama when anonymous (log out)
  • Use mobile site on a mobile resolution (desktop mode seems to be working fine) with sections collapsed by default
  • Click the print button (depicted with a red outline below) in the toolbar and print to pdf

Expected: PDF has images
Actual: PDF does not have images

Developer notes

The JS is working perfectly. The issue is the CSS animation. It prints in the starting position which is fade in. Resetting the animation in print @media solves this problem.

QA Results

ACStatusDetails
1T220668#5359958

Details

Related Gerrit Patches:
mediawiki/skins/MinervaNeue : masterMake sure lazy loaded images display in printed PDFs

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 11 2019, 12:14 AM
Jdlrobson renamed this task from Download icon doesn't download images to Regression: Download icon doesn't download images.Apr 11 2019, 12:15 AM
Jdlrobson added projects: Regression, MinervaNeue.
Jdlrobson updated the task description. (Show Details)

I tried to repro this on https://en.m.wikipedia.org/wiki/Barack_Obama but it seems to be working.

I tried a number of other pages including local development articles using the content proxy but they seem to be working too. I'm thinking that the most likely issue is either that you're hitting the parsimonious timeout of 3 seconds or an image is failing to download (errors are silently eaten). You may be able to verify the latter by examining the network requests issues after pressing the download PDF button.

Thanks for the sanity check stephen! I will take another look tomorrow. Hopefully it's the timeout like you say (which seems quite low on retrospect for an entire article!)

Not sure what the heck I'm doing wrong but https://en.m.wikipedia.beta.wmflabs.org/wiki/Qatar shows the Wikipedia:New user landing page when I'm logged in and the Qatar page when I'm logged out:

Regardless, I cannot repro on any of the pages you mentioned :| What are you seeing in the console?

Jdlrobson updated the task description. (Show Details)Apr 11 2019, 5:05 PM
Jdlrobson updated the task description. (Show Details)

I can repro on emulated Android with sections collapsed on enwiki prod.

Jdlrobson updated the task description. (Show Details)Apr 11 2019, 5:12 PM
ovasileva triaged this task as Normal priority.Apr 16 2019, 10:20 AM
Jdlrobson removed Niedzielski as the assignee of this task.Jul 9 2019, 5:51 PM
Jdlrobson updated the task description. (Show Details)

Change 521551 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Make sure lazy loaded images display in printed PDFs

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

ovasileva set the point value for this task to 2.Jul 16 2019, 5:13 PM

Change 521551 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Make sure lazy loaded images display in printed PDFs

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

Edtadros reassigned this task from Edtadros to nray.Jul 18 2019, 2:31 AM
Edtadros added subscribers: nray, Edtadros.

Test Result

Status: ❌ FAIL
OS: macOS Mojave
Browser: Chrome
Device: MBP
Emulated Device: iPhoneX

Test Artifact(s):

QA steps

  1. Make sure "expand all sections" is disabled in Special:MobileOptions
  2. Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Qatar
  3. Use mobile site on a mobile resolution with sections collapsed by default
  4. Click the print button and print to pdf

❌ AC1: Expected: PDF has images

@nray Just to be sure, there is no print button, I used the print button from the browser.

Edtadros updated the task description. (Show Details)Jul 18 2019, 2:33 AM
nray updated the task description. (Show Details)Jul 18 2019, 3:57 PM

@Edtadros sorry, I needed to update the QA instructions. You should be able to see the print button if you follow the instructions now

Edtadros reassigned this task from Edtadros to ovasileva.Jul 24 2019, 4:53 AM

Test Result

Status: ✅ PASS
OS: macOS Mojave
Browser: Chrome
Device: Google Pixel 3 XL

Test Artifact(s):

QA steps

QA steps

  • Make sure "expand all sections" is disabled in Special:MobileOptions
  • Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Barack_Obama when anonymous (log out)
  • Use mobile site on a mobile resolution (desktop mode seems to be working fine) with sections collapsed by default
  • Click the print button (depicted with a red outline below) in the toolbar and print to pdf

✅ AC1: Expected: PDF has images

@nray I executed this on BrowserStack so that I could use an android device. It took some acrobatics to get that file!

Edtadros updated the task description. (Show Details)Jul 24 2019, 4:54 AM
ovasileva closed this task as Resolved.Jul 24 2019, 3:48 PM

all done here, thanks all!