Add save file as to the html5 player context menu

VERIFIED FIXED in Firefox 21

Status

()

Firefox for Android
General
--
enhancement
VERIFIED FIXED
5 years ago
11 months ago

People

(Reporter: xti, Assigned: heyjinsu)

Tracking

(Blocks: 1 bug, {regression})

Trunk
Firefox 21
ARM
Android
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(firefox21 verified, relnote-firefox 21+, fennec-)

Details

(Whiteboard: [mentor=wesj][lang=js] [html5])

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

5 years ago
Firefox 18.0a1 (2012-09-17)
Device: Galaxy Note
OS: Android 4.0.4

Steps to reproduce:
1. Open Firefox for Android
2. Go to http://searchmp3.mobi/ and tap any Top Rated Tracks
3. Tap on Download button
4. Type the given captcha code and tap on Download button

Expected result:
After step 4, a File Picker is displayed with the following options:
* at least one 3rd party app, in this case, the default Media Player is listed
* Firefox for Android
If the first option is tapped, the selected app will start playing the song.
If the second option is selected, then Firefox will start downloading and saving the mp3 to sdcard/Downloads location.

Actual result:
Firefox will start playing the mp3 file and HTML 5 controls are displayed.

Note:
I assume that when a user wants to Download something, it means that he/she really wants to have that file on  the sdcard.

Updated

5 years ago
Severity: normal → enhancement
Summary: Add "Save target as" option when downloading a file → Add a "Save Link As" ability to download rather than play media

Comment 1

5 years ago
This should be a no brainer, there should be a configurable option to treat different media types differently when clicked on, and offer the other option on the menu.

Updated

5 years ago
tracking-firefox17: --- → ?
Wont be 17 as that's out the door.
tracking-fennec: --- → ?
tracking-firefox17: ? → ---
Flags: needinfo?(krudnitski)
Keywords: productwanted
I agree that by hitting the 'download' button, the file should download (and I would expect the corresponding Android 'download' notifications happening at the top of the screen). 

In parallel, if you have picked another media player to 'play' it and it plays as well, great. I would also expect Firefox for Android to do the same thing - be able to play it. 

It is unclear from this bug whether an installed third-party media player can be 'chosen' to play the downloaded song (in parallel to the download itself), at which point I would want Firefox to have similar behaviour.

However at the crux, if a user selects 'download', I would want the file downloaded (since that is the intent) with the option to play it within Fx with HTML5 controls.
Flags: needinfo?(krudnitski)
In comment 0, Cristian click a "Download" button which is part of the webpage, not Firefox. I don't know why the download did not start, but that is completely different than adding a context menu for "Save Link As".

