1025853 - Long press in a link to a media file does not give the option to download it

Status: RESOLVED FIXED

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

Description

[Marcelino Veiga Tuimil [:sonmarce]]

Steps to reproduce:
1. Open browser app
2. Go to http://owd.tid.es/dm2
3. Long press in link "Crazy_Horse.jpg"
4. Press "Save image" button

Expected behavior: Download starts via download manager

Actual behavior: There is no "Save image" button (just "Open link in new tab" button)

Comment 1

[Marcelino Veiga Tuimil [:sonmarce]]

Tested in today master build for hamachi

Comment 2

[Marcelino Veiga Tuimil [:sonmarce]]

This is breaking a 2.0 feature, so asking to be a blocker

Comment 3

[Marcelino Veiga Tuimil [:sonmarce]]

I forgot to mention that I also tested in 2.0 branch and I got same result

Comment 4

[Dietrich Ayala (:dietrich)]

Are we sure this is a regression?

I don't see any code to support detection of <a> tags that have a URL that is inferred to be image/audio/video without the 'download' attribute.

I also cannot find a 2.0 requirement for this linked off any of the related bugs. Marce, which 2.0 feature is this breaking? Can you link the bug where this functionality was developed?

Comment 5

[Dietrich Ayala (:dietrich)]

Ni? Aus, looks like he developed the initial @download support.

Comment 6

[Jason Smith [:jsmith]]

I'll mark qawanted to check 1.4.

Comment 7

[Dietrich Ayala (:dietrich)]

Developer note: This code has been partially duplicated in both system and browser apps, but then the code for media downloads didn't make it over to the system app.

Comment 8

[Aus Lacroix [:aus]]

Only long press on media items works at this time. This is how it used to work before we started using the new download method on the Browser API.

You can open in a new tab when long pressing on an anchor tag though. That should still work.

Adding support for long press on anchor tag is something that should get done sometime in the 2.1 timeframe when we switch to the system browser.

Comment 9

[Aus Lacroix [:aus]]

Here's the bug that is tracking this + media downloads for the system browser.

https://bugzilla.mozilla.org/show_bug.cgi?id=929532

Feel free to dupe this bug against it if that's how you roll ;)

Comment 10

[Aus Lacroix [:aus]]

I'm just going to dupe it... feels a little pointless to leave this one open.

Comment 11

[Marcelino Veiga Tuimil [:sonmarce]]

My understanding is that it is a 2.0 feature, as you can see in bug 876376. You can find description in User Story field, indicating the action of long press to save. It has two dependencies, bug 982295 & bug 983747, both landed in 2.0, and one of them including visual design for implementing it (see attachment 8433661 [details]).

Peter, please, can you clarify...

Comment 12

[Peter Dolanjski [:pdol]]

Aus, shouldn't the use case described by this bug be covered by bug 876376?

Comment 13

[Aus Lacroix [:aus]]

Indeed. It's actually my fault here. I didn't realize that the user story in 876376 included long press on anchor tags. While this particular bug here isn't a regression. We can keep it in it's reopened state to track adding support for anchor tags.

Does that seem reasonable to everyone?

I don't know that a patch for this would be necessarily accepted for 2.0 but it can certainly be done and ready within that time-frame.

Comment 14

[Marcelino Veiga Tuimil [:sonmarce]]

My opinion is it should be included in 2.0, to have all use cases of download manager. And I suppose it would not be a risky patch, right? It would be reusing gecko part for downloading media and the only UI change is just adding a button to save media.

Comment 15

[Aus Lacroix [:aus]]

(In reply to Marcelino Veiga Tuimil [:sonmarce] from comment #14)
> My opinion is it should be included in 2.0, to have all use cases of
> download manager. And I suppose it would not be a risky patch, right? It
> would be reusing gecko part for downloading media and the only UI change is
> just adding a button to save media.

Yes, I agree that it's not a very risky patch but, that decision is always up to release drivers. :)

Comment 16

[Aus Lacroix [:aus]]

Unfortunately, this patch will introduce a new string. So it will need to land ASAP.

# Generic save link context menu
save-link=Save Link

Comment 17

[Aus Lacroix [:aus]]

Attachment: Pull Request - Add save link context menu option for anchor tags.

Comment 18

[Aus Lacroix [:aus]]

This bug blocks a bug with 2.0+, it should have automatic approval and we're within the cut-off for late-l10n.

Comment 19

[Peter Dolanjski [:pdol]]

Thanks Aus!

Comment 20

[Ben Francis [:benfrancis]]

Comment on attachment 8443375 [details] [review]
Pull Request - Add save link context menu option for anchor tags.

This looks great, but I have a couple of questions before granting r+.

1. I assume this is supposed to work for links to all types of file, not just media files? If I long press on a link to a web page it tries to start to download the HTML file but it seems to get stuck at 0 bytes.
2. Where is the UX spec for this feature? The "visual design" Marce linked to is actually a screenshot from a bug report and is nothing to do with this feature. Is "Save Link" the correct string? I would have thought "Save link target" might be less confusing than "Save Link" which sounds like you're saving the link itself (what does that mean?). Also, the capitalisation is not consistent with "Open link in new tab". Happy to defer to UX on this but I can't find the spec for it, and Firefox for Android doesn't appear to have an equivalent feature to reference.

Also, feature-b2g:2.0 != blocking-b2g:2.0. This will need approval to be uplifted to 2.0 past the feature landing deadline.

Comment 21

[Aus Lacroix [:aus]]

Hi Ben!

I think the issue lies with the underlying Downloads API. It's unfortunately broken. I committed a fix to b2g-inbound this Friday and it's been merged to central (see bug 983747). If you grab a fresh gecko (central, not aurora) it should work. Let me know how it goes. :)

As for the string, I'll check in with Francis on Monday.

It's true, this will require approval for 2.0 uplift. It's definitely more of a nice to have at this point but feels pretty safe.

Comment 22

[Théo Chevalier]

The string freeze for 2.0 is today. From now on, the l10n team will need to evaluate the risk before uplifting this on 2.0.

Comment 23

[Ben Francis [:benfrancis]]

Comment on attachment 8443375 [details] [review]
Pull Request - Add save link context menu option for anchor tags.

r+ with dependencies landed and string confirmed

Comment 24

[Francesco Lodolo [:flod]]

Removing late-l10n keyword since we're past string freeze and bug has no approval flags for 2.0

Comment 25

[Aus Lacroix [:aus]]

