Closed Bug 746703 Opened 12 years ago Closed 12 years ago

ICS: Menu Button is displayed twice

Categories

(Firefox for Android Graveyard :: General, defect)

14 Branch
All
Android
defect
Not set
normal

Tracking

(firefox14 verified, firefox15 verified, blocking-fennec1.0 +)

VERIFIED FIXED
Firefox 15
Tracking Status
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- +

People

(Reporter: padamczyk, Assigned: mbrubeck)

References

Details

Attachments

(3 files, 4 obsolete files)

Attached image 2 menu buttons
Using a HTC EVO X (ICS) it appears that device does not recognized that we have a menu button within the app, so it draws another one. See screenshot. 
*The HTC has physical buttons, unlike the Nexus Galaxy.
We should discuss if this is bad/common enough to block.
tracking-fennec: --- → ?
Would be nice to know what the device market size is too.
So this is will happen all the HTC ONE devices, they will likely be fairly popular as the last HTC device was Sprint's #1 selling phone so I hear.

More background:
It appears all apps that have 2.3 support show this issue (facebook, google docs, flickr). The newer 4.0 apps don't seem to exhibit this issue such as Chrome Beta, ICS native apps or Instagram.
I suspect this can be fixed by changing targetSdkVersion to 14 in our AndroidManifest.xml.
Assignee: nobody → mbrubeck
Hardware: x86 → All
Attached patch patch (obsolete) — Splinter Review
Could someone with an affected device please verify that the bug is fixed in this test build?
https://people.mozilla.com/~mbrubeck/fennec-746703.apk
Attachment #616300 - Flags: review?(blassey.bugs)
http://android-developers.blogspot.com/2012/01/say-goodbye-to-menu-button.html explains, "if you set minSdkVersion to 10 or lower, set targetSdkVersion to 11, 12, or 13, and you do not use ActionBar, the system will add the legacy overflow button when running your app on a handset with Android 4.0 or higher.

"That exception might be a bit confusing, but it’s based on the belief that if you designed your app to support pre-Honeycomb handsets and Honeycomb tablets, it probably expects handset devices to include a Menu button (but it supports tablets that don’t have one).

"So, to ensure that the overflow action button never appears beside the system navigation, you should set the targetSdkVersion to 14. (You can leave minSdkVersion at something much lower to continue supporting older devices.)"
https://developer.android.com/sdk/android-4.0.html notes some other effects of setting targetSdkVersion to 14 or higher:

