946454 - [Roku] Show "casting" button in the video controls

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Zhenshuo Fang (:fang) - Firefox UX Team]

The "cast to screen" click feels like an extra click.

Current:
1. Long tapping on video, show "Cast to screen"
2. Click on "Cast to screen", show "Cast to device" & device list
3. Click on device name "Roku streamming device" to start streaming

Expect:
1. Long tapping on video, show "Cast to device" UI
2. Click on device name and start streaming

Comment 1

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

Sounds like you want long tapping on a video to jump straight to the list of streaming devices.

What about the context menu for a video? I think we still need to keep the regular context menu.

If we want a simpler flow, we should do as Wes suggested and add a "Cast" button to the video controls that appear when you tap a video. One downside to the single tap "controls" is that web pages can override them, so we still need a secondary way of getting to the cast UI.

Comment 2

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

I am changing this bug to add a "casting/disconnect" button to the video controls XBL binding.

Comment 3

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

Attachment: cast-button-control WIP

This is my first attempt at getting this to work.
* Add a "cast" button to the videocontrols.xml binding.
* Show and hide the button based on the type of the video media (.mp4)
* Fire an event to chrome to start the casting process.

Todo:
* Show and hide the button based on the presence of a casting device on the network.

Comment 4

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

Comment on attachment 8401992 [details] [diff] [review]
cast-button-control WIP

Thoughts on how to improve this?

Comment 5

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

Attachment: cast-button-control WIP0.2

Started using CustomEvents after talking to Dolske

Comment 6

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

Attachment: cast-button-control WIP0.3

This version adds code to let the binding know when Firefox is casting the binding (active=true) and when it's stopped (active=false).

The patch keeps a weak reference to the video element. This should protect us from circular references and allow us to nicely deactivate a video element, even if it's not in the open tab.

The patch adds an "active" state image as well. Both the ready and active state images are from the HDPI Android resources that Google wants applications to use.

TODO:
* Try to move as much of the code out of the base videocontrols binding and put it in the touchcontrols binding.

Comment 7

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

Attachment: cast-button-control v0.1

This version of the patch moves all of the casting button code and markup into the touchcontrols binding. Nothing of the casting code is in the regular binding.

I did make a few small changes to the touchcontrols binding:

* Added a video data member, instead of just a local in init(). The Utils.video (pulling it from videocontrols) did not always work, since the base videocontrols binding might not be ready and lose the race.

I plan to make a small test for this too, but let the reviews being!

Comment 8

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

Attachment: fennec-video-casting-controls.png

Screenshot of the video controls. The top video is not being cast (white button), but the bottom video is being cast (blue filled button).

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 8405721 [details] [diff] [review]
cast-button-control v0.1

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

A few questions from me. Overall looks good though :) Mostly nits, but a few questions that I'm pretty sure you have answers to.

