1005031 - Video controls are displayed in the middle of the video

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Cristina Madaras, QA [:CristinaM]]

Environment:
Build: Nightly (2014-05-01)
Device: Asus Transformer Pad TF300T (Android 4.2.1)

Steps to reproduce:
1. Start Fennec and go to http://people.mozilla.com/~nhirata/html_tp/elephants-dream.webm
2. Tap once on the video

Expected results:
The video control are displayed at the bottom part of the video.

Actual results:
The video controls are displayed in the middle of the video.

Comment 1

[Cristina Madaras, QA [:CristinaM]]

Regression window:
Last good revision: 7fe3ee0cf8be (2014-04-18)
First good revision: 582b2d81ebe1 (2014-04-19)

Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7fe3ee0cf8be&tochange=582b2d81ebe1

Comment 2

[Aaron Train [:aaronmt]]

Can you check on fxteam builds? http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/fx-team-android/

Screenshot?

Maybe:

d0849463abf6	Danny Chen — Bug 996122 - Play button superimposed on videos that are requested to start playing and then resize before any frames are presented. r=jaws

Comment 3

[Cristina Madaras, QA [:CristinaM]]

Attachment: video_controls.png

Comment 4

[Cristina Madaras, QA [:CristinaM]]

I checked fxteam builds:
Good: 1397846067
Bad: 1397855367

Comment 5

[Cristina Madaras, QA [:CristinaM]]

(In reply to cristina.madaras from comment #4)
> I checked fxteam builds:
> Good: 1397846067
> Bad: 1397855367

Pushlog: http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=f2e5bc417f96&tochange=d0849463abf6

Comment 6

[Aaron Train [:aaronmt]]

Thanks, so it's either these two.

[e-mail]
Fri Apr 18 21:08:47 2014 +0000	d0849463abf6	Danny Chen — Bug 996122 - Play button superimposed on videos that are requested to start playing and then resize before any frames are presented. r=jaws

f8118ec22ed9	Danny Chen — Bug 495593 - Video controls should match scaled video size within video element. r=jaws

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Most likely it's the second changeset that you have listed here.

Danny, are you able to investigate this on Android? I'm not sure if you have the hardware + build environment available.

Comment 8

[Danny Chen [:DChen]]

I don't have a recent Android device. Can Fennec be run on an emulator?

I've been building on Windows as well so I think I would need to setup a Linux VM.

Comment 9

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

Nick - Can you assist with getting Fennec running in an emulator?

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #9)
> Nick - Can you assist with getting Fennec running in an emulator?

I've actually only run Fennec in an emulator (successfully, that is) with an x86 emulator.  But if that will demonstrate the issue, I can certainly help with that part.  There are two things for this:

1) building Fennec for x86 architecture; (or downloading it, if that is enough)
2) preparing the x86 emulator.

For 1), I use my regular mozconfig, a different OBJDIR, and the single additional flag

ac_add_options --target=i686-linux-android

At various times, we've had problems with debug builds so I always build optimized (do not include --disable-optimize).

For 2), I suggest our very on gbrown's excellent instructions at http://gbrownmozilla.wordpress.com/2012/11/02/running-firefox-for-android-in-the-android-x86-emulator/

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

We should back out the patch from bug 495593 in the meantime since this bug doesn't look like it's getting any movement.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Jared, indeed. Can you take care of that?

Comment 14

[Danny Chen [:DChen]]

Attachment: Patch 1

Sorry about that. Had some difficulties getting my new build environment running.

I added '-moz-box-pack: end' to re-align the Fennec video controls to the bottom. This change doesn't appear to modify desktop Firefox as the controls overlay fills the entire height.

Comment 15

[Danny Chen [:DChen]]

Err, nevermind. I forgot I changed something else in the VM version of the patch. Is there a way to remove that?

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

The offending patch has been backed out of Firefox 31 now. Adjusting tracking accordingly.

Comment 17

[No-Jun Park [:njpark]]

FYI, this is also reproducible in FxOS:

│ Gaia      f3b5d74dd3428c89cab06db734c62f3c9dbb8c4d                         │
│ Gecko     https://hg.mozilla.org/mozilla-central/rev/e86a0d92d174          │
│ BuildID   20140526040203                                                   │
│ Version   32.0a1

Comment 18

[Danny Chen [:DChen]]

Attachment: Alignment patch v1

The cause was touch controls being oriented horizontally within controlsOverlay while the desktop version is vertical.

I have a separate patch for adjusting Bug 495593 to Android. Not sure where it would go.

Comment 19

[Danny Chen [:DChen]]

Attachment: Alignment patch v2

Missed a bracket while trying to split the patches into two.

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 8432174 [details] [diff] [review]
Alignment patch v2

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

I'd probably add a comment explaining why this is needed since its not obvious from the other styles in this file.

Comment 21

[Danny Chen [:DChen]]

Attachment: Patch v2 with comments

Comment 22

[Wesley Johnston (:wesj)]

Comment on attachment 8436270 [details] [diff] [review]
Patch v2 with comments

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

Looks good. Thanks :)

Comment 23

[Wesley Johnston (:wesj)]

Comment on attachment 8436270 [details] [diff] [review]
Patch v2 with comments

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

I'm fine r+'ing this too.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Danny, do you have access to tryserver? We should push this patch to tryserver before landing it. If you don't already have access, then let's get you access :)

You can file a bug similar to https://bugzilla.mozilla.org/show_bug.cgi?id=664170 to get tryserver access. See https://www.mozilla.org/hacking/commit-access-policy/ for more info.

Comment 25

[Danny Chen [:DChen]]

I have level 1 commit access now. Which test suites should I target for this component under http://trychooser.pub.build.mozilla.org/ ?

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

mochitest-5 is the test suite that contains toolkit/content/tests/widgets

Comment 27

[Danny Chen [:DChen]]

Attachment: Patch v2 with comments and +r in commit message

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

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Thanks! Marking checkin-needed accordingly.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1c4ff7de9e

Comment 30

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

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

Comment 31

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in:
Build: Firefox for Android 33.0a2 (2014-06-30)
Device: Asus Transformer Pad TF300T (Android 4.2.1) and Nexus 4 (Android 4.4.2)

Comment 32

[William Hsu [:whsu]]

Thanks everyone!
Verified it on FxOS V2.1.
Attach the screenshot (2014-07-04-20-23-49.png)

* Build information:
 - Gaia      b597b86274ab109d7ef530bc0c4b6adccddae4e4
 - Gecko     https://hg.mozilla.org/mozilla-central/rev/e8df6826a571
 - BuildID   20140704040205
 - Version   33.0a1

Comment 33

[William Hsu [:whsu]]

Attachment: 2014-07-04-20-23-49.png

Comment 34

[Kevin Brosnan [Ex-Mozilla]]

Please ask for (32) Beta approval on this patch.

Comment 35

[Danny Chen [:DChen]]

(In reply to Kevin Brosnan [:kbrosnan] from comment #34)
> Please ask for (32) Beta approval on this patch.

How do I do that?

Comment 36

[Kevin Brosnan [Ex-Mozilla]]

Details link of the patch you attached. approval-mozilla-beta flag

Comment 37

[Danny Chen [:DChen]]

Comment on attachment 8443017 [details] [diff] [review]
Patch v2 with comments and +r in commit message

Approval Request Comment
[Feature/regressing bug #]: patch to fix regression caused by bug 495593
[User impact if declined]: none, bug 495593 has been backed out
[Describe test coverage new/current, TBPL]: using current mochitest-5 suite
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 38

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8443017 [details] [diff] [review]
Patch v2 with comments and +r in commit message

(In reply to Danny Chen [:DChen] from comment #37)
> [Feature/regressing bug #]: patch to fix regression caused by bug 495593

As stated below, bug 495593 was backed out. Is this really the source of the regression?

> [User impact if declined]: none, bug 495593 has been backed out

The user impact is that video controls will display in the middle of the video instead of at the bottom.

Beta+

Comment 39

[Kevin Brosnan [Ex-Mozilla]]

I believe the backout was for 31. I can reproduce this issue in 32.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5a6749df6a78

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/5a6749df6a78

Comment 42

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in:
Build: Firefox for Android 32 Beta 2
Device: Asus Transformer Pad TF300T (Android 4.2.1)

Comment 43

[Paladin]

Attachment: Verify_image.png

This issue has been verified successfully on Flame 2.0,2.1

See attachment: Verify_image.png
Reproducing rate: 0/5
Flame 2.1 build:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0
FLame 2.0 build:
Gaia-Rev        f9d6e3d83c3922e9399a6c27f5ce4cdd27bdfd05
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/45112935086f
Build-ID        20141126000203
Version         32.0

Comment 44

[Cristina Madaras, QA [:CristinaM]]

I will mark this as Verified fixed based on comment 43.