1303993 - "Save audio" option displayed when long tapping a video

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect, P2)

Description

[u549602]

Environment: Aurora 
Device: Samsung Galaxy S6 EDGE (Android 6.0 );
Build: Aurora 51.0a2 (2016-09-20);

Steps to reproduce:
1. Go to http://people.mozilla.org/~atrain/mobile/tests/media.html
2. Long tap on a video 
3. Play the video
4. Long tap again on the video

Expected result:
@ Step 2: The context menu appears with the Save Video option
@ Step 4: Same as step 2

Actual result:
@ Step 2: The context menu appears with the Save Audio option
@ Step 4: The context menu appears with the Save Video option

Notes:
Please note that even if the user taps on Save Audio option, the download resulted is the correct video.
This issue is present on all devices and all Android versions.

For further details please check : https://www.youtube.com/watch?v=aN7aXnR5D5k

Comment 1

[Blake Wu [:bwu][:blakewu]]

Good job to find this problem!

Comment 2

[Blake Wu [:bwu][:blakewu]]

I cannot see this problem on desktop Firefox.
Ray,
Can you help check this bug?

Comment 3

[Ray Lin[:ralin]]

The context menu display item accordingly. I narrowed down the possible condition that what situation for which button to show up [0]. I guess one of the condition is fulfilled in certain state while playing (maybe size or readyState?) and make menu change from Save Audio to Save Video. I will test that tomorrow since I got no devices in hands. Thanks.


[0] https://dxr.mozilla.org/mozilla-central/rev/62f79d676e0e11b3ad59a5425b3ebb3ec5bbefb5/browser/base/content/nsContextMenu.js#723-731

Comment 4

[Ray Lin[:ralin]]

Some points cause this issue:

First, mobile's preference default not to preload data [0] but desktop default does. That's the main cause why the behavior between desktop and mobile is different. Note that when the "media.preload.default" is set to 1, video's "readyState" will remain "HAVE_NOTHING" till user click and play video.

Second, according to second paragraph of videoWidth/Height spec [1]:
"The videoWidth IDL attribute must return the intrinsic width of the video in CSS pixels. The videoHeight IDL attribute must return the intrinsic height of the video in CSS pixels. If the element's readyState attribute is HAVE_NOTHING, then the attributes must return 0." 

Follow with first point and the spec, if video haven't been played by user, the attributes videoWidth and videoHeight would return 0. Unfortunately, the condition just fulfill the statement[2] which determine isAudio or not. This explain why "Save Audio" shows before video played.

I think mobile's behavior is intended for saving bandwidth, however, it cause the misleading message. IMHO, this seems to be inevitable since we could not tell isAudio or isVideo unless we preload some of data (metadata).

[0] https://dxr.mozilla.org/mozilla-central/rev/62f79d676e0e11b3ad59a5425b3ebb3ec5bbefb5/mobile/android/app/mobile.js#602-603
[1] https://html.spec.whatwg.org/#dom-video-videowidth 
[2] https://dxr.mozilla.org/mozilla-central/rev/62f79d676e0e11b3ad59a5425b3ebb3ec5bbefb5/mobile/android/chrome/content/browser.js#926-928

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

There is no way to tell whether a file contains audio or video tracks before loading metadata. Can we just use a neutral text like "Save media" or "Save file" to fix this issue?

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Annotate [2] I found bug 791654. The justification is commented here:

http://searchfox.org/mozilla-central/rev/8910ca900f826a9b714607fd23bfa1b37a191eca/mobile/android/chrome/content/browser.js#911-912

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

which I can't really say I agree. :antlam, would you might tell us the preferred solution here? We couldn't workaround comment 5 so we would either have to show "Save media" or "Save file" sometimes, or we would have to remove the mentioned behavior here, and always show "Save Video" for video element and "Save Audio" for audio element.

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Spoke in person. Sorry this took so long but I think "Save media" makes the most sense.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1303993 - Display "Save Media" for long tapping unloaded video,

Review commit: https://reviewboard.mozilla.org/r/93334/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93334/

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8811113 [details]
Bug 1303993 - Display "Save Media" for long tapping unloaded video,

https://reviewboard.mozilla.org/r/93334/#review93336

LGTM

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a380f84548d
Display "Save Media" for long tapping unloaded video, r=sebastian

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4a380f84548d

Comment 12

[areya]

https://hg.mozilla.org/mozilla-central/rev/4a380f84548d

Comment 13

[Bogdan Surd, QA [:bsurd, NI]]

Hello,

I've verified this issue and "Save Media" is indeed being displayed now before playing a video and changes to "Save Video" once you hit the play button since the video does not have a zero width or height anymore.