982295 - [Gaia][Browser] Long Press Downloads for Media should route downloads to Downloads API

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect)

Description

[Aus Lacroix [:aus]]

Currently the Browser manually manages downloading Media assets (images, audio and video). It should route these to the Downloads API using an anchor tag with download and type attributes set accordingly.

Example:

<a href="http://hostname/path/to/media.mp3" download="media.mp3" type="application/octet-stream"></a>

This element can then be "clicked" or "pressed" to trigger a download that will be completely managed by the Downloads API (and Gaia's Download Manager).

Comment 1

[Jason Smith [:jsmith]]

I think this is a good idea, but I don't think we would hold a release on this, mainly because it's cleanup work for consistency with the new download manager. Feel free to move forward implementing this though.

Comment 2

[Ben Francis [:benfrancis]]

My work in progress patch seems to show that an <a href='...' download='...' type='application/octet-stream'> added to the browser app does not trigger a download when clicked.

The patch adds an anchor tag to the DOM programmatically, but if I just add an anchor tag to index.html of the browser app manually I see the same result. Clicking on the link just navigates the app window to the URL rather than triggering a download.

Is this a bug, or am I missing something?

Comment 3

[Aus Lacroix [:aus]]

Ben, going to try and look at this tomorrow... sorry about the wait. Leaving NI? flag untouched as a reminder :)

Comment 4

[[:fabrice] Fabrice Desré]

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a states that "In Firefox 20 this attribute is only honored for links to resources with the same-origin". Not sure if this should be read as "Starting in Firefox 20..." which would explain why it's not working there.

Comment 5

[Ben Francis [:benfrancis]]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a states that "In
> Firefox 20 this attribute is only honored for links to resources with the
> same-origin". Not sure if this should be read as "Starting in Firefox 20..."
> which would explain why it's not working there.

Yep, that's it.

I read this, but as Firefox 20 was the first version to support the download attribute I concluded it meant that Firefox 20 didn't support cross-origin downloads but future versions do.

I just created a test case which shows this isn't the case http://people.mozilla.org/~bfrancis/download.html

A cross-origin <a download> link will never trigger a download in Gecko.

That means if we want users to be able to manually trigger a download of a file from an image/audio/video element or hyperlink we will either need to lift this restriction or create a new method in the download API for that. Otherwise I guess we have to add the download API permission to the browser app.

Comment 6

[Ben Francis [:benfrancis]]

