Kevin Brosnan Reporter
	Description • 14 years ago

Open a page with <video> and controls="true". Pause button covers part of the video till you pause/unpause the video.

HTC G2
Build ID: http://hg.mozilla.org/mozilla-central/rev/e0ea4e4d401f
build date: 2011-01-21

	Aakash Desai [:aakashd]
	Comment 1 • 14 years ago

What would you propose as a solution here, Kevin?

	Kevin Brosnan Reporter
	Comment 2 • 14 years ago

Button should fade out. Seems odd that when you start the video the pause button continually covers the video but if you pause the video and then start it again the pause button disappears.

	Matt Brubeck (:mbrubeck)
	Comment 3 • 14 years ago

In bug 465839 we talked about making the controls disappear after 5 seconds,
but I'm not sure if that was ever implemented or not. (It's not happening now.)

Note: You can also make the controls disappear by tapping anywhere else in the
video.

	Wesley Johnston (:wesj) Assignee
	Comment 4 • 14 years ago

Hmm... Disappears after a few moments for me. I wonder if this is fallout from bug 618124. I'll take and look into it.

	Wesley Johnston (:wesj) Assignee
	Comment 5 • 14 years ago

Hmm... The same things actually happens on Desktop Firefox with these videos. i.e. starting playback causes the controls to appear and they won't disappear until you mouse in/out of them. Still digging into why/how...

	Wesley Johnston (:wesj) Assignee
	Comment 6 • 14 years ago

So the problem here is that the page is calling video.controls = true, which in Desktop Firefox causes the controls to appear (and not disappear until the user mouses on/off the element). On touch devices, we also show the controls, but need to set a timeout so that they hide themselves again.

This also sets up a weird condition where the controls hide themselves, but the hidden and fadeout attributes aren't set. Not quite sure how/why that is happening. For now I just changed to monitor the style and fadeout attributes. Not a good solution, but I'm not sure what else to look at without touching the desktop controls (which I am trying to avoid).

	Wesley Johnston (:wesj) Assignee
	Comment 7 • 14 years ago

The videos on this page currently don't play at all because they check for the HAVE_CURRENT_DATA ready state. We don't fire that anymore because in bug 631058, we changed to default to preload="none". You can change this pref (media.preload.default) to make them work, or the video could provide a preload attribute, or it could base its ability to play on something else.

Patch is slightly updated to remove the opacity hack. Still not exactly sure what is going on, but I am checking for the hidden attribute now instead of controlBar.hidden.

Talked to mfinkle, and it would be nice if there was some way to avoid using DOMAttrModified events, as they are expensive. This particular video actually has a controls attribute when it is created though, and then has it set to false by script (i.e. video.controls = false). I'm not sure how/if we can detect those changes.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 8 • 14 years ago

Justin - Any ETA on a review for this? Fennec 4 final code freeze approaches...

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 9 • 14 years ago

softblocker

dolske - review ping

	Justin Dolske [:Dolske]
	Comment 10 • 14 years ago

Hmm. So, I was unable to reproduce this in a desktop build, until Wes showed me the specific STR... You need to click the page's play button on the video, and then quickly move the mouse outside the video before it starts. The controls remain visible, until a mousein+mouseout to trigger the hiding.

I'm not sure if we considered this case, where (1) we intentionally have the controls showing  initially (so the user can interact with them) but (2) the page itself calls video.play() to start playback. So we should probably change the stock controls to hide themselves when they see the video start playing unexpectedly...

I don't _think_ the the page setting .controls (comment 6) is involved here. But I can't test because my Android is crashing on the page. :-( [filed bug 637961] This case should already be taken care of by layout/style/html.css...

  746  video:not([controls]) > xul|videocontrols,
  747  audio:not([controls]) > xul|videocontrols {
  748    visibility: hidden;
  749  }

I see that Fennec has a chrome/content/content.css, where the videocontrols.xml#touchControls binding is attached... Does Fennec not use mozilla-central's html.css? If so, it probably just needs those extra rules copied over to content.css?

I'm not sure if the other pieces mentioned here are just side effects from this or are actually separate bugs.

	Wesley Johnston (:wesj) Assignee
	Comment 11 • 14 years ago

You are right! This is a bit hacky, but adds a property, _fromControls, which is set to true when the "play" button is pressed. Then, when the play event fires we check for it. If it isn't set (i.e. a third party is starting the video, not our controls), we hide the controls.

This also fixes two other small bugs in the touch controls. The first is just an error in our detection for whether the controls are visible or not. The second is an error that occurs when you first tap a video in Fennec. This fires both a mouseover and a mouseup event on the video which causes it to be shown and then hidden quickly. Subsequent taps will only fire mouseup, unless you first tap off the video and then back on. I'm detecting mouseover and preventing us from doing anything on mouseup in that case.

	Wesley Johnston (:wesj) Assignee
	Comment 12 • 14 years ago

Ping again. I also put up a test page here:

http://dl.dropbox.com/u/72157/video.html

Click the play button, on desktop Firefox or Fennec, without ever hovering the video. Controls do not go away.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 13 • 14 years ago

wesj: does the patch in bug 620317 fix this bug?

	Wesley Johnston (:wesj) Assignee
	Comment 14 • 14 years ago

Doesn't seem to here.

	Doug Turner (:dougt)
	Comment 15 • 14 years ago

dolske, update on the review?

	Wesley Johnston (:wesj) Assignee
	Comment 16 • 13 years ago

dolske ping

	Wesley Johnston (:wesj) Assignee
	Comment 17 • 13 years ago

Comment on attachment 516388 [details] [diff] [review]
Patch v2

I'm pulling the review for a minute while I unbitrot/make sure this still works.

	Wesley Johnston (:wesj) Assignee
	Comment 18 • 13 years ago

Unbitrotted. I also decided on a better way to handle the interplay between mouseover and mouseup on mobile. Namely, ignoring it. I'm setting a flag in the videocontrols binding to disable mouseOver and mouseOut toggling the controls and altering the touch controls not to fire those events.

	Frank Yan (:fryn)
	Comment 19 • 13 years ago

Comment on attachment 533846 [details] [diff] [review]
Updated Patch

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

I'm not a toolkit peer, so I can't officially review this, but I'll provide some feedback.

I haven't actually tested the code. Please make sure it doesn't break on the desktop platforms, which use different CSS theming.

::: toolkit/content/widgets/videocontrols.xml
@@ +417,5 @@
>                              this.setPlayButtonState(false);
>                              this.setupStatusFader();
> +                            if (!this._fromControls) {
> +                                this.startFadeOut(this.controlBar);
> +                                this._fromControls = false;

if this._fromControls is already false, why set it to false again?

@@ +740,5 @@
>                      element.setAttribute("hidden", true);
>                  },
>  
> +                _fromControls: false,
> +

_fromControls is somewhat ambiguous. It could probably be better served with a name like _byControls, _triggeredByControls, or something more specific.

@@ +1115,4 @@
>      </implementation>
>  
>      <handlers>
> +        <handler event="mouseup"><![CDATA[

Why the addition of the CDATA wrapper? The body of the method wasn't changed, so I'm assuming it was valid XML before.

	Wesley Johnston (:wesj) Assignee
	Comment 20 • 13 years ago

Thanks for the feedback. Renamed the variable and cleaned up the other stuff. Gavin, do you mind reviewing this?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 21 • 13 years ago

Frank or Dolske should review this, they know the video controls stuff much better than I.

	Frank Yan (:fryn)
	Comment 22 • 13 years ago

Comment on attachment 534616 [details] [diff] [review]
Patch v3?

_triggeredByControls starts out as false, but once it's set to true, nothing ever sets it back to false, even though there could be other play() calls that are not triggered by the default controls. Shouldn't the code look like:

+ if (!this._triggeredByControls)
+   this.startFadeOut(this.controlBar);
+ this._triggeredByControls = false;

I'm happy to give feedback along the way, but Dolske is most familiar with all the videocontrols mousein/out interactions, so it's probably best if he reviews the final patch.

	Wesley Johnston (:wesj) Assignee
	Comment 23 • 13 years ago

Whoops! Happy to take feedback, especially when it comes from me deleting code without looking at it.

	Frank Yan (:fryn)
	Comment 24 • 13 years ago

Comment on attachment 534627 [details] [diff] [review]
patch v3.1

>          <handler event="mouseover">
> -            this.Utils.onMouseInOut(event);
> +            if (!this.disableMouseTransitions)
> +                this.Utils.onMouseInOut(event);
>          </handler>
>          <handler event="mouseout">
> -            this.Utils.onMouseInOut(event);
> +            if (!this.disableMouseTransitions)
> +                this.Utils.onMouseInOut(event);
>          </handler>

-              let event = document.createEvent("MouseEvents");
-              event.initEvent("mouseover", true, true);
-              this.Utils.videocontrols.dispatchEvent(event);
...
-              let event = document.createEvent("MouseEvents");
-              event.initEvent("mouseout", true, true);
-              this.Utils.videocontrols.dispatchEvent(event);

If so, the first block of code implies that the mousein and mouseout events are still fired when using the touch UI, but you removed the code that dispatches the mouseover and mouseout events in the second block, so I don't understand why all the |this.disableMouseTransitions| code is necessary.

Also, just to make sure I understand this bug correctly, this bug affects only the touch UI; yes?

	Wesley Johnston (:wesj) Assignee
	Comment 25 • 13 years ago

TouchUI only. I think I pointed out earlier, you can see "pause button doesn't hide" effect in Firefox by calling play on a video without ever hovering it, but that is separate from the things you are looking at here.

Tapping any element in Fennec fires (after a small delay to make sure it isn't a double tap) mousedown, mouseover and mouseup. Taps on the same element directly after the first will just fire mousedown and mouseup. To fire a mouseout, you have to tap off of the element.

In addition, if video controls are already showing, tapping on the video may fire a mouseover, but should hide the controls.

So the mousein and mouseout events are still fired, even without us dispatching them in this binding, but we want to ignore them here.

	Frank Yan (:fryn)
	Comment 26 • 13 years ago

Comment on attachment 534627 [details] [diff] [review]
patch v3.1

(In reply to comment #25)

Ah, ok. That makes sense.

I'm flagging this f+ simply based on the code.
I have not actually tested it.

Handing this over to dolske, but his review queue is likely long.

	Frank Yan (:fryn)
	Comment 27 • 13 years ago

Comment on attachment 534627 [details] [diff] [review]
patch v3.1

I just tested this patch on desktop (Windows 7) on top of m-c, and it causes the controls to disappear entirely when the media begins playing.

Here's the testcase I used:
https://bug502892.bugzilla.mozilla.org/attachment.cgi?id=522552

	Justin Dolske [:Dolske]
	Comment 28 • 13 years ago

Comment on attachment 534627 [details] [diff] [review]
patch v3.1

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

::: toolkit/content/widgets/videocontrols.xml
@@ +416,5 @@
>                          case "play":
>                              this.setPlayButtonState(false);
>                              this.setupStatusFader();
> +                            if (!this._triggeredByControls)
> +                                this.startFadeOut(this.controlBar);

This shouldn't happen for desktop controls. If you click play, the controls should stay until you mouseout. Otherwise they unexpectedly disappear on you.

Add a readonly isTouchControl <field> to your binding, and add a this.isTouchControl check here. That should suppress it for desktop and make it work for you.

Oh, I see you have a disableMouseTransitions field. Same idea, I'd just use the same isTouchControl for both.

@@ +1019,4 @@
>  
>          <constructor>
>            this.TouchUtils.init(this);
> +          this.disableMouseTransitions = true;

I think, but am not completely sure, that you can have
  <field name="disableMouseTransitions">true</field>
in your binding to override the <field> from the base (desktop) binding. If so, you should be able to set readonly="true" too.

@@ +1053,4 @@
>              },
>  
>              showControls : function() {
> +              this.Utils.startFadeIn(this.Utils.controlBar);

You'll probably want a dynamicControls() check here too, otherwise pressing play on an <audio> element makes it disappear.

	Justin Dolske [:Dolske]
	Comment 29 • 13 years ago

Comment on attachment 534627 [details] [diff] [review]
patch v3.1

Uhh, wtf? I set r- in Splinter, and it somehow transformed that to a f+!

	Justin Dolske [:Dolske]
	Comment 30 • 13 years ago

Oh, and probably worth a comment in the mouse <handlers> noting that we're doing this to ignore the events triggered by touch events.

	Wesley Johnston (:wesj) Assignee
	Comment 31 • 13 years ago

Nits addressed (I hope). Thanks guys!

Note that on Desktop we will (still) not hide controls if a page calls video.play() on a video element that is using our builtin controls. The user will have to mouseover, and mouseout for the contols to hide.

	Justin Dolske [:Dolske]
	Comment 32 • 13 years ago

Comment on attachment 534821 [details] [diff] [review]
Patch v3

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

::: toolkit/content/widgets/videocontrols.xml
@@ +749,2 @@
>                      else
>                          this.video.pause();

Nit: brace the else block too, since you're adding them. Single-line "} else {". :)

@@ +965,5 @@
>  
>      <handlers>
>          <handler event="mouseover">
> +            if (!this.isTouchControl)
> +                this.Utils.onMouseInOut(event);

Hmm. Are the isTouchControl checks in these <handlers> even needed now? You're removing the fake mouseover/mouseout events in this patch... Or do taps on Fennec still generate mouseover/mouseout events elsewhere?

	Wesley Johnston (:wesj) Assignee
	Comment 33 • 13 years ago

Yep. They fired automatically by the platform, and we occasionally fire them ourselves as well.

	Marco Bonardo [:mak]
	Comment 34 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/f6fd8c0cb882

	Andreea Pod
	Comment 35 • 13 years ago

Verified fixed on Aurora: Mozilla /5.0 (Android;Linux armv7l;rv:7.0a2) Gecko/20110721 Firefox/7.0a2 Fennec/7.0a2