Closed
Bug 1071831
Opened 10 years ago
Closed 10 years ago
HTML5 video playback is not paused in the browser after opening in external player (open with app)
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox32 affected, firefox33 affected, firefox34 affected, firefox35 affected, fennec+)
RESOLVED
FIXED
Firefox 35
People
(Reporter: czerny.jakub, Assigned: esawin)
References
Details
(Keywords: reproducible)
Attachments
(1 file, 2 obsolete files)
4.76 KB,
patch
|
esawin
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0 Build ID: 20140831201947 Steps to reproduce: a) <video> tag in HTML page 1. open page containing video tag with video with sound 2. start video playback 3. long click on video to view context menu 4. select "Open With an App" or b) playing standalone video file 1. open video file with sound (*.mp4, *.webm) 2. ensure the video is playing 3. open video in external app by clicking on android icon at the end of the address bar Actual results: Video played inside Firefox is not paused. Screen is overlaid by player application. The sound of video is played twice. Expected results: Playback in Firefox should be paused when the video file is opened in external application.
Comment 1•10 years ago
|
||
Which device, Android version, player used, and version of Firefox are you using?
OS: Android → Linux
Hardware: All → x86_64
Reporter | ||
Updated•10 years ago
|
OS: Linux → Android
Hardware: x86_64 → All
Updated•10 years ago
|
OS: Linux → Android
Hardware: x86_64 → All
Reporter | ||
Comment 2•10 years ago
|
||
Device: Samsung Galaxy S2 Android: 4.1 Player: VLC, generic android video player Firefox: 32
Comment 3•10 years ago
|
||
Thanks. I was able to reproduce. Seems annoying, tracking?
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
status-firefox32:
--- → affected
status-firefox33:
--- → affected
status-firefox34:
--- → affected
status-firefox35:
--- → affected
Ever confirmed: true
Keywords: reproducible
Summary: HTML5 video is not paused when opened in external player → HTML5 video playback is not paused in the browser after opening in external player (open with app)
Updated•10 years ago
|
Assignee: nobody → esawin
tracking-fennec: ? → +
Comment 4•10 years ago
|
||
This should be very trivial, but there are two paths through which this can happen. Long pressing on a video and opening a video directly in Firefox. The first is trivial. We just need to pause in here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7812 The second goes through: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7848 getting the video and pausing it there will be more complex.
Assignee | ||
Comment 5•10 years ago
|
||
Pause media playback when launching external apps, resume if external app launch is canceled. We could resume playback when exiting the external app, too. It would need some more work though and might not be the desired behavior.
Attachment #8497547 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 6•10 years ago
|
||
Comment on attachment 8497547 [details] [diff] [review] video-pause.patch This seems sane to me, but I want Wes to see if this is the right approach and whether you have enough of the code paths covered.
Attachment #8497547 -
Flags: review?(wjohnston)
Attachment #8497547 -
Flags: review?(mark.finkle)
Attachment #8497547 -
Flags: feedback+
Comment 7•10 years ago
|
||
Comment on attachment 8497547 [details] [diff] [review] video-pause.patch Review of attachment 8497547 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +7825,5 @@ > > + _getMediaContentElement(element) { > + // Opening media files directly constructs a dummy document with one > + // child node holding the media content. > + return element && element.childNodes[0] || element; I don't really like this. Its confusing enough what's happening here and why it works that you should add some inline docs. i.e. this function document care about media elements at all. It just checks if the element we're passed has a child, if it does it returns the first child, otherwise it returns the element itself. Video elements have source tags, so its possible they'll have children inside them. Will this fail then? What if you pass in a random <div>. This returns something to you in that case. Should it? I guess this is really only ever passed document.activeElement for entire pages. On a MediaDocument there won't be any <source> tags so that will either be the <body> or the <video> and if we get the <body> this will pick the <video>. On non-MediaDocuments the results will be a bit random depending on what is focused. I think for here we only really care about MediaDocuments so we should try to return null if the document is a non-Media one. Maybe you could check document.contentType before calling updatePageAction, and only pass an element if the type is video or audio. If it is one of those, document.body.firstChild is (probably?) always the node you want, so you can remove this check/function entirely. i.e. don't pass an element if its not a video/audio? @@ +7845,5 @@ > clickCallback: () => { > UITelemetry.addEvent("launch.1", "pageaction", null, "helper"); > > + let wasPlaying = mediaElement && mediaElement.play && !mediaElement.paused && !mediaElement.ended; > + if (wasPlaying && mediaElement.pause) { Its confusing that you check for mediaElement.play here, but check for pause in the line before you call pause. We should at least be consistent (or just throw if you get a mediaElement that's not instanceof Ci.nsIDOMHTMLMediaElement and don't do these checks at all).
Attachment #8497547 -
Flags: review?(wjohnston) → review-
Assignee | ||
Comment 8•10 years ago
|
||
Thanks for the review! I did remove the checks for the pause and play functions and check for the expected type explicitly instead. However, I have left the type check inside of _setUriForPageAction, since I don't want to add redundancy each time before calling updatePageAction and would like to defer the check as long as possible, i.e., only do it when page actions are actually available for the given document. Do you still prefer doing the content type checking before calling updatePageAction instead?
Attachment #8497547 -
Attachment is obsolete: true
Attachment #8502101 -
Flags: review?(wjohnston)
Comment 9•10 years ago
|
||
Comment on attachment 8502101 [details] [diff] [review] video-pause.patch Review of attachment 8502101 [details] [diff] [review]: ----------------------------------------------------------------- I can live with this :) Thanks :) ::: mobile/android/chrome/content/browser.js @@ +7903,5 @@ > > + _getMediaContentElement(contentDocument) { > + if (contentDocument.contentType.indexOf("video/") !== 0 && > + contentDocument.contentType.indexOf("audio/") !== 0) { > + return null; "startsWith" is a little clearer than indexOf() !== 0. @@ +7911,5 @@ > + > + if (element instanceof HTMLBodyElement) { > + element = element.firstChild; > + } > + if (element instanceof HTMLMediaElement) { At a line between these two checks.
Attachment #8502101 -
Flags: review?(wjohnston) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Review comments addressed. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7c8f841a44a3
Attachment #8502101 -
Attachment is obsolete: true
Attachment #8502584 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/1cf977b4f846
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1cf977b4f846
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•