Requesting full-screen on a video element offers no back or close button. The user is stuck on the video.

Well...this is a bad UX obviously. Probably want to fix this too.

Apps that implement fullscreen (which is great for mobile videos and I recommend it to partners!) now get punished, as users need to close and kill the app to get out of the video and back to the app. As partner engineer I can confirm that many important apps have videos. Until this is fixed, I can only recommend using web activities to launch the video in the gaia video player. As we currently block apps from submission when we find that users can get stuck by not having a back button; we might want to apply the same measurements on our frontend. Therefor I would nominate it as blocker and get some more input.

I guess this is a problem with the built in video controls? Jared: do we still use the android video controls on B2G? Or are we using something else now?

Can someone confirm if this is apps only or browser as well?

I tried with my Unagi phone and B2G appears to be using the Android touch controls (not the same as desktop Firefox). The built-in touch controls lack an enter fullscreen button, so this problem only appears to be one that will affect apps. Of course, a website can use the DOM Fullscreen API to get a video in fullscreen. Using pearce.org.nz/fullscreen for testing, I opened a video in fullscreen using the B2G browser and then I lacked a back button as well as some weird graphics jank (the video didn't cover the full page and I was still able to pan around; zooming caused the video to move places). The simple way to fix this for B2G will probably be to fix bug 704229 for Android + B2G by adding an enter+exit fullscreen button to the touch controls.

Does anyone from product have input? Rob, do you have any thoughts? My knee-jerk reaction at this point is to implement some sort of workaround like putting the browser in the background makes the video exit fullscreen (weak, I know).

The easiest thing to implement would be the fullscreen enter/exit button on the touch controls.

Comments for triage: We should track this, but my initial hunch is to not block.

(In reply to Jared Wein [:jaws] from comment #7) > The easiest thing to implement would be the fullscreen enter/exit button on > the touch controls. I fully agree.

(And that's a very small/low-risk patch)

From a product perspective, this is a pretty significant usability issue and will lead to user frustration. Unless Josh/Casey disagree, I'm suggesting that we block on this.

(In reply to Peter Dolanjski from comment #11) > From a product perspective, this is a pretty significant usability issue and > will lead to user frustration. Unless Josh/Casey disagree, I'm suggesting > that we block on this. Agreed. I'm glad that this was caught. We cannot allow dead ends in the UX. (In reply to Jared Wein [:jaws] from comment #7) > The easiest thing to implement would be the fullscreen enter/exit button on > the touch controls. Presumably we will still need to create the front end visuals and logic to trigger said button?

(In reply to Josh Carpenter [:jcarpenter] from comment #12) > Presumably we will still need to create the front end visuals and logic to > trigger said button? Yeah, see bug 704229 for that work. An icon will need to be made. The logic is pretty simple and can be copied from the desktop implementation.

Umm...why is this dependent on a FF Android bug? Jared - Can you clarify? Do we need to fix bug 704229 to fix this bug?

Jason: see comment 5. Fixing that bug would apparently fix this one. It used to be that when we were in fullscreen, the home button would exit fullscreen but not go home. The home button also used to do things like dismiss dialogs without actually going to the homescreen. Josh: was that change intentional? Could we fix this bug by just making Home exit fullscreen, and requiring the user to tap Home twice if they want to go to the homescreen from a fullscreen video?

(In reply to David Flanagan [:djf] from comment #15) > Josh: was that change intentional? Could we fix this bug by just making > Home exit fullscreen, and requiring the user to tap Home twice if they want > to go to the homescreen from a fullscreen video? Unfortunately it's not so easy :) Home button should always exit app. The solution here is to add the required UI.

These two attachments show the video controls we get with a <video controls> element in regular non-fullscreen and in fullscreen mode. Peter: I want to add an 'enter fullscreen' button when we're not in fullscreen and an 'exit fullscreen' button when we are in fullscreen. If I was doing it myself, I would use the same icons we use on desktop. I'd move the mute control to the left and put the fullscreen controls on the right where the mute control is now. If you want something different, though, please let me know.

Margaret, Would you take a look at this patch? There are new icons and css in b2g, but nothing new for Fennec. I worry though that my changes to videocontrols.xml will require corresponding changes in the CSS for Fennec, or the layout will be off. Also, it looks like videocontrols.xml is also used with audio elements, so I need to test this patch with <audio> to make sure that no fullscreen icons are offered in that case. I don't know if there are automated tests for this stuff, but if there are, I don't even know how to push the patch to the tryserver...

I forgot to mention, you can reproduce the bug and test the patch by visiting http://djf.net/v

Comment on attachment 709318 [details] [diff] [review] new icons and styles, plus a little new xbl code Review of attachment 709318 [details] [diff] [review]: ----------------------------------------------------------------- Is the intent of this patch to update all of the video icons for B2G? It seems out of scope of just fixing the issue at hand. ::: b2g/chrome/content/touchcontrols.css @@ +81,4 @@ > padding: 0px; > margin: 0px; > background-color: transparent; > + border-radius: 3px; Why is this added as part of this patch? @@ -176,5 @@ > } > > -.controlBar[firstshow="true"] .playButton { > - -moz-transform: none; > -} Why does this code need to be removed? ::: toolkit/content/widgets/videocontrols.xml @@ +1481,4 @@ > </box> > <vbox class="controlBar" hidden="true"> > <hbox class="buttonsBar"> > + <button class="fullscreenButton" The fullscreen button shouldn't be the first button in left-to-right order. It would be better to have the touchcontrols mirror the desktop controls and keep them in the same order where possible. @@ +1615,4 @@ > self.delayHideControls(self.controlsTimeout); > }, false); > this.Utils.muteButton.addEventListener("click", function() { self.delayHideControls(self.controlsTimeout); }, false); > + nit: unnecessary whitespace @@ +1628,2 @@ > > + // If the video is not at the start, then we probably just nit: trailing whitespace

By the way, and I should have said this in my previous comment, thank you for getting a patch together for this bug so quickly.

(In reply to Jared Wein [:jaws] from comment #21) > Comment on attachment 709318 [details] [diff] [review] > new icons and styles, plus a little new xbl code > > Review of attachment 709318 [details] [diff] [review]: > ----------------------------------------------------------------- > > Is the intent of this patch to update all of the video icons for B2G? It > seems out of scope of just fixing the issue at hand. Visual designer Peter La provided the new icons. He was pretty horrified by the existing look. Like "we can't ship this" horrified. > ::: b2g/chrome/content/touchcontrols.css > @@ +81,4 @@ > > padding: 0px; > > margin: 0px; > > background-color: transparent; > > + border-radius: 3px; > > Why is this added as part of this patch? > I haven't actually run that by Peter. But since the icons became slightly more rounded with this patch, I tried to round the progress bar too. > @@ -176,5 @@ > > } > > > > -.controlBar[firstshow="true"] .playButton { > > - -moz-transform: none; > > -} > > Why does this code need to be removed? > Previously, there were just two buttons in an <hbox>, so the play button was not centered when play and mute were displayed. So elsewhere we had a transform to move the play button to the right. But now that there are three buttons we don't need that other transform. So we don't need to remove it here. > ::: toolkit/content/widgets/videocontrols.xml > @@ +1481,4 @@ > > </box> > > <vbox class="controlBar" hidden="true"> > > <hbox class="buttonsBar"> > > + <button class="fullscreenButton" > > The fullscreen button shouldn't be the first button in left-to-right order. > It would be better to have the touchcontrols mirror the desktop controls and > keep them in the same order where possible. > I agree, but implemented it as visual design asked me to. Peter, do you want to reconsider this? Jared: in what order would you put the buttons? play, mute, fullscreen? Or keep play in the center and do mute, play, fullscreen? > @@ +1615,4 @@ > > self.delayHideControls(self.controlsTimeout); > > }, false); > > this.Utils.muteButton.addEventListener("click", function() { self.delayHideControls(self.controlsTimeout); }, false); > > + > > nit: unnecessary whitespace > > @@ +1628,2 @@ > > > > + // If the video is not at the start, then we probably just > > nit: trailing whitespace Jared: do you have any idea how to get rid of the thin box that appears around the scrubber? Its an accessibility thing, I think, but I don't know that we need it here. It doesn't seem to show up in Fennec, but Peter wanted it gone in B2G, and I couldn't figure out how to do it. (I tried the ::-moz-inner-focus pseudo element but it wouldn't go away for me.)

Comment on attachment 709318 [details] [diff] [review] new icons and styles, plus a little new xbl code Thanks for beating me to leave feedback, Jared :) I agree that the touch controls don't look as nice as the desktop controls, so they could use some UX love. Does Peter have any mockups of what this should look like? Because fennec has a different touchcontrols.css, this patch actually doesn't affect it, so we can remove the dependency on bug 704229 (I tested a fennec build to verify). Obviously it would be nice to fix that bug, but we can do that separately. Actually, I'm wondering if there is a good reason why these styles aren't in a shared file in toolkit. Currently the files are identical except for paths to images, so I think it could be a good follow-up to use a shared file. It might be tricky with the way we're currently just overriding videocontrols.css, but that's probably an artifact of when mobile support was just tacked on as an afterthought.

(In reply to David Flanagan [:djf] from comment #23) > Jared: do you have any idea how to get rid of the thin box that appears > around the scrubber? Its an accessibility thing, I think, but I don't know > that we need it here. It doesn't seem to show up in Fennec, but Peter wanted > it gone in B2G, and I couldn't figure out how to do it. (I tried the > ::-moz-inner-focus pseudo element but it wouldn't go away for me.) I don't have a b2g build environment set up, so I can only provide some possible things to look in to. One of the following may fix it (using #scrubber to reference the scrubber element for simplification), #scrubber:focused { outline-width: 0; } #scrubber:focused { border-width: 0; } #scrubber:-moz-focus-ring { outline-width: 0; } #scrubber:-moz-focus-ring { border-width: 0; }

This patch adds a panel to the Gaia UITests app to display the default video and audio controls and verify that video has buttons for entering and leaving fullscreen but that audio does not.

In addition to the UITest panel above, I've also updated the test case at djf.net/v to include an audio element and a <meta> tag so it displays at a reasonable size.

(In reply to :Margaret Leibovic from comment #24) > Comment on attachment 709318 [details] [diff] [review] > new icons and styles, plus a little new xbl code > > Thanks for beating me to leave feedback, Jared :) > > I agree that the touch controls don't look as nice as the desktop controls, > so they could use some UX love. Does Peter have any mockups of what this > should look like? Yes, he gave me mockups, and I've implemented them. So this is what Peter wanted (except he wanted to get rid of the outline/border box around the thumb, and I didn't ask him about rounding the corners of the progress bar). > Because fennec has a different touchcontrols.css, this patch actually > doesn't affect it, so we can remove the dependency on bug 704229 (I tested a > fennec build to verify). My worry was that because I've changed the tree structure in videocontrols.xml (by moving the fullscreen button from one container to another) that the existing touchcontrols.css file for fennec would no longer be appropriate and the buttons would be in the wrong places. If you still have the test fennec build, would you mind trying the updated test page (djf.net/v) again to make sure that this hasn't messed up the audio tag controls? Obviously it would be nice to fix that bug, but we > can do that separately. Okay. If I haven't broken Fennec with this patch, I'm fine with just landing the FirefoxOS fix. Actually, I'm wondering if there is a good reason > why these styles aren't in a shared file in toolkit. Currently the files are > identical except for paths to images, so I think it could be a good > follow-up to use a shared file. It might be tricky with the way we're > currently just overriding videocontrols.css, but that's probably an artifact > of when mobile support was just tacked on as an afterthought. I'm not qualified to do that part. I'll address Jared's whitespace nits, and check with Peter again about the order of the button and the border radius thing, then I'll ask for final review before landing

Comment on attachment 709787 [details] link to patch on github Looks good to me. The build on my device is out of date and not cooperating, but I tested on desktop Firefox and it looks good.

As Margaret suggests, I'm removing the dependency on bug 704229. We'll just fix this for FirefoxOS here and let someone from the Fennec team fix the corresponding Fennec bug.

This screenshot shows what the audio controls look like in FirefoxOS with this patch. I think the the problem is that when the fullscreen button is hidden (with XUL hidden:true) the layout of the reamining two buttons no longer works right. It used to be that neither video nor audio had three buttons, and the CSS had a workaround to move the play button to the right. But now I only need the workaround for audio controls. And, because we're going to fix the fennec bug separately, I can't change videocontrols.xml too much or I might break the Fennec version.

This screenshot shows the fix for the audio tag, and is the visual that I plan to land. Peter: I wasn't able to figure out how to get rid of the box around the scrubber or even how to center it around the scrubber.

Margaret, This is ready for review now. The last little tweak I had to make was to set a CSS style when the controls were displayed on an audio tag so that I could adjust the position of the play button in css.

Ignore that.... Looks like I attached the wrong patch

Hopefully this is the right version of the patch!

Okay, I'm attaching the patch more more time. This time with the "patch" box checked.

I've merged the gaia uitest into master: https://github.com/mozilla-b2g/gaia/commit/a09adaad108292cc5dbce8ff53c6cb452a8b16be

Comment on attachment 709885 [details] [diff] [review] revised patch with audio controls fixed, nits addressed Review of attachment 709885 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, but I'd like to get Jared's feedback about the class vs. attribute selector. ::: b2g/chrome/content/touchcontrols.css @@ +50,5 @@ > > +/* > + * normally the button bar has fullscreen spacer play spacer mute, but if > + * this is an audio control rather than a video control, the fullscreen button > + * is hidden by videocontrols.xml, and that alters the position of the Nit: trailing whitespace. Also, my OCD wants these to both be full sentences with capitalized letters and periods, instead of a mix :) @@ +53,5 @@ > + * this is an audio control rather than a video control, the fullscreen button > + * is hidden by videocontrols.xml, and that alters the position of the > + * play button. This workaround moves it back to center > + */ > +.controlBar.audio .playButton { In gecko code, I feel like we usually prefer setting attributes rather than adding class names, but I'm not sure if there's a reason for that or if it's just habit. I would maybe favor that just to be consistent with the rest of this file. Jared, do you know if there's any reason why one might be preferred over the other? ::: toolkit/content/widgets/videocontrols.xml @@ +1633,5 @@ > + // the controls to remain visible. this.controlsTimeout is a full > + // 5s, which feels too long after the transition. > + if (video.currentTime !== 0) { > + this.delayHideControls(this.Utils.HIDE_CONTROLS_TIMEOUT_MS); > + } Testing in Fennec, I think this actually fixes a bug where the video controls reappear when you enter full screen mode. Nice :)

Comment on attachment 709885 [details] [diff] [review] revised patch with audio controls fixed, nits addressed Review of attachment 709885 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/touchcontrols.css @@ +53,5 @@ > + * this is an audio control rather than a video control, the fullscreen button > + * is hidden by videocontrols.xml, and that alters the position of the > + * play button. This workaround moves it back to center > + */ > +.controlBar.audio .playButton { There should be no technical reason why an attribute should be preferred over className. I think the habit may originate from how XUL controls can change based on the value of attributes. I'm fine with leaving this as is.

This is ready to land. Setting checkin-needed. It also needs to be uplifted. I'm hoping that the tef+ flag and the status-b2g18 and status-b2g18v1.0.0 flags are enough to make that happen.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #43) > https://hg.mozilla.org/mozilla-central/rev/d6f27aa7aa9c Who is going to uplift this to the gecko-18 and v1.0.0 branches? I think that's required here before we can close out this bug.

John, on the Gecko side of things, our standard practice is to close bugs when they hit mozilla-central and use the tracking flags for branch uplifts. I will uplift this bug to the b2g18 branches. It didn't show up in my queries because I purposefully ignore bugs filed under Gaia when looking for branch uplifts (since they're typically landing on Github). In the future, you can set checkin-needed on the bug (like I've done here) and I will find it that way (I look for those regardless of the component they're filed under).

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7f0ce822f072 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/9c0f96f8f3ba

Build ID:20130214070203 Gecko http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d1288313218e Gaia 6544fdb8dddc56f1aefe94482402488c89eeec49 Kernel: Dec 5 Verified this bug no longer reproduces on "Unagi" device