I can assume that the download did not happen as expected because Firefox has a default action to "open" files that are downloaded, if we find an application that can handle the file.
I don't think we need a 'save link as' if the download actually downloaded (which is what I would expect to happen by clicking on that button).
I would expect the ability to hard tap on a link and select 'save link as' or 'save target as' on an indexed file directory like a music FTP rather than tapping each link and having the browser playing the music
Discussed this at length in the Mobile Triage meeting. The solution we are looking at is adding a context menu option to the html5 player to save the file.
Keywords: productwanted
Summary: Add a "Save Link As" ability to download rather than play media → Add save file as to the html5 player context menu
Whiteboard: [mentor=wesj][lang=js]
tracking-fennec: ? → -
Blocks: 818987
(In reply to Karen Rudnitski from comment #5)
> I don't think we need a 'save link as' if the download actually downloaded
> (which is what I would expect to happen by clicking on that button).

“Download buttons” on many sites are mere links without the download attribute and without the HTTP response having a Content-Disposition header. So from the point of view of Firefox, those are just links and don’t have any “download” semantics. Yet, as a user, I expect to be able to tap&hold for the context menu and do “Save As…” even when the site has done nothing to make the link have download semantics.
(In reply to Henri Sivonen (:hsivonen) from comment #8)
> “Download buttons” on many sites are mere links without the download
> attribute and without the HTTP response having a Content-Disposition header.
> So from the point of view of Firefox, those are just links and don’t have
> any “download” semantics. Yet, as a user, I expect to be able to tap&hold
> for the context menu and do “Save As…” even when the site has done nothing
> to make the link have download semantics.

Thanks Henri - we did talk about this at length in Thursday's triage meeting and the above was explained. We are discussing how best to implement better functionality in this kind of scenario.
A compromise here would be to replace "Save as PDF" in the dropdown menu with just "Save" for media files, where "Save as PDF" is not useful.
If anyone is interested in picking this up, wanted to write up some quick instructions on how you would go about fixing this. This bug is similar to bug 818987, so these are similar instructions.

First steps are to get a Fennec build up and running. There are pretty in depth instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android but don't hesitate to ask if you run into problems.

Context menu options for Fennec are added using our context menu api:

https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/NativeWindow/contextmenus

This is very similar to the "Save Image" context menu item we already have here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#457

In fact, I think we could likely hijack that context menu item for this (maybe we should do that for the shareImage element above this as well? That's a separate bug....).

Instead of a string, the contextmenu api also allows passing in a function for the title. The function is passed the element that we're looking at. So you could take that element, see if its a video/audio/image element (element instanceof HTMLVideoElement) and show an appropriate title "Save Image", "Save Audio", "Save Video".

We'll also have to modify the second argument passed here (imageSaveableContex) to also return true for video/audio elements. Its defined here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1435

It does some checks to make sure the image is not a 404 link. For now I'd be fine skipping that for video/audio (which we try to avoid loading unless the user has hit play to save mobile bandwidth).

The third argument is the function called when this context menu item is selected. Again, we can skip the attempt to pull the mimetype out of the cache for video/audio for now. We'll just have to get the src of the media and pass it straight to ContentAreaUtils.internalSave.

For reference, the desktop firefox code for doing this same stuff is at:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1089

but its significantly different than Fennec's. For now, I don't think we need all that complexity here. Someday it would be nice to move that desktop file to toolkit and try and share more of this code.
Xti, out of curiosity, is this site setting Content-Disposition correctly? If it is, then we're handling it incorrectly.

If it isn't, then I'd still like to ensure we're handling it correctly for sites that set it correctly. But that would be a new bug.

Updated

5 years ago
Flags: needinfo?(nicolae.cristian)
(Reporter)

Comment 13

5 years ago
I searched on the page source for this setting, but I didn't found anything.

I rechecked this issue on a couple of browsers and as you can see on this video http://youtu.be/LXoolhWLs9Y there is a different behaviour for each tested mobile browser (Nightly, Chrome, Stock Browser, Dolphin and Opera).

I opened http://searchmp3.mobi/download-music-i-dont-like-feat-kanye-west-chief-keef-pusha-t-jadakiss-big-sean-725741.xhtml?source=index (mobile version - it can be switched from the bottom of the page) on Firefox for Desktop and if I tap on the Download button, the download popup is triggered (which asks me what I want: to save the file and where, or open it with an app).

I inspected the Download button and this is how it looks:
<a href="http://searchmp3.mobi/download-music-i-dont-like-feat-kanye-west-chief-keef-pusha-t-jadakiss-big-sean-725741-mp3-free.xhtml?source=index"><img src="http://searchmp3.mobi/assets/mobile/images/download.png" alt="Download: Music - I Dont Like Feat. Kanye West, Chief Keef, Pusha T, Jadakiss &amp; Big Sean mp3 free" title="Download: Music - I Dont Like Feat. Kanye West, Chief Keef, Pusha T, Jadakiss &amp; Big Sean Mp3 Free"></a>

Also, I didn't see any meta name="content-disposition" in page's <head>. Let me know if I can help more regarding this issue.
Flags: needinfo?(nicolae.cristian)

Updated

5 years ago
Duplicate of this bug: 821860
Report of bug 821860 said file-saving of audio previously worked -> Regression.
Keywords: regression
(Assignee)

Comment 16

5 years ago
Hi everyone,
Is this bug still good to work on?
I have been checking out this bug and I would like to work on it.
Yep! I wrote pretty detailed instructions on this in comment #11, but feel free to ping me here or on irc in #mobile if you need help. Step #1 is build Fennec: https://wiki.mozilla.org/Mobile/Fennec/Android

Comment 18

5 years ago
Hi,

I would like to solve this bug for a school project.  Is it available to be worked on?
(Assignee)

Comment 19

5 years ago
Wesley, so far, I have a "Save Video" show up in html5 player's context menu. For a proper html5 video tag in http://html5demos.com/video, the long tap on the video shows "Save Video." As for other video/audio elements in anchor tags such as the download buttons in searchmp3.mobi, comment #7 says that's not what we are after in this bug.

Now I have one question, how can I identify audio files in html5 player?
It seems that when mp3 files are opened in html5 player, it shows up as HTMLVideoElement. Is checking for videoHeight and videoWidth == 0 sufficient?

I've only tested this change with my phone. Please let me know how I can proceed on from this point on. Thank you!
(Assignee)

Comment 20

5 years ago
Created attachment 704133 [details] [diff] [review]
patch to add "Save Video" or "Save Audio" context menu option in HTML5 player(for browser.js)
Attachment #704133 - Flags: review?(wjohnston)
(Assignee)

Comment 21

5 years ago
Created attachment 704134 [details] [diff] [review]
"Save Video" and "Save Audio" string patch (for browser.properties)
Attachment #704134 - Flags: review?(wjohnston)
(Assignee)

Comment 22

5 years ago
Let me know if there's anything I can improve.
The attachments you have added need to be in the form of a patch on the modified files, and not whole attachments of your local files.

https://developer.mozilla.org/en/docs/Creating_a_patch
(Assignee)

Comment 24

5 years ago
Created attachment 704177 [details] [diff] [review]
adding "Save Video" and "Save Audio" to HTML5 player in browser.js

I wasn't sure who to put in for review, so I left it blank.
Attachment #704133 - Attachment is obsolete: true
Attachment #704133 - Flags: review?(wjohnston)
(Assignee)

Comment 25

5 years ago
Created attachment 704178 [details] [diff] [review]
add "Save Video" and "Save Audio" strings in browser.properties

again..wasn't sure who to put for review.
Attachment #704134 - Attachment is obsolete: true
Attachment #704134 - Flags: review?(wjohnston)
(Assignee)

Comment 26

5 years ago
Created attachment 705155 [details] [diff] [review]
mq patch file for bug 791654

Messed up many times, but hopefully i can get it right this time. Thanks Wesley.
Attachment #704177 - Attachment is obsolete: true
Attachment #704178 - Attachment is obsolete: true
Attachment #705155 - Flags: review?(wjohnston)
Comment on attachment 705155 [details] [diff] [review]
mq patch file for bug 791654

Review of attachment 705155 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. I need to test and talk to dolske about the desktop code, so just a feedback+ for now. If you want to address these nits and put it up again, I think it will be good.

::: mobile/android/chrome/content/browser.js
@@ +491,5 @@
>            }
>          });
>        });
> +
> +    //JINSU BUG 791654