* Hardware acceleration is enabled by default.
* JNI uses indirect references to avoid GC-related bugs.
* Apps that don't specify a theme will use the device's default theme.
Just loaded your apk. FIXED!!!!!
Any possible side effects from targeting SDK 14?
(In reply to Aaron Train [:aaronmt] from comment #9)
> Any possible side effects from targeting SDK 14?

See comment 7 above for known side effects.  I don't think any of these *should* cause any user-visible changes, but we'd definitely need to watch for regressions from this patch, and be prepared to back it out if there are bad regressions that couldn't be fixed in time for the first release.
tracking-fennec: ? → ---
blocking-fennec1.0: --- → ?
Attachment #616300 - Flags: review?(blassey.bugs) → review+
blocking-fennec1.0: ? → +
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ecd07342540
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/7ecd07342540
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 747681
Backed out for causing bug 747681: https://hg.mozilla.org/integration/mozilla-inbound/rev/9daee30907ea
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Aaron points out in bug 747681 that this change hardware-accelerated our window by default. We probably don't handle that too well.

More info: http://android-developers.blogspot.ca/2011/11/android-40-graphics-and-animations.html

I am rather against this landing as-is again. Too many moving parts! It should be the first thing to land in a regular train model, IMO.
(In reply to Joe Drew (:JOEDREW!) from comment #13)
> Backed out for causing bug 747681:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9daee30907ea

Backout merged to m-c in time for 14:
https://hg.mozilla.org/mozilla-central/rev/9daee30907ea
I think we should consider *not* blocking on this because the only available fix is high-risk.  (See comment 10 and comment 14 for details.)  If we can fix this in Firefox 15 instead, it could still be released before buttony ICS phones are too widespread.
blocking-fennec1.0: + → ?
let's try bumping the SDK level to 14 and turning off accelerated surfaces. Either way, we need this fixed sooner rather than later so it can be well tested.
blocking-fennec1.0: ? → +
Attached patch patch v2 (obsolete) — Splinter Review
There's a test build at https://people.mozilla.com/~mbrubeck/fennec-746703-b.apk

Can someone with an affected phone verify that this fixes the menu button, while not causing crashes on rotate (bug 747681)?
Attachment #616300 - Attachment is obsolete: true
Attachment #617975 - Flags: review?(blassey.bugs)
Attached patch patch v2 (obsolete) — Splinter Review
Forgot hg qref.
Attachment #617975 - Attachment is obsolete: true
Attachment #617975 - Flags: review?(blassey.bugs)
Attachment #617989 - Flags: review?(blassey.bugs)
The build above in comment #18 specifying hardwareAccelerated with false is not valid overrode with the forced declaration of targetSDK 14 as it reintroduces bug 747681.
Comment on attachment 617989 [details] [diff] [review]
patch v2

(In reply to Aaron Train [:aaronmt] from comment #20)
> The build above in comment #18 specifying hardwareAccelerated with false is
> not valid overrode with the forced declaration of targetSDK 14 as it
> reintroduces bug 747681.

The targetSdk attribute is not supposed to override an explicit hardwareAccelerated="false" attribute.  According to the docs, "If necessary, you can manually disable hardware acceleration with the hardwareAccelerated attribute for individual <activity> elements or the <application> element." - https://developer.android.com/sdk/android-4.0.html

I think my patch was wrong.  Preparing a new build and patch.
Attachment #617989 - Attachment is obsolete: true
Attachment #617989 - Flags: review?(blassey.bugs)
Attached patch patch v3 (obsolete) — Splinter Review
The previous patch was missing an "android:" namespace prefix.  Oops!

New test build at https://people.mozilla.com/~mbrubeck/fennec-746703-c.apk
Attachment #618711 - Flags: review?(blassey.bugs)
Comment on attachment 618711 [details] [diff] [review]
patch v3

Bug 747681 is apparently not caused by hardware acceleration.  If I just add hardwareAccelerated="true" to the manifest, there are no problems.  So it is caused by some other targetSdkVersion="14" change -- maybe the JNI changes.
Attachment #618711 - Flags: review?(blassey.bugs)
Attached patch patch v4Splinter Review
The activity was restarting because of the new "screenSize" configuration change introduced in SDK version 13 and later.  This fixes the regression.
Attachment #618711 - Attachment is obsolete: true
Attachment #618786 - Flags: review?(blassey.bugs)
Built with this patch. Rotation seems to be fixed. Though I'm crashing when using CTP Flash. Build at http://people.mozilla.com/~kbrosnan/tmp/fennec-746703-d.apk
(In reply to Kevin Brosnan [:kbrosnan] from comment #25)
> Built with this patch. Rotation seems to be fixed. Though I'm crashing when
> using CTP Flash. Build at
> http://people.mozilla.com/~kbrosnan/tmp/fennec-746703-d.apk

The suspicion was that the flash issues were caused by hardware accel. Can we try the current patch plus disabling hardware accel?
Here's an additional patch that disables HW acceleration, and a build with both patches:
https://people.mozilla.com/~mbrubeck/fennec-746703-e.apk
Attachment #619009 - Flags: review?(blassey.bugs)
Keywords: qawanted
Target Milestone: Firefox 14 → ---
qawanted: assigning to kevin to take a look.
fennec-746703-e.apk is still crashing on the Galaxy Nexus when playing Flash. My Android 2.3 phone does not crash when playing Flash.
I/GeckoApp(15116): Got message: DOMContentLoaded
I/GeckoTab(15116): Updated title: Unbelievably Delayed Reaction - YouTube for tab with id: 1
D/GeckoFavicons(15116): Creating LoadFaviconTask with URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo and favicon URL = http://s.ytimg.com/yt/favicon-refresh-vfldLzJxy.ico
D/GeckoFavicons(15116): Calling loadFavicon() with URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo and favicon URL = http://s.ytimg.com/yt/favicon-refresh-vfldLzJxy.ico (4)
I/GeckoApp(15116): URI - http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo, title - Unbelievably Delayed Reaction - YouTube
D/GeckoFavicons(15116): Favicon URL is now: http://s.ytimg.com/yt/favicon-refresh-vfldLzJxy.ico
D/GeckoFavicons(15116): Calling getFaviconUrlForPageUrl() for http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo
D/GeckoFavicons(15116): Downloading favicon for URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo with favicon URL = http://s.ytimg.com/yt/favicon-refresh-vfldLzJxy.ico
I/Gecko   (15116): Compositor: Composite took 50 ms.
D/GeckoFavicons(15116): Downloaded favicon successfully for URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo
D/GeckoFavicons(15116): Saving favicon on browser database for URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo
I/GeckoAppShell(15116): createSurface
E/dalvikvm(15116): JNI ERROR (app bug): accessed stale local reference 0x2590007d (index 31 in a table of size 30)
E/dalvikvm(15116): VM aborting
F/libc    (15116): Fatal signal 11 (SIGSEGV) at 0xdeadd00d (code=1)
D/GeckoFavicons(15116): Saving favicon URL for URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo
D/GeckoFavicons(15116): Calling setFaviconUrlForPageUrl() for http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo
D/GeckoFavicons(15116): LoadFaviconTask finished for URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo (4)
I/GeckoApp(15116): Favicon successfully loaded for URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo
I/GeckoApp(15116): Favicon is for current URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo
I/GeckoTab(15116): Updated favicon for tab with id: 1
I/ActivityManager(  192): Process org.mozilla.fennec_mbrubeck (pid 15116) has died.
W/ActivityManager(  192): Force removing ActivityRecord{41b668d8 org.mozilla.fennec_mbrubeck/.App}: app died, no saved state
I/WindowManager(  192): WIN DEATH: Window{41aa0e90 SurfaceView paused=false}
I/WindowManager(  192): WIN DEATH: Window{41909ca0 org.mozilla.fennec_mbrubeck/org.mozilla.fennec_mbrubeck.App paused=false}
D/dalvikvm(  373): GC_CONCURRENT freed 2897K, 29% free 17013K/23687K, paused 2ms+3ms
I/DEBUG   (12519): debuggerd committing suicide to free the zombie!
D/Zygote  (  118): Process 15116 exited cleanly (11)
W/InputManagerService(  192): Got RemoteException sending setActive(false) notification to pid 15116 uid 10033
I/DEBUG   (15190): debuggerd: Apr 13 2012 21:29:36
D/dalvikvm(  373): GC_FOR_ALLOC freed 311K, 27% free 17426K/23687K, paused 30ms
D/dalvikvm(  373): GC_FOR_ALLOC freed 1359K, 28% free 17245K/23687K, paused 24ms
I/dalvikvm-heap(  373): Grow heap (frag case) to 18.469MB for 1644560-byte allocation
Attachment #619009 - Flags: review?(blassey.bugs) → review+
Attachment #618786 - Flags: review?(blassey.bugs) → review+
It looks like the flash crash is due to the JNI changes mentioned in comment 18. This may be a latent bug in our code that we can fix. (I hope it's not a bug in the Flash plugin's code...)
Status: REOPENED → ASSIGNED
that jni bug is fuxed in bug 749750
Do we want to do one more build to test this or are we all set?
Now that I'm back home I was able to test this locally and verified that bug 749750 fixes the Flash crash.  Pushed to the targetSdkVersion change to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ddb0ce899b

I'll wait until this has made it into Nightly before requesting Aurora approval.  There's also a new test build here if anyone wants to start testing early:
https://people.mozilla.com/~mbrubeck/fennec-746703-f.apk
Target Milestone: --- → Firefox 15
Comment on attachment 619009 [details] [diff] [review]
part 2: disable hardware acceleration?

Not checking in part 2 (disable hardware acceleration) because it did not end up having any effect on the regressions we found.
Attachment #619009 - Flags: checkin-
https://hg.mozilla.org/mozilla-central/rev/9c0d42c3ae49
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Comment on attachment 618786 [details] [diff] [review]
patch v4

[Approval Request Comment]

User impact if declined: Incorrect main UI layout on many ICS devices.

Testing completed (on m-c, etc.): Landed on m-c 5/1

Risk to taking this patch (and alternatives if risky):  Android-only blocker fix.  This patch is somewhat risky since it has various effects on how Android configures our app.  (See discussion above for details.)  Two regressions were already found and fixed.  However, the only alternative as far as I know is to leave this bug unfixed.

String changes made by this patch: None.
Attachment #618786 - Flags: approval-mozilla-aurora?
Comment on attachment 618786 [details] [diff] [review]
patch v4

[Triage Comment]
While a bit risky, this is mobile only and blocks Fennec 1.0. Approved for Aurora 14.
Attachment #618786 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: qawanted
There is a single Menu Button on the latest Nightly and Aurora builds.
Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-29)
Firefox 14.0a2 (2012-05-29)

Device: Galaxy Nexus
OS: Android 4.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.

Attachment

General

Created:
Updated:
Size: