Last Comment Bug 627752 - pause button blocks part of html5 video
: pause button blocks part of html5 video
Status: VERIFIED FIXED
[inbound]
:
Product: Toolkit
Classification: Components
Component: Video/Audio Controls (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Wesley Johnston (:wesj)
:
:
Mentors:
http://www.mozilla.com/en-US/firefox/...
Depends on:
Blocks: 465839
  Show dependency treegraph
 
Reported: 2011-01-21 08:52 PST by Kevin Brosnan
Modified: 2013-11-12 00:56 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
7+


Attachments
Screenshot of the pause button blocking the video (86.95 KB, image/png)
2011-01-21 08:52 PST, Kevin Brosnan
no flags Details
Patch v1 (2.89 KB, patch)
2011-01-28 17:30 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch v1.1 (3.00 KB, patch)
2011-02-22 17:32 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch v2 (3.98 KB, patch)
2011-03-02 14:00 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Updated Patch (3.74 KB, patch)
2011-05-19 16:49 PDT, Wesley Johnston (:wesj)
fryn: feedback-
Details | Diff | Splinter Review
Patch v3? (3.26 KB, patch)
2011-05-23 16:25 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
patch v3.1 (3.32 KB, patch)
2011-05-23 16:50 PDT, Wesley Johnston (:wesj)
dolske: review-
Details | Diff | Splinter Review
Patch v3 (3.56 KB, patch)
2011-05-24 10:17 PDT, Wesley Johnston (:wesj)
dolske: review+
Details | Diff | Splinter Review

Description Kevin Brosnan 2011-01-21 08:52:22 PST
Created attachment 505817 [details]
Screenshot of the pause button blocking the video

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
Comment 1 Aakash Desai [:aakashd] 2011-01-25 10:47:20 PST
What would you propose as a solution here, Kevin?
Comment 2 Kevin Brosnan 2011-01-25 10:49:36 PST
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.
Comment 3 Matt Brubeck (:mbrubeck) 2011-01-25 10:49:55 PST
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.
Comment 4 Wesley Johnston (:wesj) 2011-01-25 10:53:16 PST
Hmm... Disappears after a few moments for me. I wonder if this is fallout from bug 618124. I'll take and look into it.
Comment 5 Wesley Johnston (:wesj) 2011-01-28 11:05:41 PST
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...
Comment 6 Wesley Johnston (:wesj) 2011-01-28 17:30:49 PST
Created attachment 508056 [details] [diff] [review]
Patch v1

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).
Comment 7 Wesley Johnston (:wesj) 2011-02-22 17:32:42 PST
Created attachment 514380 [details] [diff] [review]
Patch v1.1

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.
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2011-02-26 10:23:32 PST
Justin - Any ETA on a review for this? Fennec 4 final code freeze approaches...
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2011-03-01 13:04:17 PST
softblocker

dolske - review ping
Comment 10 Justin Dolske [:Dolske] 2011-03-01 18:42:58 PST
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.
Comment 11 Wesley Johnston (:wesj) 2011-03-02 14:00:13 PST
Created attachment 516388 [details] [diff] [review]
Patch v2

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.
Comment 12 Wesley Johnston (:wesj) 2011-03-04 10:56:12 PST
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.
Comment 13 Chris Pearce (:cpearce) 2011-03-13 13:45:15 PDT
wesj: does the patch in bug 620317 fix this bug?
Comment 14 Wesley Johnston (:wesj) 2011-03-13 16:45:45 PDT
Doesn't seem to here.
Comment 15 Doug Turner (:dougt) 2011-03-18 14:33:03 PDT
dolske, update on the review?
Comment 16 Wesley Johnston (:wesj) 2011-04-25 11:39:38 PDT
dolske ping
Comment 17 Wesley Johnston (:wesj) 2011-05-19 14:17:05 PDT
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.
Comment 18 Wesley Johnston (:wesj) 2011-05-19 16:49:33 PDT
Created attachment 533846 [details] [diff] [review]
Updated Patch

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.
Comment 19 Frank Yan (:fryn) 2011-05-19 21:52:15 PDT
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.
Comment 20 Wesley Johnston (:wesj) 2011-05-23 16:25:06 PDT
Created attachment 534616 [details] [diff] [review]
Patch v3?

Thanks for the feedback. Renamed the variable and cleaned up the other stuff. Gavin, do you mind reviewing this?
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-23 16:30:24 PDT
Frank or Dolske should review this, they know the video controls stuff much better than I.
Comment 22 Frank Yan (:fryn) 2011-05-23 16:41:58 PDT
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.
Comment 23 Wesley Johnston (:wesj) 2011-05-23 16:50:14 PDT
Created attachment 534627 [details] [diff] [review]
patch v3.1

Whoops! Happy to take feedback, especially when it comes from me deleting code without looking at it.
Comment 24 Frank Yan (:fryn) 2011-05-23 17:06:19 PDT
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?
Comment 25 Wesley Johnston (:wesj) 2011-05-23 17:20:27 PDT
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.
Comment 26 Frank Yan (:fryn) 2011-05-23 17:24:05 PDT
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.
Comment 27 Frank Yan (:fryn) 2011-05-23 20:45:14 PDT
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
Comment 28 Justin Dolske [:Dolske] 2011-05-23 20:52:13 PDT
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.
Comment 29 Justin Dolske [:Dolske] 2011-05-23 20:53:13 PDT
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+!
Comment 30 Justin Dolske [:Dolske] 2011-05-23 20:55:05 PDT
Oh, and probably worth a comment in the mouse <handlers> noting that we're doing this to ignore the events triggered by touch events.
Comment 31 Wesley Johnston (:wesj) 2011-05-24 10:17:44 PDT
Created attachment 534821 [details] [diff] [review]
Patch v3

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.
Comment 32 Justin Dolske [:Dolske] 2011-06-22 17:59:27 PDT
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?
Comment 33 Wesley Johnston (:wesj) 2011-06-24 14:53:35 PDT
Yep. They fired automatically by the platform, and we occasionally fire them ourselves as well.
Comment 34 Marco Bonardo [::mak] 2011-06-25 03:31:59 PDT
http://hg.mozilla.org/mozilla-central/rev/f6fd8c0cb882
Comment 35 Andreea Pod 2011-07-21 06:42:22 PDT
Verified fixed on Aurora: Mozilla /5.0 (Android;Linux armv7l;rv:7.0a2) Gecko/20110721 Firefox/7.0a2 Fennec/7.0a2

Note You need to log in before you can comment on or make changes to this bug.