This comment likely won't be helpful in the future. Might as well remove it.

@@ +498,5 @@
> +            //my test shows that HTML5 player sees mp3 as video.
> +            //Be smart and display audio context menu
> +            if (aTarget.videoWidth == 0 || aTarget.videoHeight == 0 ) {
> +                return Strings.browser.GetStringFromName("contextmenu.saveAudio");
> +            }

We shouldn't need this. I'm guessing that someone is pushing an audio file in a <video> tag. Do you have an example?

@@ +507,5 @@
> +    }, NativeWindow.contextmenus.mediaSaveableContext,
> +    function(aTarget) {
> +        let url = aTarget.currentSrc || aTarget.src;
> +        let filePickerTitleKey = (aTarget instanceof HTMLVideoElement &&
> +            (aTarget.videoWidth != 0 && aTarget.videoHeight != 0)) ? "SaveVideoTitle" : "SaveAudioTitle";

TBH, these titles aren't even shown, but they're nice to have.

@@ +509,5 @@
> +        let url = aTarget.currentSrc || aTarget.src;
> +        let filePickerTitleKey = (aTarget instanceof HTMLVideoElement &&
> +            (aTarget.videoWidth != 0 && aTarget.videoHeight != 0)) ? "SaveVideoTitle" : "SaveAudioTitle";
> +        //skipped trying to pull MIME type out of cache until further instructions say so
> +        ContentAreaUtils.internalSave(url, null, null, null, null, false, filePickerTitleKey, null, aTarget.ownerDocument.documentURIObject, aTarget.ownerDocument, true, null);

Desktop does something wildly more complex. I have no idea why. I'll ping some people to see if they know. If there is, we should try and share there code. i.e. this saveHelper code should be moved into ContentAreaUtils:

http://hg.mozilla.org/mozilla-central/annotate/2876e73c9b6f/browser/base/content/nsContextMenu.js#l943

@@ +1563,5 @@
> +    mediaSaveableContext: {
> +      matches: function mediaSaveableContextMatches(aElement) {
> +        //a simple check to see if the DOM element is of video/audio tag.
> +        //skipped on checking for 404 links for now because we don't load
> +        //video/audio data without the user's consent.

No need for this one either. The code is implied from the method name.

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +234,5 @@
>  contextmenu.playMedia=Play
>  contextmenu.pauseMedia=Pause
>  contextmenu.shareMedia=Share Video
>  contextmenu.showControls2=Show Controls
> +#JINSU BUG 791654

No need for the comment
Attachment #705155 - Flags: review?(wjohnston) → feedback+
In fact, might as well just ping dolske here. Any idea why the desktop code for saving a media is using the nsIExternalHelperAppService and not just a simple nsIWebBrowserPersist?
Flags: needinfo?(dolske)
I don't think we added anything special for saving media, it just happened to end up using the existing context menu code for saving items. I'm not really familiar with the merits of nsIExternalHelperAppService vs nsIWebBrowserPersist.
Flags: needinfo?(dolske)
(Assignee)

Comment 30

5 years ago
@@ +498,5 @@
> +            //my test shows that HTML5 player sees mp3 as video.
> +            //Be smart and display audio context menu
> +            if (aTarget.videoWidth == 0 || aTarget.videoHeight == 0 ) {
> +                return Strings.browser.GetStringFromName("contextmenu.saveAudio");
> +            }

Added this check because I saw the desktop version doing the similar check.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#498

Also, when I was testing it with my Galaxy S3 device on http://searchmp3.mobi/, the context menu showed up with "Save Video" without this check.
(Assignee)

Comment 31

5 years ago
Created attachment 705399 [details] [diff] [review]
mq patch file for bug 791654 clean

comments cleaned up
Attachment #705155 - Attachment is obsolete: true
Attachment #705399 - Flags: review?(wjohnston)
Comment on attachment 705399 [details] [diff] [review]
mq patch file for bug 791654 clean

Review of attachment 705399 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Just some style nits and still a bunch of whitespace. Can you fix them and add a commit message and then I'll r+ this! i.e. make sure your Username is set in your .hgrc file and then run something like:

hg qref -U
hg qref -m "Bug 791654 - Add Save Video/Audio to context menus. r=wesj"

The top of the patch file should then have that info in it like this one here:

https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/ed2ed42808ca

and upload the patch straight from your mozilla-central/.hg/patches/myPatch.

::: mobile/android/chrome/content/browser.js
@@ +492,5 @@
>          });
>        });
> +
> +    NativeWindow.contextmenus.add(function(aTarget) {
> +        if (aTarget instanceof HTMLVideoElement) {

I should have mentioned earlier. We use a 2 space indent in our JS files.

@@ +493,5 @@
>        });
> +
> +    NativeWindow.contextmenus.add(function(aTarget) {
> +        if (aTarget instanceof HTMLVideoElement) {
> +            //If the media is actually audio,

We don't enforce strict line lengths here. But do put a space after the //. i.e. I'd rather have:

// If a video element is zero width or height, its essentially an HTMLAudioElement.

@@ +495,5 @@
> +    NativeWindow.contextmenus.add(function(aTarget) {
> +        if (aTarget instanceof HTMLVideoElement) {
> +            //If the media is actually audio,
> +            //be smart and display audio context menu
> +            if (aTarget.videoWidth == 0 || aTarget.videoHeight == 0 ) {

And we don't put braces around single line ifs, so this can just be:

if (aTarget.videoWidth == 0 || aTarget.videoHeight == 0 )
  return Strings.browser.GetStringFromName("contextmenu.saveAudio");

@@ +1559,5 @@
>      },
>  
> +    mediaSaveableContext: {
> +      matches: function mediaSaveableContextMatches(aElement) {
> +       if (aElement instanceof HTMLVideoElement) {

Same. No need for these braces, you can even just move this to a single statement:

return (aElement instanceof HTMLVideoElement ||
        aElement instanceof HTMLAudioElement);

@@ +3566,5 @@
>        let restoring = aStateFlags & Ci.nsIWebProgressListener.STATE_RESTORING;
>        let showProgress = restoring ? false : this.showProgress;
>  
>        // true if the page loaded successfully (i.e., no 404s or other errors)
> +      let success = false;

Pull all these whitespace changes out. An easy way to do that is to just open the patch file and delete them physically. It won't work if there are changes further down the file, but these are all after your real changes. Yay!
Attachment #705399 - Flags: review?(wjohnston) → feedback+
(Assignee)

Comment 33

5 years ago
Created attachment 706195 [details] [diff] [review]
code style and white space corrections

Thanks for the heads up. Im learning a lot about good coding styles.
Attachment #705399 - Attachment is obsolete: true
Attachment #706195 - Flags: review?(wjohnston)
(Assignee)

Comment 34

5 years ago
Created attachment 706214 [details] [diff] [review]
Added a line to default to "Save Video" when the element is neither HTMLVideo nor HTMLAudio.

Sorry, Wes. There  was a snafu with some indentations.
Attachment #706195 - Attachment is obsolete: true
Attachment #706195 - Flags: review?(wjohnston)
Attachment #706214 - Flags: review?(wjohnston)
Comment on attachment 706214 [details] [diff] [review]
Added a line to default to "Save Video" when the element is neither HTMLVideo nor HTMLAudio.

Review of attachment 706214 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! I'll get this landed today! Thanks
Attachment #706214 - Flags: review?(wjohnston) → review+
Actually, I'll land right now. Should be in nightly tomorrow:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce79b39c6e0e
(Assignee)

Comment 37

5 years ago
Yay, thanks Wesley!
https://hg.mozilla.org/mozilla-central/rev/ce79b39c6e0e
Assignee: nobody → heyjinsu
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
I don't see this functionality at all in Nightly 01/27 in the context-menus of various HTML5 videos, how is this used?
(Assignee)

Comment 40

5 years ago
Hmm...I mainly tested on 'searchmp3.mobi' and 'html5demos.com/videos'. I'm migrating my hard disk at the moment...I will test it out hopefully by tomorrow.
Ok, I'm not sure why, but this 01/28 has this change-set, perhaps there were a series of back-outs? I can confirm, I see this now in Nightly (01/28). Looks good!
Status: RESOLVED → VERIFIED
status-firefox21: --- → verified
Flags: in-moztrap?(andreea.pod)

Comment 42

5 years ago
TC added in the Context menu suite: https://moztrap.mozilla.org/manage/case/6198/
Flags: in-moztrap?(andreea.pod) → in-moztrap+
relnote-firefox: --- → ?

Updated

5 years ago
relnote-firefox: ? → -
This is a common user request in the support forums. Adding h264/mp3 playback 'broke' the downloading of these files.
relnote-firefox: - → ?

Updated

5 years ago
relnote-firefox: ? → 21+
(In reply to Kevin Brosnan [:kbrosnan] from comment #43)
> This is a common user request in the support forums. Adding h264/mp3
> playback 'broke' the downloading of these files.

Is there a bug on file for that?
(In reply to Justin Dolske [:Dolske] from comment #44)
> (In reply to Kevin Brosnan [:kbrosnan] from comment #43)
> > This is a common user request in the support forums. Adding h264/mp3
> > playback 'broke' the downloading of these files.
> 
> Is there a bug on file for that?

Yeah. Its this one. We fixed it.
Ah! Ok. Sounded like this bug added a menu, but the H.264 support then broke it.

Updated

4 years ago
Whiteboard: [mentor=wesj][lang=js] → [mentor=wesj][lang=js] [html5]

Updated

4 years ago
Blocks: 867604
Depends on: 1303993
You need to log in before you can comment on or make changes to this bug.