::: mobile/android/chrome/content/CastingApps.js
@@ +62,5 @@
>          break;
>      }
>    },
>  
> +  handleVideoBindingAttached: function handleVideoBindingAttached(aTab, aEvent) {

The "everything should just do one thing" in me guy thinks this should be in a separate class.

@@ +81,5 @@
> +
> +    // Let the binding know casting is allowed
> +    let event = video.ownerDocument.createEvent("CustomEvent");
> +    event.initCustomEvent("media-videoCasting", false, true, JSON.stringify({ allow: true }));
> +    video.dispatchEvent(event);

This won't dynamically adjust. Nor do we handle cases where we're already casting something and you open it again very well.

@@ +297,5 @@
> +    let video = this.session.videoRef.get();
> +    if (video) {
> +      let event = video.ownerDocument.createEvent("CustomEvent");
> +      event.initCustomEvent("media-videoCasting", false, true, JSON.stringify({ active: true }));
> +      video.dispatchEvent(event);

You might as well wrap this up in a helper function.

_sendEventToVideo(video, {active: true});

::: toolkit/content/widgets/videocontrols.xml
@@ +1655,5 @@
> +              let unwrappedVideo = XPCNativeWrapper.unwrap(this.video);
> +              let castingData = JSON.parse(eventDetail);
> +              if ("allow" in castingData) {
> +                if (castingData.allow)
> +                  unwrappedVideo.mozAllowCasting = true;

Why is this stored on the video and not in the binding.

@@ +1662,5 @@
> +              }
> +
> +              if ("active" in castingData) {
> +                if (castingData.active)
> +                  unwrappedVideo.mozIsCasting = true;

And this?

@@ +1670,5 @@
> +              this.setCastButtonState();
> +            },
> +
> +            startCasting : function () {
> +              this.videocontrols.dispatchEvent(new CustomEvent("VideoBindingCast"));

It confuses me a bit that this doesn't toggle the state, but after talking it through with you, it sounds worth testing this way for now, and adjusting once we have some users.

@@ +1681,5 @@
> +                return;
> +              }
> +
> +              if (unwrappedVideo.mozIsCasting) {
> +                this.castingButton.setAttribute("active", "true");

'active' is a strange attribute name, since its also a pseudoclass. I have nothing better though :)

@@ +1686,5 @@
> +              } else {
> +                this.castingButton.removeAttribute("active");
> +              }
> +
> +              var value = this.castingButton.getAttribute("castinglabel");

I'm sure there's a good reason for this, but why not just set it directly?

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 8405721 [details] [diff] [review]
cast-button-control v0.1

Looks ok to me. Wesley's review if fine for the touch/mobile bits.

Nit: Might want to ponder about something more descriptive for "cast to device". Just because "device" seems like such an ambiguous term (and we usually use it to refer to mobile devices).

Comment 11

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

(In reply to Wesley Johnston (:wesj) from comment #9)

> ::: mobile/android/chrome/content/CastingApps.js

> > +  handleVideoBindingAttached: function handleVideoBindingAttached(aTab, aEvent) {
> 
> The "everything should just do one thing" in me guy thinks this should be in
> a separate class.

I do too, but CastingApps is the best place for it IMO. I don't want to scatter video casting code around too much.

> > +    let event = video.ownerDocument.createEvent("CustomEvent");
> > +    event.initCustomEvent("media-videoCasting", false, true, JSON.stringify({ allow: true }));
> > +    video.dispatchEvent(event);
> 
> This won't dynamically adjust. Nor do we handle cases where we're already
> casting something and you open it again very well.

Right. That is more complicated and I wanted to experiement with it in a different bug. I don't think it blocks getting the first pass of this code landed.

> > +      let event = video.ownerDocument.createEvent("CustomEvent");
> > +      event.initCustomEvent("media-videoCasting", false, true, JSON.stringify({ active: true }));
> > +      video.dispatchEvent(event);
> 
> You might as well wrap this up in a helper function.
> 
> _sendEventToVideo(video, {active: true});

Done

> ::: toolkit/content/widgets/videocontrols.xml

> > +                if (castingData.allow)
> > +                  unwrappedVideo.mozAllowCasting = true;
> 
> Why is this stored on the video and not in the binding.
> 

> > +              if ("active" in castingData) {
> > +                if (castingData.active)
> > +                  unwrappedVideo.mozIsCasting = true;
> 
> And this?

The existing code using this pattern to allow the chrome JS to check for state without exposing it to content.

> > +              var value = this.castingButton.getAttribute("castinglabel");
> 
> I'm sure there's a good reason for this, but why not just set it directly?

I was following the pattern in the file, but you're right. We can just set it directly.


(In reply to Justin Dolske [:Dolske] from comment #10)

> Nit: Might want to ponder about something more descriptive for "cast to
> device". Just because "device" seems like such an ambiguous term (and we
> usually use it to refer to mobile devices).

You're right. "Device" is not what I wanted anyway. "Screen" is better suited, so now the string is "Cast to Screen".

Comment 12

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

Try run looks fine:
https://tbpl.mozilla.org/?tree=Try&rev=218767888d6a

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/45a45f4a228f

leave open. i promised a test.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/45a45f4a228f

Comment 15

[Flaviu Cos]

Verified as fixed in builds:
- 35.0a1 (2014-09-30);
- 34.0a2 (2014-09-30);
- 33 beta 7;
Device: Asus Transformer Tab (Android 4.2.1).