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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

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.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: (1/3) Add IsTablet method to AndroidBridge

This is basically extracted from wesj's attachment 572933 [details] [diff] [review] from bug 671634.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: (2/3) Add tablet flag to jar manifest parser and nsSystemInfo

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.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: (3/3) Use honeycomb theme and tablet layout on ICS tablets

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.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: (2/3) Add tablet flag to jar manifest parser and nsSystemInfo

Added a missing #ifdef.

Comment 5

[Dave Townsend [:mossop]]

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.

Comment 6

[Matt Brubeck (:mbrubeck)]

(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 7

[Doug Turner (:dougt)]

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?

Comment 8

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

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.

Comment 9

[Benjamin Smedberg]

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).

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: (2/3) Add tablet flag to jar manifest parser and nsSystemInfo

(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.

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: (2/3) Add tablet flag to jar manifest parser and nsSystemInfo

Oops, I uploaded the wrong file before.

Comment 12

[Matt Brubeck (:mbrubeck)]

Attachment: (2/3) Add tablet flag to jar manifest parser and nsSystemInfo

Fixed an uninitialized variable warning.

Comment 13

[Matt Brubeck (:mbrubeck)]

Attachment: (3/3) Use honeycomb theme and tablet layout on ICS tablets

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

Comment 14

[Matt Brubeck (:mbrubeck)]

Attachment: (3/3) Use honeycomb theme and tablet layout on ICS tablets

Forgot to qref.  (Man, I am having trouble uploading the right patches to this bug.)

Comment 15

[Matt Brubeck (:mbrubeck)]

Attachment: (1/3) Add IsTablet method to AndroidBridge

Added commit message and addressed review nit.  Carrying r=dougt.

Comment 16

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/7cb2752fce12
https://hg.mozilla.org/mozilla-central/rev/ae066b87a525
https://hg.mozilla.org/mozilla-central/rev/759a688577d5

Comment 17

[Matt Brubeck (:mbrubeck)]

Attachment: rolled-up patch for Aurora

[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.

Comment 18

[Matt Brubeck (:mbrubeck)]

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 19

[Alex Keybl [:akeybl]]

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.

Comment 20

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/deaeace0c0bc