(In reply to Ben Francis [:benfrancis] from comment #20)
> 2. Where is the UX spec for this feature? The "visual design" Marce linked
> to is actually a screenshot from a bug report and is nothing to do with this
> feature. Is "Save Link" the correct string? I would have thought "Save link
> target" might be less confusing than "Save Link" which sounds like you're
> saving the link itself (what does that mean?). Also, the capitalisation is
> not consistent with "Open link in new tab". Happy to defer to UX on this but
> I can't find the spec for it, and Firefox for Android doesn't appear to have
> an equivalent feature to reference.

Francis, could you point us to the spec or pick the magic string? I've checked on Android and Desktop and the functionality there is just different. I do like "Save link target" though but maybe that's a little too technical for users?

Comment 26

[Dietrich Ayala (:dietrich)]

(In reply to Ghislain Aus Lacroix [:aus] from comment #25)
> Francis, could you point us to the spec or pick the magic string? I've
> checked on Android and Desktop and the functionality there is just
> different. I do like "Save link target" though but maybe that's a little too
> technical for users?

On desktop, this causes confusion. For "target" to have meaning, a user must have an understanding of how linking on the Web works, which is rarely the case. We can probably do better.

Even "Download" by itself would be less confusing.

Comment 27

[Aus Lacroix [:aus]]

(In reply to Dietrich Ayala (:dietrich) from comment #26)
> (In reply to Ghislain Aus Lacroix [:aus] from comment #25)
> > Francis, could you point us to the spec or pick the magic string? I've
> > checked on Android and Desktop and the functionality there is just
> > different. I do like "Save link target" though but maybe that's a little too
> > technical for users?
> 
> On desktop, this causes confusion. For "target" to have meaning, a user must
> have an understanding of how linking on the Web works, which is rarely the
> case. We can probably do better.
> 
> Even "Download" by itself would be less confusing.

Definitely. It was just pleasing my engineer brain with it's precision. 'Download' is super descriptive and it does route the action directly to the Download Manager which feels consistent.

I've updated the pull request with 'Download' in the hopes that Francis likes it as well. :)

Comment 28

[bhavana bajaj [:bajaj]]

This has string changes and cannot land at this point in 2.0 unless there are other alternative without string change.

Comment 29

[Peter Dolanjski [:pdol]]

l10n team: this string is obviously beyond the freeze date.  Is there a way to get this accepted for 2.0?  From your perspective, is the risk low?  Are there alternate solutions?

Comment 30

[Delphine Lebédel [:delphine - Use needinfo]]

If this is not a blocking feature for 2.0 we can't break the string freeze again.
We're wrapping up today all the bugs that were already breaking string freeze.

Comment 31

[Axel Hecht [:Pike]]

(setting the dependency on the gecko bug 983747)

IMHO bhavana said it all, it's her schedule.

Comment 32

[Marcelino Veiga Tuimil [:sonmarce]]

Is it possible to reuse an existing string?

Comment 33

[Aus Lacroix [:aus]]

(In reply to Marcelino Veiga Tuimil [:sonmarce] from comment #32)
> Is it possible to reuse an existing string?

Unfortunately, no. The browser doesn't have any string that would describe the action properly.

Comment 34

[Aus Lacroix [:aus]]

Comment on attachment 8443375 [details] [review]
Pull Request - Add save link context menu option for anchor tags.

Since this won't land in 2.0, I took a stab and making it even better for the user. We can land this into the System Browser as well.

Comment 35

[Francis Djabri [:djabber]]

Sorry for the delay, I've been in no-laptop workshops during this week. I outlined some UI for saving content in the "Saving content" section of the Rocket bar spec on Box here: https://mozilla.box.com/s/2tix674298wtc4e4hewh

Let me know if you have any comments.

Comment 36

[Ben Francis [:benfrancis]]

Hi Francis, I already looked at this spec but I don't think it answers the question. This feature is long pressing on hyperlinks to download any type of file (and remember we can't accurately tell what the file type is until we've downloaded it).

Comment 37

[Ben Francis [:benfrancis]]

Francis, note that Aus's new patch above makes the menu item say "Download <filename>" where <filename> is defined in the download attribute of the hyperlink or derived from the URL.

Comment 38

[Ben Francis [:benfrancis]]

Comment on attachment 8443375 [details] [review]
Pull Request - Add save link context menu option for anchor tags.

Clearing the review flag until we have a UX spec

Comment 39

[Francis Djabri [:djabber]]

Attachment: Screen Shot 2014-06-27 at 11.46.23 AM.png

Ben, got it. Reading through Bug 876376, I see that the interactions for long pressing on an Audio link and Video link on page 55 of the spec are in fact not possible, unless we do some guessing about the file type based on the extension. 

About the string in question, I would use "Save" instead of "Download" to keep the terminology consistent. I've used "Save" throughout in preference to "Download" since we automatically save downloaded files. 

I'd also prefer to use a generic term rather than use the filename in the string, as file names can be long and we might not be able to display them fully. 

So I'll specify the string as "Save linked file" and the full context menu for a hyperlink should be:

"Open in new window"
"Add to home screen"
"Save linked file"
"Share link"

I'll update and re-issue the spec.

Comment 40

[Aus Lacroix [:aus]]

Comment on attachment 8443375 [details] [review]
Pull Request - Add save link context menu option for anchor tags.

Both implementations according to the new spec from Francis (Browser App, since it's still in use for now and System Browser).

Comment 41

[Ben Francis [:benfrancis]]

Comment on attachment 8443375 [details] [review]
Pull Request - Add save link context menu option for anchor tags.

Sorry for the delay, r+ from me.

Travis could use some love.

Comment 42

[Aus Lacroix [:aus]]

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

Fixed! 

Tests that are failing have bugs associated with them to get fixed (or disabled), they have the same behavior on gaia-try.