Closed Bug 708772 Opened 13 years ago Closed 13 years ago

XUL Fennec uses non-tablet Gingerbread theme/UI on tablets running Android 4.0 (Ice Cream Sandwich)

Categories

(Firefox for Android Graveyard :: General, defect)

Firefox 9
All
Android
defect
Not set
normal

Tracking

(firefox8 unaffected, firefox9+ wontfix, firefox10+ fixed, firefox11 fixed)

RESOLVED FIXED
Firefox 11
Tracking Status
firefox8 --- unaffected
firefox9 + wontfix
firefox10 + fixed
firefox11 --- fixed

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

Attachments

(4 files, 7 obsolete files)

5.32 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
4.75 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
5.30 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
14.82 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
Currently the XUL Fennec "honeycomb" theme and tablet layout are enabled only on Android 3.x (Honeycomb) devices.  On devices running Android 4.x, the "gingerbread" theme and non-tablet UI are used.  See bug 704693 and bug 705026 for background.

This means that XUL Fennec's will use the smartphone UI on any tablet that ships with Android 4.0 or higher.  Also, the UI will suddenly regress from tablet to smartphone on tablets that are upgraded from 3.x to 4.x.

Our long-term plan is to stop shipping XUL Fennec and replace it with Native Fennec, but in the short term we plan to keep shipping XUL Fennec to large-screen tablet devices (until Native Fennec has a comparable tablet UI).  We need at least a temporary solution to make XUL Fennec work right on Android 4 tablets, in case they ship before our native tablet UI is ready.
This is basically extracted from wesj's attachment 572933 [details] [diff] [review] from bug 671634.
Attachment #580172 - Flags: review?(doug.turner)
This lets us switch themes and layout based on the screen size.

This is not an ideal solution.  The right way to do this would be a single skin that uses different styles dynamically based on JavaScript or CSS media queries.  But changing from our current manifest-based theme switching to something more dynamic is too big and risky to land in beta, and not worth it for a product that we plan to stop shipping soon.

I'd like to ship this as a short-term hack, and if desired we can file a bug to remove it after we stop shipping XUL fennec on Android.  The code is all inside "#if ANDROID" blocks, so it will not impact other platforms.
Attachment #580174 - Flags: review?(dtownsend)
Use both honeycomb theme and tablet layout on tablet-sized devices running Android 3.0 and up; gingerbread or froyo theme on all other devices.

The changes to Util.isTablet and themes/core/jar.mn ensure that the Android devices where we default to tablet layout are exactly the same as the Android devices where we default to the honeycomb theme.
Attachment #580175 - Flags: review?(mark.finkle)
Added a missing #ifdef.
Attachment #580174 - Attachment is obsolete: true
Attachment #580174 - Flags: review?(dtownsend)
Attachment #580177 - Flags: review?(dtownsend)
Comment on attachment 580177 [details] [diff] [review]
(2/3) Add tablet flag to jar manifest parser and nsSystemInfo

