Closed Bug 1352009 Opened 7 years ago Closed 7 years ago

Investigate whether we need to do anything to support larger aspect ratios

Categories

(Firefox for Android Graveyard :: General, defect, P1)

All
Android
defect

Tracking

(fennec+, firefox53+ wontfix, firefox54+ verified, firefox55+ verified)

VERIFIED FIXED
Firefox 55
Tracking Status
fennec + ---
firefox53 + wontfix
firefox54 + verified
firefox55 + verified

People

(Reporter: sebastian, Assigned: mkaply)

References

Details

Attachments

(3 files)

Google published this confusing blog post:
https://android-developers.googleblog.com/2017/03/update-your-app-to-take-advantage-of.html

From the posting it's not clear to me whether we should add something like this to our manifest:
<meta-data android:name="android.max_aspect" android:value="2.1" />

The posting also mentions "android:resizeableActivity" - but we can't set this without using the 24/25/26 SDK first. This is something we should set for multiscreen anyways - as soon as we can.
This is Firefox for Android on a Samsung Galaxy S8 with an aspect ratio of 18.5:9, in FHD+ resolution (2220x1080). The black bars at the top and bottom of the screen are there in both other resolutions supported by the device. Some other apps (such as Chrome) don't have those black bars.
Thanks!

Flagging for triage.
tracking-fennec: --- → ?
snorp: this is the friend I mentioned on IRC today.
Flags: needinfo?(snorp)
We added a max_aspect value of 2.0 in bug 1345014. Seems like we want to use an even higher value for now. And later enable resizeableActivity (after bug 1352015).
See Also: → 1345014
Yeah, it appears we just need to keep upping that value. Weird.
I wonder if we should just set this to some comical value like like 5. I think I'd rather see at worst a slightly-busted UI than these black bars.
Flags: needinfo?(snorp)
> I wonder if we should just set this to some comical value like like 5. I think I'd rather see at worst a slightly-busted UI than these black bars.

If it was going to bust the UI, it should bust at the low end to, right? Shouldn't we theoretically be able to set it to 5 and see how it looks on existing devices?
I just upgraded to Samsung's latest release and now there's a workaround: using the app switcher, I can force Firefox to full screen, which eliminates the black bars and looks great. The setting persists across app restarts.
(In reply to Mike Kaply [:mkaply] from comment #7)
> > I wonder if we should just set this to some comical value like like 5. I think I'd rather see at worst a slightly-busted UI than these black bars.
> 
> If it was going to bust the UI, it should bust at the low end to, right?
> Shouldn't we theoretically be able to set it to 5 and see how it looks on
> existing devices?

Yeah, let's do it. If we could we'd set resizeableActivity ("support whatever") anyways.
There's finally some public info about this:

https://android-developers.googleblog.com/2017/03/update-your-app-to-take-advantage-of.html

They are recommending 2.1.
And we should at least uplift this to 54. I originally didn't uplift because you had to change a user setting on the G6 to see the problem, but with it more common, we should at least get it to 54.
Comment on attachment 8860999 [details]
Bug 1352009 - Bump max_aspect_ratio per Android guidelines.

https://reviewboard.mozilla.org/r/133030/#review135786
Attachment #8860999 - Flags: review?(s.kaspari) → review+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/6bcf19f3164c
Bump max_aspect_ratio per Android guidelines. r=sebastian
https://hg.mozilla.org/mozilla-central/rev/6bcf19f3164c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
[triage]
per comment12 we're tagging 54+.
sebastian, could you request the uplift to 54?
tracking-fennec: ? → +
Priority: -- → P1
Assignee: nobody → mozilla
Comment on attachment 8860999 [details]
Bug 1352009 - Bump max_aspect_ratio per Android guidelines.

Approval Request Comment

[Feature/Bug causing the regression]: New phones with different aspect ratios have been released.

[User impact if declined]: On those phones Fennec will not scale to use all screen space (See attached screenshot).

[Is this code covered by automated tests?]: No.

[Has the fix been verified in Nightly?]: I do not have one of those phones.

[Needs manual test from QE? If yes, steps to reproduce]: Just launching Fennec on one of those phones.

[List of other uplifts needed for the feature/fix]: Just this one.

[Is the change risky?]: Very very low.

[Why is the change risky/not risky?]: We are only bumping the max supported aspect ratio in the manifest from 2.0 to 2.1

[String changes made/needed]: -
Attachment #8860999 - Flags: approval-mozilla-beta?
Attachment #8860999 - Flags: approval-mozilla-aurora?
Would it be worthwhile asking for release approval since we're doing a Fennec point release anyway? Especially given that the S8 just came out?
With the risk being super low I'd say yes. I'll request.
Comment on attachment 8860999 [details]
Bug 1352009 - Bump max_aspect_ratio per Android guidelines.

See comment 17. With the risk being so low we'd like to get it into the next dot release.
Attachment #8860999 - Flags: approval-mozilla-release?
I've verified that Nightly renders full-screen. Thank you all for taking care of this so quickly!
[Tracking Requested - why for this release]:
Track 54+/55+ for better user experience.
Comment on attachment 8860999 [details]
Bug 1352009 - Bump max_aspect_ratio per Android guidelines.

Fix an issue related to different aspect ratios and was verified. Beta54+. Should be in 54 beta 4.
Attachment #8860999 - Flags: approval-mozilla-beta?
Attachment #8860999 - Flags: approval-mozilla-beta+
Attachment #8860999 - Flags: approval-mozilla-aurora?
Attachment #8860999 - Flags: approval-mozilla-aurora-
needs rebasing for beta

grafting 413576:6bcf19f3164c "Bug 1352009 - Bump max_aspect_ratio per Android guidelines. r=sebastian"
merging mobile/android/base/AndroidManifest.xml.in
warning: conflicts while merging mobile/android/base/AndroidManifest.xml.in! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(mozilla)
This is running into conflicts because bug 1345014 only landed on m-c. Given that this patch is just updating the one line that patch added, it seems reasonable and fair to just land that patch first so this grafts cleanly onto it. Our other option would be to create a Beta patch in this bug that effectively does the exact same thing. From a blame standpoint, the former option seems better.
Flags: needinfo?(mozilla)
Depends on: 1345014
Might as well pull this change into 53.0.2 as well...
We have a bunch going on for 53.0.2 and I'd like to not complicate that build with any further risk so we can get it out the door. I'll keep it tracked in case we end up with yet another fennec dot release but otherwise it will go out with 54.
Let's leave this to ride with 54. I would love to have some verification of the fix in 54, before we get much later in beta.
Flags: qe-verify+
Flags: needinfo?(mihai.ninu)
Flags: needinfo?(ioana.chiorean)
Hi Liz,

Since I couldn't get access to a Samsung Galaxy S8 (yet), tried this issue on a Samsung Galaxy S7 EDGE (closest device to S8) (Android 7.0) and on a LG V20 (Android 7.0). 
No issues to report in both cases using 54.0b8 build and latest 55.0a1 build.
Flags: needinfo?(mihai.ninu)
Flags: needinfo?(ioana.chiorean)
Comment on attachment 8860999 [details]
Bug 1352009 - Bump max_aspect_ratio per Android guidelines.

As liz said in comment #30
Attachment #8860999 - Flags: approval-mozilla-release? → approval-mozilla-release-
Got my hands on a S8. Played around with it on this and works good. Both Nightly and Beta.
Status: RESOLVED → VERIFIED
Remove qe-verify flag based on comment 33.
Flags: qe-verify+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: