Page MenuHomePhabricator

Build download button for mobile PDF download
Closed, ResolvedPublic3 Estimated Story Points

Description

Background

This is the first task for the mobile PDF functionality with scope of building and instrumenting the download button

Design/Flow

  • Add new article action - SVG below
  • Tapping on the action triggers mobile browser print option
  • User returns to article using Native OS controls

Asset

and Zeplin https://zpl.io/2vMpr4r

Acceptance criteria

  • Add button to article action bar on mobile
  • Button must appear on all article namespace pages except the main page
  • When user selects button, Print menu should be triggered
  • This article action shows only for UA Android
  • The button does not show if you have JavaScript disabled or you are using a grade C browser e.g Opera Mini

Printing on iOS

Note we will exclude this button from iOS as the native browser print there does not provide pdf functionality


Although iOS 11 has print to pdf functionality we are not aware of any way to invoke it programmatically via JS and it does not apply our own print styles (see https://phabricator.wikimedia.org/T177215#3700576)


Archived from previous way to solve this problem

Questions

  1. Since we don't have a backend, what will clicking the button trigger?

@phuedx: It doesn't really matter.

  1. Should this be feature flagged? Yes.

@phuedx: In the short term, we could develop this feature in a feature branch and deploy it to the staging server.

There should be a single feature flag that defaults to false.

Testing criteria

The new feature has been enabled on reading web staging.
Visit http://reading-web-staging.wmflabs.org/w/index.php?title=Dave_Gallaher&mobileaction=toggle_view_mobile
A print button should show in all browsers except iOS with the exception of Grade C browsers (e.g. Opera Mini and UTC browser).
Clicking it should print.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
phuedx updated the task description. (Show Details)Oct 2 2017, 3:00 PM
phuedx updated the task description. (Show Details)Oct 2 2017, 3:30 PM

I think this is a case of cart before the horse. We don't have a service yet and I'm not even sure it will reliably scale to this sort of traffic.... am I alone in this thinking?

phuedx added a comment.Oct 3 2017, 9:35 AM

Not having a service isn't a reason not to do this – this is what interfaces are for, after all.

Whether we can build a service that will reliably scale to this sort of traffic is up for debate, as you say. I'd also say that it's not clear whether we need to expose this button to everyone, per the results of T176467.

Jdlrobson set the point value for this task to 5.Oct 3 2017, 4:55 PM

Questions from estimation:

  • Why doesn't this depend on getting the service built?
  • We can build this but it could be premature - how the service is built will impact how this work is achieved and what we instrument.

Piotr is a little more optimistic about this task - he voted 3.

To supplement @Jdlrobson's notes, I believe he also specifically asked for a schema to be defined but it kind of depends on the capability of the service.

phuedx added a comment.Oct 4 2017, 8:26 AM

Piotr is a little more optimistic about this task - he voted 3.

FTR I voted 2. You can call me Optimist Prime.

I just want to point out that this isn't necessarily comparable to the desktop numbers. In particular:

Question from my side - I think @phuedx suggested that we could track the instrumentation directly in Grafana. I think this is sufficient, as our questions are mainly around daily/monthly usage. Do we think it's necessary to build a separate eventlogging schema?

Questions from estimation:

  • Why doesn't this depend on getting the service built?

Because this task is about creating and instrumenting a button that sends an HTTP request somewhere. It's not about the rest of the workflow, nor is it about designing the HTTP interface for the service (which is on my TOTASK list).

  • We can build this but it could be premature - how the service is built will impact how this work is achieved and what we instrument.

Per the description, the instrumentation should answer "How many times is this button clicked per day/month?". This can be achieved by creating a counter in StatsD and plotting it in a Grafana graph. With that last bit in mind, I'm happy to bump my estimate to a 3.

We might want to mark this as blocked on T177379: [Spike] Design interface for the new renderer service. Thoughts?

I just want to point out that this isn't necessarily comparable to the desktop numbers. In particular:

@ovasileva: The concern is that any traffic from this button is additional traffic to a service that's high risk/falling over.

I think this is a discussion for the parent task though.

@phuedx: In the short term, we could develop this feature in a feature branch and deploy it to the staging server.

Any advantages of developing in a feature branch as opposed to doing it on top of master and feature flagging it?

phuedx added a comment.Oct 4 2017, 1:52 PM

@bmansurov: Clean merge when we're happy, maybe. We should do whatever we feel most comfortable doing and feature branching is an option.

Hey there - I'd also be interested in knowing (if possible) if we're seeing
frequency of use, as in: is this a few people doing this a lot or lots of
people doing this a little? (or neither :)

Jdlrobson added a comment.EditedOct 4 2017, 4:51 PM

Could the button not be wired to do a JavaScript print as part of this? That seems like a super cheap experiment and is not blocked on the PDF generation service. I hate the idea of having a feature branch that sits there until we're ready and all the merge conflicts that come with it or a disabled feature in master that never gets enabled.

Nirzar updated the task description. (Show Details)Oct 6 2017, 12:18 AM

@ovasileva @Jdlrobson @phuedx @atgo

I updated the task description and flows to remove dependency on the backend service.
the new flow is simpler to implement. I have also updated the AC but PM should take another look at it to refine.

This needs re-estimation.

Nirzar updated the task description. (Show Details)Oct 6 2017, 12:20 AM
This comment was removed by ovasileva.
ovasileva updated the task description. (Show Details)Oct 6 2017, 12:14 PM

@Nirzar great - this looks much more actionable and adds user value.

Open question - what is default value for feature flagging. Should it be turned on in beta mode?

We might need browser-specific behavior here as well. Firefox has a separate "save as PDF" option:
Page -> Save as PDF

We might want to trigger this instead. Expecting more detail soon via T177829: [Spike] Determine browser default for saving print styles

We might want to trigger this instead.

AFAIK there's no JavaScript API for us to do this.

Jdlrobson updated the task description. (Show Details)Oct 10 2017, 4:19 PM
phuedx updated the task description. (Show Details)Oct 10 2017, 4:22 PM
ovasileva updated the task description. (Show Details)Oct 10 2017, 4:32 PM
Jdlrobson updated the task description. (Show Details)Oct 10 2017, 4:33 PM
Jdlrobson updated the task description. (Show Details)
ovasileva changed the point value for this task from 5 to 3.Oct 10 2017, 4:36 PM

@ovasileva suggested I contribute some screenshots from my phone, just in case there is an assumption that on Android the default print function always leads directly to a PDF. This is for Firefox on an S7:
Three dot button --> "Print" leads to the "Select a printer" system dialog:

Tapping "Select a printer" leads to a choice between "Save as PDF" and "Add printer"; selecting the former allows one to download a PDF:

Similar for Chrome, with "Share..." --> "Print" leading to the same "select a printer" system dialog. (But once one has selected the PDF option there, it is offered directly next time, as in F10127478 .)

@Nirzar - I'm guessing the example @Tbayer noted would probably happen fairly regularly. Would it make sense to include a modal (as a separate task) that says something like "please select save as PDF as your printer choice" or something of the sort?

based on the comment by @Tbayer in T177856#3676481, it seems no changes to sampling rate are necessary - I'll update the task description accordingly.

Change 385095 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Add feature flagged DownloadIcon

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

I've intentionally left out the Android sniffing for the time being as I'm not sure that's correct. User agent sniffing generally should be avoided and is likely to lead to a very complicated regex so I want to challenge that.

Wouldn't we want to enable this for Chrome on iOS for example ?
Note our frontend code browser singleton doesn't handle targeting Android so I want to be sure before proceeding to build out that helper method.

User agent sniffing generally should be avoided and is likely to lead to a very complicated regex so I want to challenge that.

A quick Google yields that [[ https://davidwalsh.name/detect-android | navgiator.userAgent.indexOf( 'Android' ) !== -1 matches the majority of Android devices ]] and adding navigator.userAgent.indexOf( 'Kindle' ) !== -1 should catch older Kindle devices. I'd note that:

Wouldn't we want to enable this for Chrome on iOS for example?

I remember the This article action shows only for UA Android AC being introduced because we were concerned that this would increase the load on the already-overloaded Electron-based render service. So, yeah, let's discuss whether the AC is still relevant, but let's make sure that we (as Software Engineers) are representing its technical requirements well so that we can make the best decision possible.

Wouldn't we want to enable this for Chrome on iOS for example?

I remember the This article action shows only for UA Android AC being introduced because we were concerned that this would increase the load on the already-overloaded Electron-based render service. So, yeah, let's discuss whether the AC is still relevant, but let's make sure that we (as Software Engineers) are representing its technical requirements well so that we can make the best decision possible.

There was a workflow reason for this as well. Triggering PDFs on Chrome in iOS is more difficult than in Android. I believe @ABorbaWMF has a more detail. For now, I think we can focus on Android only.

From https://developer.mozilla.org/en-US/docs/Web/HTTP/Browser_detection_using_the_user_agent "it's very rarely a good idea to use user agent sniffing".

Implementation Technically this is trivial to do, once you've decided on the user agents, but bare in mind new user agents often mean updates are needed (note how IE identifies itself as "Mozilla/5.0" in the useragent for historical reasons [1]) so we'd have to keep on top of those. We can thus expect bugs such as "print button doesn't show on X browser. Deciding on the appropriate user agent will need further discussion however. For instance Firefox is available on Android and it sounds like that is not wanted. Chrome runs on various browsers so we'd need to take care there - do we want to display on desktop? This conversation takes time, is it worth it?

Technical debt: Limiting a feature to a certain browser, however adds a chunk of technical debt. It fragments the experience meaning different browsers behave differently so now testing needs to be aware of this and take this different behaviour into account. Our main usage of browser detection so far as caused major problems - the fixed header for iOS has had many bugs relating to this special treatment which is still open in T129360. Browsers change all the time. While we may now consider the iOS now bad, in a years time maybe it isn't. User agents demand constant attention with every browser update to ensure we don't penalise specific browsers.

user expectations It causes confusions to users. A user switching from Android to iOS is likely to be confused to why their user interface is different and possibly report a bug. A user trying to explain to his parent how to print Wikipedia articles needs to be wary of the platform availability.

Neutrality my main concern here is we are purposely favouring a certain platform - Android. This concerns me, especially if that definition ends up including Firefox. Wikimedia/Wikipedia is supposed to be a neutral platform and blacklisting certain browsers because we feel the experience is bad goes against that. It also assumes that users on a certain platform are stupid (e.g. cannot work out how to print). Do we really want to do this?

Happy to talk about this next week if it's of interest.

[1] https://msdn.microsoft.com/en-us/library/ms537503(v=vs.85).aspx

I can't speak to technical debt but I can certainly speak to ux part of this.

user expectations It causes confusions to users. A user switching from Android to iOS is likely to be confused to why their user interface is different and possibly report a bug. A user trying to explain to his parent how to print Wikipedia articles needs to be wary of the platform availability.

This feature is supposed to be an experiment. we don't have service, we can't control os ui. there are a lot of drawbacks to this.
cross platform consistency is last of our problems. we don't even aim for consistency on our native apps :|

Neutrality my main concern here is we are purposely favouring a certain platform - Android. This concerns me, especially if that definition ends up including Firefox. Wikimedia/Wikipedia is supposed to be a neutral platform and blacklisting certain browsers because we feel the experience is bad goes against that. It also assumes that users on a certain platform are stupid (e.g. cannot work out how to print). Do we really want to do this?

we are using an OS feature. we have to restrict to _that_ OS. It's not about iOS users, it's about iOS as a platform.
I'm wholeheartedly against adding a download action that triggers a useless print dialog on iOS.

Ultimately, this is @ovasileva 's call, but want to offer some feedback.

user expectations It causes confusions to users. A user switching from Android to iOS is likely to be confused to why their user interface is different and possibly report a bug. A user trying to explain to his parent how to print Wikipedia articles needs to be wary of the platform availability.

This feels like a pretty extreme edge case that can be addressed when/if it comes up. As @Nirzar said, this is an experiment to continue to learn and give user value, Users across platforms are already having different experiences. We should not provide what we know to be a broken experience to iOS users and we should not limit our learning and development of user motivations/needs when we have a solution that moves us forward.

This feature is supposed to be an experiment

Sure, but experiments in production have a habit of not becoming experiments and sticking around. We are not creating a prototype here - we are introducing code to our main website. Let's make sure it's good code, even if we are going to remove it in a month. We also don't do enough talking upfront in my opinion, so it's good to get to the crux of the issue rather than change our mind and implementation down the road.

I'm wholeheartedly against adding a download action that triggers a useless print dialog on iOS.

This context was missing from the description. I've had to explore this myself and I now see that it seems to send the page to a wireless printer that usually doesn't exist. So yes, it makes sense you would not want to show it here.This would have been useful to know upfront when working on this task.

iOS11 doesn't seem to have a bad experience though (it even provides pdf generation): https://www.igeeksblog.com/how-to-save-webpages-as-pdf-on-iphone-ipad/ - have we considered this? Why are we restricting there too?

It sounds like really what we're doing here is blacklisting iOS (possibly all, possibly pre iOS 11). This is very different to limiting the feature to Android (even if it doesn't seem this way). We even have code to limit features from iOS versions!

This context was missing from the description. I've had to explore this myself and I now see that it seems to send the page to a wireless printer that usually doesn't exist. So yes, it makes sense you would not want to show it here.This would have been useful to know upfront when working on this task.

not sure why the tasks are split now but this discussion happened on the parent epic >
https://phabricator.wikimedia.org/T163472#3590003

iOS11 doesn't seem to have a bad experience though (it even provides pdf generation): https://www.igeeksblog.com/how-to-save-webpages-as-pdf-on-iphone-ipad/ - have we considered this? Why are we restricting there too?

It's only available on safari so far. AFAIK there is no web api to trigger that dialog.
even if we are able to trigger this special "create pdf" ios 11 dialog with our own button, the pdf generation doesn't use print styles.

here's a pdf generated from wikipedia with iOS 11 pdf creator


^^ missing content from sections completely

:( I wish iOS had better support for this

Jdlrobson updated the task description. (Show Details)Oct 20 2017, 7:49 PM

Change 385433 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Disable print button on iOS

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

not sure why the tasks are split now but this discussion happened on the parent epic > https://phabricator.wikimedia.org/T163472#3590003

Thanks for this and the new context!
\_(ツ)_/¯ yeh no idea why we didn't just reuse that task and fragment that conversation.

https://gerrit.wikimedia.org/r/385433 disables the button on iOS only. Is that acceptable? If we want to exclude other platforms/browsers I'd rather do that when we encounter problems rather than guessing. Sound good?

Jdlrobson updated the task description. (Show Details)Oct 20 2017, 7:56 PM

The epic covers/covered the flow that was proposed initially. This task covers/covered building the download button. There's been a whole bunch of changes to this task and the epic hasn't been updated given those conversations. Maybe we can take the time to tidy up the epic as part of signing off on this task?

Change 385095 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Add feature flagged DownloadIcon

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

User agent sniffing generally should be avoided and is likely to lead to a very complicated regex so I want to challenge that.

A quick Google yields that [[ https://davidwalsh.name/detect-android | navgiator.userAgent.indexOf( 'Android' ) !== -1 matches the majority of Android devices ]] and adding navigator.userAgent.indexOf( 'Kindle' ) !== -1 should catch older Kindle devices. I'd note that:

I guess the state of the art here may be ua-parser, which indeed seems to rely on the presence of the string "Android" (with this exception to exclude mobile IE on Windows Phone, and two other tests to catch Kindle and UCWEB on Android).

Change 385433 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Disable print button on iOS

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

Jdlrobson removed a project: Patch-For-Review.
Jdlrobson updated the task description. (Show Details)

@Jdlrobson, I just wanted to make sure I am testing in the right place. Right now on reading-web-staging.wmflabs.org I am seeing the print icon, but it only 'works' on Chrome. I tried a few so far: Firefox, Dolphin, and Maxthon. Tapping the icon does nothing.

@ABorbaWMF looks like you may have found some bugs. I've opened T179529 - can you update the list (be sure to include device and browser!)
I can replicate the issue on Dolphin on Android for instance...

T179529 is great. Thanks @ABorbaWMF for surfacing those bugs!
Assuming that captures all the bugs we have found in the feature we'll pick that card up in grooming separately since this is new information that needs analysis we didn't know when picking up the card.

ovasileva closed this task as Resolved.Nov 6 2017, 2:32 PM

Resolving based on QA so far. Will be creating a card for deployment to the browsers not affected by T179529: [Spike] Can we detect browsers where the window.print function simply doesn't work?