(In reply to Ben Francis [:benfrancis] from comment #5)
> Otherwise I guess we have to add the download API permission to the browser
> app.

Actually I'm not sure that would even help, is there even a privileged API call that can manually trigger a download from an app?

Comment 7

[Aus Lacroix [:aus]]

(In reply to Ben Francis [:benfrancis] from comment #6)
> (In reply to Ben Francis [:benfrancis] from comment #5)
> > Otherwise I guess we have to add the download API permission to the browser
> > app.
> 
> Actually I'm not sure that would even help, is there even a privileged API
> call that can manually trigger a download from an app?

Damn, that sucks. And yep, we'd have to add a method to trigger a download from a URI. That doesn't exist right now. It shouldn't be too difficult to do this and it does sound like it would be awfully useful for applications to have a way to do this. I'm going to go ahead and file a bug to add this method to the Downloads API and add it as a dependent of this bug.

Comment 8

[Ben Francis [:benfrancis]]

Unassigning myself for now, until we figure out platform support for this and the path forward.

Comment 9

[Aus Lacroix [:aus]]

Attachment: Pull Request - Use new 'download' method on Downloads API for Image, Audio and Video downloads via long press

Comment 10

[Aus Lacroix [:aus]]

Attachment: Pull Request - Use new 'download' method on Browser API.

Comment 11

[Ben Francis [:benfrancis]]

Comment on attachment 8423494 [details] [review]
Pull Request - Use new 'download' method on Browser API.

This is awesome.

Comment 12

[Aus Lacroix [:aus]]

(In reply to Ben Francis [:benfrancis] from comment #11)
> Comment on attachment 8423494 [details] [review]
> Pull Request - Use new 'download' method on Browser API.
> 
> This is awesome.

I love replacing a big pile of code with one line. It's so good. So good! :D

Comment 14

[jachen]

Attachment: bug_982295_visualstyleissue.png

Rob is PTO today so I'm posting this on his behalf. He noted that this prompt doesn't have the right style. I'll post him as needs info to follow up.

Comment 15

[Rob MacDonald [:robmac]]

Attachment: Value Selector Example (Email)

Example of value selector from 2.0 (from Email).

Comment 16

[Rob MacDonald [:robmac]]

> Created attachment 8434367 [details]
> Value Selector Example (Email)
> 
> Example of value selector from 2.0 (from Email).

The attached example shows the intended format of the value selector in 2.0. Also flagging Eric in case he has anything to add.

Comment 17

[Eric Pang [:epang] UX]

Nothing to add, this looks good to me :). Thanks guys!

Comment 18

[bhavana bajaj [:bajaj]]

NI aus, to get this landed before the merge tomorrow, as this looks ready..

Comment 19

[Aus Lacroix [:aus]]

(In reply to bhavana bajaj [:bajaj] from comment #18)
> NI aus, to get this landed before the merge tomorrow, as this looks ready..

There are dependencies which are clearly listed that have not landed. Please read the whole bug before flagging for needinfo.

(In reply to Rob MacDonald [:robmac] from comment #16)
> > Created attachment 8434367 [details]
> > Value Selector Example (Email)
> > 
> > Example of value selector from 2.0 (from Email).
> 
> The attached example shows the intended format of the value selector in 2.0.
> Also flagging Eric in case he has anything to add.

Hi guys, this bug is purely to update which API the downloads use. If we want to change and update the visual we'll want to file a new bug.

Comment 20

[Aus Lacroix [:aus]]

Requesting approval for 2.0 landing now since this requires a Gecko patch that will take a day or two to show up in Aurora.

Comment 21

[bhavana bajaj [:bajaj]]

(In reply to Ghislain Aus Lacroix [:aus] from comment #20)
> Requesting approval for 2.0 landing now since this requires a Gecko patch
> that will take a day or two to show up in Aurora.

(In reply to Ghislain Aus Lacroix [:aus] from comment #19)
> (In reply to bhavana bajaj [:bajaj] from comment #18)
> > NI aus, to get this landed before the merge tomorrow, as this looks ready..
> 
> There are dependencies which are clearly listed that have not landed. Please
> read the whole bug before flagging for needinfo.

I did check the dependency, https://bugzilla.mozilla.org/show_bug.cgi?id=983747#c34 and it looked almost ready to land hence flagged you here.
> 
> (In reply to Rob MacDonald [:robmac] from comment #16)
> > > Created attachment 8434367 [details]
> > > Value Selector Example (Email)
> > > 
> > > Example of value selector from 2.0 (from Email).
> > 
> > The attached example shows the intended format of the value selector in 2.0.
> > Also flagging Eric in case he has anything to add.
> 
> Hi guys, this bug is purely to update which API the downloads use. If we
> want to change and update the visual we'll want to file a new bug.

I don't think we would block on this bug for a release, as comment #1 explains. If you are ready to uplift this and the dependency in the next few days, please seek gaia/gecko approvals. Based on the risk/reward of this patch, the approval will be acted upon.

Comment 22

[Aus Lacroix [:aus]]

> I don't think we would block on this bug for a release, as comment #1
> explains. If you are ready to uplift this and the dependency in the next few
> days, please seek gaia/gecko approvals. Based on the risk/reward of this
> patch, the approval will be acted upon.

I'll go ahead and ask for approval on this item once the patch for bug 983747 makes it into Aurora. Apologies, status in bug 983747 could have been clearer.

Comment 23

[Aus Lacroix [:aus]]

Comment on attachment 8423494 [details] [review]
Pull Request - Use new 'download' method on Browser API.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 876376
[User impact] if declined: Functionality as present is broken under many scenarios.
[Testing completed]: On device with custom gecko using patch from bug 983747 and gaia master + this pull request. 
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none

Comment 24

[Jason Smith [:jsmith]]

Why is this feature absolutely needed in 2.0?

Comment 25

[Aus Lacroix [:aus]]

Commit (master): https://github.com/mozilla-b2g/gaia/commit/901646a3279c66daa7621bebb62641779d8ae22c

Fixed on master. I'm leaving this open until it 983747 lands on aurora.

Comment 26

[bhavana bajaj [:bajaj]]

(In reply to Jason Smith [:jsmith] from comment #24)
> Why is this feature absolutely needed in 2.0?

This missed the 2.0 train due to a gecko uplift by a few hours and merge day activity and this exception was discussed with Release management (:bajaj) to get this uplifted keeping the risk/reward in mind.

Comment 27

[Aus Lacroix [:aus]]

Commit (v2.0): https://github.com/mozilla-b2g/gaia/commit/593b6b1765c76017fa575066f1139e5332595804

Fixed.