Having a flag that is only valid for android seems like a mistake to me, but this is bsmedberg's call.
Attachment #580177 - Flags: review?(dtownsend) → review?(benjamin)
(In reply to Dave Townsend (:Mossop) from comment #5)
> Having a flag that is only valid for android seems like a mistake to me, but
> this is bsmedberg's call.

As I said in comment 2, I regard this as a temporary hack and am proposing it only because I don't think we can justify the time/risk to do this the right way.  I'm open to any alternative suggestions.  Should we change the name to something like "tablet-HACK-DO-NOT-USE"?
Comment on attachment 580172 [details] [diff] [review]
(1/3) Add IsTablet method to AndroidBridge

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

::: embedding/android/GeckoAppShell.java
@@ +1696,5 @@
>          GeckoSmsManager.send(aNumber, aMessage);
>      }
> +
> +    public static boolean isTablet() {
> +        if (android.os.Build.VERSION.SDK_INT >= 9) {

Chris Peterson has been using Build.VERSION_CODES.GINGERBREAD instead of 9.  Can we do that here?
Attachment #580172 - Flags: review?(doug.turner) → review+
Comment on attachment 580175 [details] [diff] [review]
(3/3) Use honeycomb theme and tablet layout on ICS tablets

This seems OK as a (hopefully) short term solution. I might like displaysize={xhdpi|hdpi|...} better than tablet={1|0}, but this is a short term fix.
Attachment #580175 - Flags: review?(mark.finkle) → review+
Comment on attachment 580177 [details] [diff] [review]
(2/3) Add tablet flag to jar manifest parser and nsSystemInfo

I can't figure out why this is a string flag instead of a regular flag. Please make it a regular flag (with a bool).
Attachment #580177 - Flags: review?(benjamin) → review-
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> I can't figure out why this is a string flag instead of a regular flag.
> Please make it a regular flag (with a bool).

Fixed.
Attachment #581037 - Flags: review?(benjamin)
Oops, I uploaded the wrong file before.
Attachment #580177 - Attachment is obsolete: true
Attachment #581037 - Attachment is obsolete: true
Attachment #581037 - Flags: review?(benjamin)
Attachment #581089 - Flags: review?(benjamin)
Fixed an uninitialized variable warning.
Attachment #581089 - Attachment is obsolete: true
Attachment #581089 - Flags: review?(benjamin)
Attachment #581096 - Flags: review?(benjamin)
Attachment #581096 - Attachment description: patch → (2/3) Add tablet flag to jar manifest parser and nsSystemInfo
Attachment #581096 - Flags: review?(benjamin) → review+
The previous patch had a problem on non-XLARGE honeycomb devices like the HTC Flyer.  It used the gingerbread theme / phone layout, which doesn't match the OS theme -- and more importantly, does not have any way to access the app menu.

This updates the patch to always use the honeycomb/tablet UI on all honeycomb devices.  It switches based on screen size on Ice Cream Sandwich devices only.  (We still show the legacy menu button on ICS, so menu access is not an issue.)

Here's a build with the updated patch.  I haven't yet tested this on Ice Cream Sandwich.  I'll try to get it running in the Android 4 emulator, but I'd appreciate it if someone can try it on a real ICS phone too:
http://people.mozilla.com/~mbrubeck/fennec-708772-b.apk
Attachment #580175 - Attachment is obsolete: true
Attachment #581714 - Flags: review?(mark.finkle)
Forgot to qref.  (Man, I am having trouble uploading the right patches to this bug.)
Attachment #581714 - Attachment is obsolete: true
Attachment #581714 - Flags: review?(mark.finkle)
Attachment #581748 - Flags: review?(mark.finkle)
Added commit message and addressed review nit.  Carrying r=dougt.
Attachment #580172 - Attachment is obsolete: true
Attachment #581761 - Flags: review+
Attachment #581748 - Flags: review?(mark.finkle) → review+
[This is a combined diff that rolls up all three patches from this bug, to simplify the approval process.]

Requesting approval for Aurora 10.  This fixes a bug that will cause Fennec to regress to its non-tablet smartphone UI on any tablet device with Android 4.0 or higher installed.  We want this for Firefox 10 because there is a risk that tablet devices might ship with (or get updated to) Android 4 in the Firefox 10 time-frame.

(It would have been nice to get this into Firefox 9 just in case, but I don't think it's worth delaying the build.  I'm expecting that Firefox 10 will at least be in the beta channel by the time Android 4 ships to tablet users, so if necessary we can just tell affected users to switch to beta.)

The patch is mobile-only (every line is in a fennec-only file or an "ifdef ANDROID" block).  We've tested it on Android 2.x, 3.x, and 4.x devices.  The patch is fairly small and straightforward, but not zero-risk.  If there are any bugs then the main regression risk is that Fennec will start using a different theme than intended on some device that we haven't tested.
Attachment #581781 - Flags: review+
Attachment #581781 - Flags: approval-mozilla-aurora?
I forgot to merge the Java part from /embedding/android to /mobile/android/base.  Fixed in this followup push:
https://hg.mozilla.org/mozilla-central/rev/f8ace2116988

That followup is not needed on Aurora, because /mobile/android does not exist in Aurora.
Comment on attachment 581781 [details] [diff] [review]
rolled-up patch for Aurora

[Triage Comment]
Approved for Aurora. Please land on Monday 12/19 or earlier in order to make the cut-over to Beta.

CC'ing Jaclyn and Patrick so that they're aware.
Attachment #581781 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: