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)
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)
723.81 KB,
image/png
|
Details | |
58 bytes,
text/x-review-board-request
|
Margaret
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details |
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
Assignee | ||
Comment 2•8 years ago
|
||
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)
Comment 3•8 years ago
|
||
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)
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
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
Comment 6•8 years ago
|
||
(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/
Assignee | ||
Comment 7•8 years ago
|
||
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
Assignee | ||
Comment 8•8 years ago
|
||
sorry, correct apk's links: - good build is [2] - bad build is [3]
Comment 9•8 years ago
|
||
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
Assignee | ||
Comment 10•8 years ago
|
||
(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)
Comment 11•8 years ago
|
||
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)
Comment 12•8 years ago
|
||
(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)
Updated•8 years ago
|
Flags: needinfo?(dtownsend)
Comment 13•8 years ago
|
||
(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)
Assignee | ||
Comment 14•8 years ago
|
||
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)
Comment 15•8 years ago
|
||
(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!
Assignee | ||
Comment 16•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51645/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51645/
Attachment #8750806 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 17•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51647/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51647/
Assignee | ||
Comment 18•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8750806 -
Attachment is obsolete: true
Attachment #8750806 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 19•8 years ago
|
||
Sorry I was so hurry and misplaced the patch, revision 2 would be the right one. Thanks.
Updated•8 years ago
|
Attachment #8750805 -
Flags: review?(margaret.leibovic) → review+
Comment 20•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 21•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/07e3e85b5ce6
Keywords: checkin-needed
Comment 22•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/07e3e85b5ce6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
tracking-fennec: ? → 48+
Assignee | ||
Comment 23•8 years ago
|
||
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?
Comment 24•8 years ago
|
||
Looks like 49 was affected too.
Comment 25•8 years ago
|
||
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+
Comment 26•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/fd75a1f393fe
Comment 27•8 years ago
|
||
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)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•