Closed Bug 627752 Opened 13 years ago Closed 13 years ago

pause button blocks part of html5 video


(Toolkit :: Video/Audio Controls, defect)

Not set



Tracking Status
firefox7 --- fixed
fennec 7+ ---


(Reporter: kbrosnan, Assigned: wesj)




(Whiteboard: [inbound])


(2 files, 6 obsolete files)

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

Build ID:
build date: 2011-01-21
What would you propose as a solution here, Kevin?
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.
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
Blocks: 465839
tracking-fennec: --- → ?
OS: Android → All
Hardware: ARM → All
Hmm... Disappears after a few moments for me. I wonder if this is fallout from bug 618124. I'll take and look into it.
Assignee: nobody → wjohnston
tracking-fennec: ? → 2.0+
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...
Attached patch Patch v1 (obsolete) — Splinter Review
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).
Attachment #508056 - Flags: review?(dolske)
Attached patch Patch v1.1 (obsolete) — Splinter Review
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.
Attachment #508056 - Attachment is obsolete: true
Attachment #514380 - Flags: review?(dolske)
Attachment #508056 - Flags: review?(dolske)
Whiteboard: [needs-review (dolske)]
Justin - Any ETA on a review for this? Fennec 4 final code freeze approaches...

dolske - review ping
Whiteboard: [needs-review (dolske)] → [needs-review (dolske)] [fennec-softblocker]
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 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.
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #514380 - Attachment is obsolete: true
Attachment #516388 - Flags: review?(dolske)
Attachment #514380 - Flags: review?(dolske)
Ping again. I also put up a test page here:

Click the play button, on desktop Firefox or Fennec, without ever hovering the video. Controls do not go away.
tracking-fennec: 2.0+ → 2.0-
Component: General → Video/Audio Controls
Product: Fennec → Toolkit
QA Contact: general →
wesj: does the patch in bug 620317 fix this bug?
Doesn't seem to here.
dolske, update on the review?
Whiteboard: [needs-review (dolske)] [fennec-softblocker] → [needs-review (dolske)][fennec-4.1?]
dolske ping
tracking-fennec: - → 6+
Whiteboard: [needs-review (dolske)][fennec-4.1?] → [needs-review (dolske)]
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.
Attachment #516388 - Flags: review?(dolske)
Attached patch Updated Patch (obsolete) — Splinter Review
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.
Attachment #516388 - Attachment is obsolete: true
Attachment #533846 - Flags: review?(fryn)
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.
Attachment #533846 - Flags: review?(fryn) → feedback-
Whiteboard: [needs-review (dolske)]
Attached patch Patch v3? (obsolete) — Splinter Review
Thanks for the feedback. Renamed the variable and cleaned up the other stuff. Gavin, do you mind reviewing this?
Attachment #534616 - Flags: review?(
Frank or Dolske should review this, they know the video controls stuff much better than I.
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.
Attachment #534616 - Flags: review?(
Attached patch patch v3.1 (obsolete) — Splinter Review
Whoops! Happy to take feedback, especially when it comes from me deleting code without looking at it.
Attachment #534616 - Attachment is obsolete: true
Attachment #534627 - Flags: feedback?(fryn)
Attachment #533846 - Attachment is obsolete: true
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?
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 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.
Attachment #534627 - Flags: review?(dolske)
Attachment #534627 - Flags: feedback?(fryn)
Attachment #534627 - Flags: feedback+
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:
Attachment #534627 - Flags: review?(dolske)
Attachment #534627 - Flags: feedback-
Attachment #534627 - Flags: feedback+
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.
Attachment #534627 - Flags: feedback- → feedback+
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+!
Attachment #534627 - Flags: feedback+ → review-
Oh, and probably worth a comment in the mouse <handlers> noting that we're doing this to ignore the events triggered by touch events.
Attached patch Patch v3Splinter Review
Nits addressed (I hope). Thanks guys!

Note that on Desktop we will (still) not hide controls if a page calls on a video element that is using our builtin controls. The user will have to mouseover, and mouseout for the contols to hide.
Attachment #534627 - Attachment is obsolete: true
Attachment #534821 - Flags: review?(dolske)
Comment on attachment 534821 [details] [diff] [review]
Patch v3

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

::: toolkit/content/widgets/videocontrols.xml
@@ +749,2 @@
>                      else
>                ;

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?
Attachment #534821 - Flags: review?(dolske) → review+
Yep. They fired automatically by the platform, and we occasionally fire them ourselves as well.
Whiteboard: [inbound]
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
tracking-fennec: 6+ → 7+
Verified fixed on Aurora: Mozilla /5.0 (Android;Linux armv7l;rv:7.0a2) Gecko/20110721 Firefox/7.0a2 Fennec/7.0a2
You need to log in before you can comment on or make changes to this bug.