1018468 - Provide fullscreen button for video

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

People always complain about this. We should provide it.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

This is a quick WIP. I also removed the padding around the controls because I hate it more than anything else in the world. Screenshot at (with casting also enabled):

http://people.mozilla.org/~wjohnston/video1.png

Stole icons from B2G. Also, need to fix the play button positioning. I don't think we want to make this too complex and keep it focused on fullscreen buttons (we can do that in a different bug...), but pinging UX for whatever feedback they have

Comment 2

[Ian Barlow (:ibarlow)]

Can we use this as an opportunity to clean up the look of our video controls in general? I think it's time. 

Anthony, want to take a look at this when you have a moment?

Comment 3

[Wesley Johnston (:wesj)]

Maybe, but some changes are harder than others. i.e. the scrubber in the middle of the video is actually a stack of a few scrubbers and a special draggable thumb. Changes to them are hard to get right, and I'd rather do that separately.

Comment 4

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

As a basic video player unification would be great. Do we know what the overall scope would be for this? Like Wes pointed out, some of these features would be harder to move the needle on than others and I want to get an overall feel for it before I start on this again. Or is it safe to assume everything in the image preview are our controls? (Cast, Play/pause, volume (and scrubber), and scrolling (scrubber))

I have some icons for the controls so that would be a good start.

Comment 5

[Wesley Johnston (:wesj)]

We have control over everything in the controls box. The more different it is from the original, the harder it likely is. Like I said, the scrubber is a bit of a beast. This is Gecko drawing, not native ui, so animations may suffer some performance problems.

These need so much work that these bugs always get derailed into "rewrite the whole thing". I'd like to keep this focused on the fullscreen button and really minor changes (i.e. image swaps, colors, the padding was trivial to remove here), and we can do some more complex stuff somewhere else. If you have ideas/mockups, I can try to give assessments on the difficulty of things. I can try and copy the HTML into a desktop page that we can easily tinker with as well...

That risks someone deciding its "good enough now" and the complex stuff falling back into the bin of tasks to do "later". If UX wants to revamp these, I'd like to get something on the roadmap and give it some priority.

Comment 6

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

Alright, well let me add the full screen icon to my queue and we can go from there. I'll paste it into this bug when I have one for you.

Comment 7

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

Attachment: preview_videocontrols.png

Posting a WIP for now to get some thoughts on this. Essentially I'm weighing the tailed vs. tail-less versions of the arrows. I've attached the entire set (as it stands atm) to get a better overview.

Thoughts?

Comment 8

[Ian Barlow (:ibarlow)]

Thanks Anthony. I think I like #2 for the full screen control. 


(In reply to Wesley Johnston (:wesj) from comment #5)
> That risks someone deciding its "good enough now" and the complex stuff
> falling back into the bin of tasks to do "later". If UX wants to revamp
> these, I'd like to get something on the roadmap and give it some priority.

I get that. Looks like bug 704229 is starting to get some attention again too, perhaps we could roll this work in with that? 

Anyway as a first pass, I would propose to replace all the 3 year old icons we're using, and finesse the positioning of some of the icons and time indicators. If we can refine even more, great, but that would be my bare minimum. And sorry Wes but I would block landing this bug on those visual changes. Like you said, I don't want to keep adding little bits and not fixing the harder ones that make our overall video experience feel wonky.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This just adds the fullscreen button like before. It centers it better than the old patch.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8438019 [details] [diff] [review]
Patch v1

Looks simple enough and does not change any code in the binding, so I don't think we need any other reviews here.

Comment 11

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/25fb66c1006f

Maybe UX can open a separate bug when they have some designs?

Comment 12

[Ian Barlow (:ibarlow)]

Grrr. Let's do it all in bug 704229

Comment 13

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

^ I was just submitting my comment to this lol

Comment 14

[:Margaret Leibovic]

Backed out for some failures in /tests/toolkit/content/tests/widgets/test_videocontrols_standalone.html

https://hg.mozilla.org/integration/fx-team/rev/5d3393c15401

Comment 15

[Wesley Johnston (:wesj)]

Grr. Removing the padding around the outside broke some tests with hardcoded sizes. Changing it seems to fix them:

https://tbpl.mozilla.org/?tree=Try&rev=a9b073523d07

Will upload that patch tonight.

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch v2

The only change here is the test fix. Thats needed because we removed margins on the controls making them slightly "smaller".

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bb62b533b336

Comment 18

[Teodora Vermesan (:TeoVermesan)]

Opening http://www.w3.org/2010/05/video/mediaevents.html, the video has the fullscreen button  on the left, so:
Verified fixed on:
Device: LG Nexus 4
OS: Android 4.4.2
Build: Firefox for Android 32.0a2 (2014-07-02)