Closed Bug 1267935 Opened 8 years ago Closed 8 years ago

Video controls are not correctly scaled for several video types

Categories

(Firefox for Android Graveyard :: Audio/Video, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox46 unaffected, firefox47 unaffected, firefox48 verified, firefox49 verified, fennec48+)

VERIFIED FIXED
Firefox 49
Tracking Status
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 --- verified
firefox49 --- verified
fennec 48+ ---

People

(Reporter: u549602, Assigned: ralin)

References

Details

Attachments

(2 files, 1 obsolete file)

Environment: Aurora
Device: Xiaomi mi i4 (Android 5.1.1 );
Build: Aurora 48.0a2 (2016-04-26);

Steps to reproduce:
1. Go to youtube and play a random video
2. Trigger the video control display 

Expected result:
Video controls are correctly scaled to the video width

Actual result:
Video controls menu is not correctly scaled

Notes:
Please note that this issue also occurs for the  VP9/WEBM, mp4 video types
This issue is not reproducible on the latest nightly build, just after the nightly uplift to aurora
Ray, can you look into this?
Assignee: nobody → ralin
Flags: needinfo?(ralin)
No problem, I'd like to.

* The issue occurs on my Z3C and LG G2 as well. 
* The differences between Aurora and Nightly is only a commit, but change nothing related to layouts. 

I don't know if there's a good approach to inspect element in video tag, but the border of content can be present by long-pressed on element sometimes... so I found the content of the button seems to be a rectangle rather than square though I set 48 * 48px size and fixed padding around. Therefore, I suspect the issue is caused by different rendering or graphic supports between nightly and aurora build system. I didn't come up with a good direction to go on this kind of issue so far, could you give me some suggestion? Many thanks :-)
Flags: needinfo?(ralin) → needinfo?(margaret.leibovic)
I'm not sure of the best way to debug this... esawin, are you a good person to ask about media questions?
Flags: needinfo?(margaret.leibovic) → needinfo?(esawin)
Debugging media playback works relatively well by enabling the corresponding PR logs, but this isn't an issue with the media stack.

I'm not familiar with the media controls overlay code, so I can't really give useful directions here.

I can't reproduce it on 49 and Aurora is currently stuck on 47, what's the regression range for this?
Flags: needinfo?(esawin)
I bet this is a regression from bug 1250741.

Given that this sounds like a problem with the XBL video controls, I think the best tool to debug is the remote debugger.

Ray, have you tried using the remote inspector to inspect the anonymous content?

https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging/Debugging_Firefox_for_Android_with_WebIDE
(In reply to Eugen Sawin [:esawin] from comment #4)

> I can't reproduce it on 49 and Aurora is currently stuck on 47, what's the
> regression range for this?

You should also be able to download 48 builds direct from the FTP server:
http://ftp.mozilla.org/pub/mobile/tinderbox-builds/mozilla-aurora-android-api-15/
Thank you Margaret!

I've tried remote inspector, but unfortunately still not able to select anonymous elements. However, I think I narrowed down the possible regression range with Tim's help.

Steps to find regression:
1. clone mozilla aurora repo
2. mozregression -> found the first bad build is [1], but could not find any good build with new UI by mozregression
4. checkout to [1], build apk manually and confirmed as a bad build[2] on mobile phone. (not artifact install)
5. try several changeset and found [0] is a good build[3].

The changesets between [0] to [1] are mostly related to tag updating. I think [1] is probably the highest chance to make regression, though it's weird and doesn't make sense. 
To make sure it's not my environment's factors to lead this kind of result, if you have time, you could build these two changesets and help me confirm it. Thanks.

I'll go deep into the flags and try to figure out the reason.


[0] https://hg.mozilla.org/releases/mozilla-aurora/rev/FIREFOX_AURORA_48_BASE
[1] https://hg.mozilla.org/releases/mozilla-aurora/rev/0d6a91c76a9e
[2] 48.0a1-1c6385ae1fe7.apk: https://drive.google.com/a/mozilla.com/file/d/0B6XCugEDFBX8QTRiX0M0RWNqbWs/view?usp=sharing
[3] 48.0a2-0d6a91c76a9e.apk: https://drive.google.com/a/mozilla.com/file/d/0B6XCugEDFBX8UWVaa1d3UDVkM3M/view?usp=sharing
sorry, correct apk's links:
- good build is [2]
- bad build is [3]
I'm not certain it's relevant, but I notice that the "/toolkit/themes/mobile" directory is part of the build only if NIGHTLY_BUILD is defined:

https://hg.mozilla.org/releases/mozilla-aurora/file/b6738ca6/toolkit/themes/moz.build#l26

This directory contains some resources that are referenced by /mobile/android/themes/core/touchcontrols.css:

https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/mobile/global/media
(In reply to Matt Brubeck (:mbrubeck) from comment #9)
> I'm not certain it's relevant, but I notice that the
> "/toolkit/themes/mobile" directory is part of the build only if
> NIGHTLY_BUILD is defined:

Thanks you so much, Matt!  It is key point of the differences.

Margaret, I think it could be solved by removing the "NIGHTLY_BUILD" condition and let aurora use mobile theme. Is there any concerns we use window theme instead of mobile on non nightly branches? 

Thanks.
Flags: needinfo?(margaret.leibovic)
That's weird, I see that was introduced in bug 1227997. Did this cause regressions on earlier versions of Fennec as well? How are these styles working at all on 45 and up?

If we're going to fix this by removing the NIGHTLY_BUILD condition, we should make sure the theme is used for all versions, not just Nightly and Aurora (otherwise, we'll have the same problem when this gets to Beta).

mfinkle or Mossop, can you give us more context here?
Flags: needinfo?(mark.finkle)
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(dtownsend)
(In reply to Ray Lin[:ralin] from comment #10)

> Margaret, I think it could be solved by removing the "NIGHTLY_BUILD"
> condition and let aurora use mobile theme. Is there any concerns we use
> window theme instead of mobile on non nightly branches? 

Yes. We should have shipped the mobile theme last year. Obviously a major oversight that it's still behind a NIGHTLY flag. The flag should be removed and uplifted as far as possible.
Flags: needinfo?(mark.finkle)
Flags: needinfo?(dtownsend)
(In reply to Mark Finkle (:mfinkle) from comment #12)
> (In reply to Ray Lin[:ralin] from comment #10)
> 
> > Margaret, I think it could be solved by removing the "NIGHTLY_BUILD"
> > condition and let aurora use mobile theme. Is there any concerns we use
> > window theme instead of mobile on non nightly branches? 
> 
> Yes. We should have shipped the mobile theme last year. Obviously a major
> oversight that it's still behind a NIGHTLY flag. The flag should be removed
> and uplifted as far as possible.

Okay, let's get a patch for this.

However, I tested the video controls on beta/release, and they don't seem to be broken at all, so I'm having a hard time believing bug 1227997 would have caused this specific problem only on aurora.

Ray, have you tried changing this build file to see that this fixes the issue?
Flags: needinfo?(ralin)
Yes, I've tried on latest code base with two builds: one without change and one removed the condition, the result shows that the one removed the condition is fixed.

Now I'm building again to reconfirm it. However, I spent a morning comparing possible differences, but nothing was found so far.
Flags: needinfo?(ralin)
(In reply to Ray Lin[:ralin] from comment #14)
> Yes, I've tried on latest code base with two builds: one without change and
> one removed the condition, the result shows that the one removed the
> condition is fixed.
> 
> Now I'm building again to reconfirm it. However, I spent a morning comparing
> possible differences, but nothing was found so far.

Okay, well I'm happy to r+ a change to get rid of this NIGHTLY flag. If that fixes the problem, we don't need to do too much more investigation.

Thank you mbrubeck for helping us figure this out!
Comment on attachment 8750805 [details]
MozReview Request: Bug 1267935 - Remove NIGHTLY_BUILD condition from mobile theme. r?margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51645/diff/1-2/
Attachment #8750805 - Attachment description: MozReview Request: tmp → MozReview Request: Bug 1267935 - Remove NIGHTLY_BUILD condition from mobile theme. r?margaret
Attachment #8750805 - Flags: review?(margaret.leibovic)
Attachment #8750806 - Attachment is obsolete: true
Attachment #8750806 - Flags: review?(margaret.leibovic)
Sorry I was so hurry and misplaced the patch, revision 2 would be the right one. Thanks.
Attachment #8750805 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8750805 [details]
MozReview Request: Bug 1267935 - Remove NIGHTLY_BUILD condition from mobile theme. r?margaret

https://reviewboard.mozilla.org/r/51645/#review48555

Thanks for the good find. Let's request uplift to aurora and beta as well, since this will help us save on APK size.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/07e3e85b5ce6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
tracking-fennec: ? → 48+
Comment on attachment 8750805 [details]
MozReview Request: Bug 1267935 - Remove NIGHTLY_BUILD condition from mobile theme. r?margaret

Approval Request Comment
[Feature/regressing bug #]: Bug 1250741

[User impact if declined]: As the screenshot shown in Bug 1267935, user got a incorrect video control layout

[Describe test coverage new/current, TreeHerder]: landed 6 days ago, no new error related to this patch.

[Risks and why]: low. This patch remove the build condition and make fennec build with mobile theme on all release instead of only on nightly build. Fennec aurora is supposed to work good with this config since the previous patches are tested in mobile theme before uplift.

[String/UUID change made/needed]: none
Attachment #8750805 - Flags: approval-mozilla-aurora?
Looks like 49 was affected too.
Comment on attachment 8750805 [details]
MozReview Request: Bug 1267935 - Remove NIGHTLY_BUILD condition from mobile theme. r?margaret

improve the video, taking it.
Attachment #8750805 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in build:
- Aurora 48.0a2 (2016-05-29);
- Nightly 49.0a1 (2016-05-29);
Device: Nexus 5 (Android 6.0.1) and Asus ZenPad 8 (Android 5.0.2)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.