The 'back' button on the upper left corner from the settings menu does not do anything on some devices

VERIFIED FIXED in Firefox 38

Status

()

Firefox for Android
Settings and Preferences
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Flaviu Cos, Assigned: mcomella)

Tracking

({regression})

Trunk
Firefox 40
ARM
Android
regression
Points:
---

Firefox Tracking Flags

(firefox37 unaffected, firefox38+ verified, firefox38.0.5 verified, firefox39+ verified, firefox40+ verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Environment: 
Device: Asus Transformer Tab (Android 4.0.3);
Build: Nightly 40.0a1 (2015-04-08);

Steps to reproduce:
1. Launch Fennec;
2. Go to Menu -> Settings;
3. Tap on the 'back' button on the upper left corner to exit settings.

Expected result:
Settings menu is closed.

Actual result:
The button doesn't do anything.

Notes:
Not reproducible on Asus Transformer Tab (Android 4.2.1);
(Reporter)

Comment 1

3 years ago
Inbound regression window:

Last good revision: ab01fd91bc02
First bad revision: 870046adb53a

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ab01fd91bc02&tochange=870046adb53a
Keywords: regression
[Tracking Requested - why for this release]: regression

bug 1132751 looks suspicious, ni to Mike
tracking-firefox38: --- → ?
tracking-firefox39: --- → ?
tracking-firefox40: --- → ?
Flags: needinfo?(michael.l.comella)

Comment 3

3 years ago
Reproduced too on ZTE x86 too (Android 4.0.4)
Oops! liuche was concerned about this in bug 1132751 comment 11:

> ::: mobile/android/base/preferences/GeckoPreferences.java
> (Diff revision 1)
> > -                actionBar.setHomeButtonEnabled(true);
> 
> For some reason I thought this was related to some v14+ bug:
> https://code.google.com/p/android/issues/detail?id=58007
> 
> But this doesn't even seem to be the fix, so if the "back" button works on,
> say, v14 devices for going "up", feel free to remove it.

I'll try adding it back in. I can't repro locally so here is a test build:

  https://people.mozilla.com/~mcomella/apks/mcomella-1152314_01.apk

Flaviu, can you repro?
Assignee: nobody → michael.l.comella
Blocks: 1132751
Flags: needinfo?(michael.l.comella) → needinfo?(flaviu.cos)
Created attachment 8590956 [details]
MozReview Request: bz://1152314/mcomella

/r/6895 - Bug 1152314 - Backout changeset ae9a28f9f736. r=liuche
/r/6897 - Bug 1152314 - Add comment for settings back button workaround. r=liuche

Pull down these commits:

hg pull -r 38b1a50ad4895883f3e785a21518123d1c7afc6f https://reviewboard-hg.mozilla.org/gecko/
Attachment #8590956 - Flags: review?(liuche)
Created attachment 8591073 [details]
Screenshot: Back icon disappears

I repro-ed this on an Asus Transformer; while this patch does fix the back button not working, on subsequent opening of the Settings menu, the icon is gone.
Flags: needinfo?(flaviu.cos)
Comment on attachment 8590956 [details]
MozReview Request: bz://1152314/mcomella

Clearing this review for some clarification - is it okay to have the icon disappear from the action bar on tap?
Flags: needinfo?(michael.l.comella)
Attachment #8590956 - Flags: review?(liuche)
Tracking for 38+
tracking-firefox38: ? → +
tracking-firefox39: ? → +
tracking-firefox40: ? → +
This is too late for beta 4 but we can still get this in for mobile beta 6, which will go to build next Monday.
Comment on attachment 8590956 [details]
MozReview Request: bz://1152314/mcomella

/r/6895 - Bug 1152314 - Duplicate action bar configuration in code. r=liuche

Pull down this commit:

hg pull -r 7788dca35c768b2da5e89857f260bd8832173bf5 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8590956 - Flags: review?(liuche)
Flags: needinfo?(michael.l.comella)
Note that this may not work on all Honeycomb devices because I use methods in code to set the attributes and unfortunately, some of these methods are not available on Honeycomb.

Flaviu, can you repro on Honeycomb? Build is below:

https://people.mozilla.com/~mcomella/apks/mcomella-1152314_02.apk
Flags: needinfo?(flaviu.cos)
Michael: please test this with locale switching. I had to jump through some real crazy hoops to make everything work everywhere, and this might be one of the hoops.
(In reply to Michael Comella (:mcomella) from comment #11)
> Note that this may not work on all Honeycomb devices because I use methods
> in code to set the attributes and unfortunately, some of these methods are
> not available on Honeycomb.
> 
> Flaviu, can you repro on Honeycomb? Build is below:
> 
> https://people.mozilla.com/~mcomella/apks/mcomella-1152314_02.apk

I've installed the build on Acer Iconia (3.2.1), but the app stops working (exists to home screen) after a few seconds from start. I'll attach the log file.
Flags: needinfo?(flaviu.cos)
Created attachment 8592867 [details]
logfennec1.txt
(In reply to Richard Newman [:rnewman] from comment #12)
> Michael: please test this with locale switching. I had to jump through some
> real crazy hoops to make everything work everywhere, and this might be one
> of the hoops.

By going into preferences, making sure everything looks correct, exiting, switching the locale, reopening preferences, and verifying the locale has changed?

I'm not sure why locale switching would be affected with this patch but if you give me an STR, I'll check.
Flags: needinfo?(rnewman)
(In reply to Catalin Suciu from comment #13)
> I've installed the build on Acer Iconia (3.2.1), but the app stops working
> (exists to home screen) after a few seconds from start. I'll attach the log
> file.

Looks like an unrelated bug - I filed bug 1154836.
How about this one, Catalin?

https://people.mozilla.com/~mcomella/apks/mcomella-1152314_03.apk
Flags: needinfo?(catalin.suciu)
(In reply to Michael Comella (:mcomella) from comment #15)

> I'm not sure why locale switching would be affected with this patch but if
> you give me an STR, I'll check.

The locale switching does stuff when leaving a settings fragment and when leaving GeckoPreferences altogether, so there's some risk if that code is touched.

Steps to verify:

* Open Settings. Verify fragment title.
* Open Customize. Verify.
* Hit hardkey back. Expected: back to Settings.
* Open Customize > Home. Verify.
* Hit action bar back. Expected: back to Customize.
* Hit action bar back. Expected: back to Settings.
* Open Language.
* Tap 'English'. Pick Español (España).
* Verify that title is "Idioma".
* Hit back. Verify that title is "Configuración".
* Personalizar > Inicio. The panel names will be in English (known bug), but everything else should be Spanish.
* Back, back, back.
* Hit menu: it should be in Spanish.
* Hit the tab switcher, +. You should see "Termino de búsqueda…" in the URL bar, and home panels should be named in Spanish.
* Leave the browser. Force-quit. Launch Fennec. It should be in Spanish.
Flags: needinfo?(rnewman)
(In reply to Michael Comella (:mcomella) from comment #17)
> How about this one, Catalin?
> 
> https://people.mozilla.com/~mcomella/apks/mcomella-1152314_03.apk

Same behavior. Fennec stops working.
The logs are showing a different error.
Flags: needinfo?(catalin.suciu)
Created attachment 8593224 [details]
log1152314_03.txt
(In reply to Catalin Suciu from comment #19)
> Same behavior. Fennec stops working.
> The logs are showing a different error.

I first see a different error:

 org.mozilla.gecko.GeckoApp$SessionRestoreException: Could not read from session file

This is probably because we failed so early last time and corrupted the session file. But then I see the same error again, which I think is the one to kill the browser:

 java.lang.ClassNotFoundException: org.mozilla.gecko.MediaPlayerManager$GeckoPresentation

I noticed some more information this time around though:

 W/dalvikvm( 3943): Unable to resolve superclass of Lorg/mozilla/gecko/MediaPlayerManager$GeckoPresentation; (56)
 W/dalvikvm( 3943): Link of class 'Lorg/mozilla/gecko/MediaPlayerManager$GeckoPresentation;' failed
 I/dalvikvm( 3943): Could not find method org.mozilla.gecko.MediaPlayerManager$GeckoPresentation.getDisplay, referenced from method org.mozilla.gecko.MediaPlayerManager.updatePresentation
 W/dalvikvm( 3943): VFY: unable to resolve virtual method 23128: Lorg/mozilla/gecko/MediaPlayerManager$GeckoPresentation;.getDisplay ()Landroid/view/Display;
Catalin, how about this?

https://people.mozilla.com/~mcomella/apks/mcomella-1152314_04.apk

(Built on top of bug 1154836 and bug 1155331 to avoid HC crashes)
Flags: needinfo?(catalin.suciu)
Comment on attachment 8590956 [details]
MozReview Request: bz://1152314/mcomella

https://reviewboard.mozilla.org/r/6893/#review5945

Ship It!

::: mobile/android/base/resources/values-v11/styles.xml
(Diff revision 2)
> +         <!-- Partially duplicated in GeckoPreferences.onCreate - look there for more details. -->

You could be more specific and also/just reference initActionBar()

::: mobile/android/base/preferences/GeckoPreferences.java
(Diff revision 2)
> +        if (Versions.feature14Plus) {

I guess this should work for v11-v13 devices, because the bug is v14+?
Attachment #8590956 - Flags: review?(liuche) → review+
(In reply to Michael Comella (:mcomella) from comment #22)
> Catalin, how about this?
> 
> https://people.mozilla.com/~mcomella/apks/mcomella-1152314_04.apk
> 
> (Built on top of bug 1154836 and bug 1155331 to avoid HC crashes)

Works fine on this build. Tapping on the Fennec icon will exit from Settings. Tested on Acer Iconia A500 (3.2.1)
 
Please note that before landing bug #1132751, the icon was not displayed on this device (see bug #1055556)
Currently the new icon is displayed, but the 'Back' symbol is missing
Flags: needinfo?(catalin.suciu)
(In reply to Catalin Suciu from comment #24)
> Please note that before landing bug #1132751, the icon was not displayed on
> this device (see bug #1055556)
> Currently the new icon is displayed, but the 'Back' symbol is missing

Seems alright given:
  1) Honeycomb EOL - bug 1155801
  2) There's the Android back button to escape from the menu, which users are probably used to using.
https://reviewboard.mozilla.org/r/6893/#review6351

> I guess this should work for v11-v13 devices, because the bug is v14+?

I was leaning on the XML attributes doing the right thing for HC devices.

This is an improvement for v14+ devices, at the very least.
Comment on attachment 8590956 [details]
MozReview Request: bz://1152314/mcomella

Approval Request Comment
[Feature/regressing bug #]: bug 1132751
[User impact if declined]:
  The back button on the top left corner of settings doesn't do anything on some devices.

[Describe test coverage new/current, TreeHerder]: Tested locally, QA testing
[Risks and why]: 
  Low - we duplicate some existing XML attributes in code. We previously had one attribute duplicated in code so I extended that to all of the attributes. Worst case, we get them wrong and have that override the XML to give the wrong state, and we have a similar issue to the one we have now (e.g. back still doesn't work, build icon is missing, etc.)

[String/UUID change made/needed]: None
Attachment #8590956 - Flags: approval-mozilla-beta?
Attachment #8590956 - Flags: approval-mozilla-aurora?
Hi Michael, happy to take this for 39, but it looks like it still needs checkin and I'd like to see it on mozilla-central before we uplift. Thanks!
Flags: needinfo?(michael.l.comella)
https://hg.mozilla.org/mozilla-central/rev/1fe760f5c298
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Looks like we're good - thanks, Liz!
Flags: needinfo?(michael.l.comella)
Comment on attachment 8590956 [details]
MozReview Request: bz://1152314/mcomella

Taking it for 38 as it is an important regression.
Attachment #8590956 - Flags: approval-mozilla-release+
Attachment #8590956 - Flags: approval-mozilla-beta?
Attachment #8590956 - Flags: approval-mozilla-aurora?
Attachment #8590956 - Flags: approval-mozilla-aurora+
(Reporter)

Comment 33

3 years ago
Verified as fixed in build 40.0a1 (2015-04-28);
Device: Asus Transformer Tab (Android 4.0.3).
status-firefox40: fixed → verified
Comment on attachment 8590956 [details]
MozReview Request: bz://1152314/mcomella

(In reply to Sylvestre Ledru [:sylvestre] from comment #32)
> Comment on attachment 8590956 [details]
> MozReview Request: bz://1152314/mcomella
> 
> Taking it for 38 as it is an important regression.

38 is beta.
Attachment #8590956 - Flags: approval-mozilla-release+ → approval-mozilla-beta?
Comment on attachment 8590956 [details]
MozReview Request: bz://1152314/mcomella

[Triage Comment]
No, it is not.

m-r = 38.0
m-b = 38.0.5
Attachment #8590956 - Flags: approval-mozilla-beta? → approval-mozilla-release+
(Reporter)

Comment 39

3 years ago
Verified as fixed in build 49.0a2 (2015-04-29);
Device: Asus Transformer Tab (Android 4.0.3).
status-firefox39: fixed → verified
(Reporter)

Comment 40

3 years ago
Verified as fixed in Firefox 38 Beta 10 build 2;
Device: Asus Transformer Tab (Android 4.0.3).
status-firefox38: fixed → verified
(Reporter)

Comment 41

3 years ago
Verified as fixed in Firefox 38.0.5 RC.
Device: Asus Transformer Tab (Android 4.0.3).
Status: RESOLVED → VERIFIED
status-firefox38.0.5: fixed → verified
Comment on attachment 8590956 [details]
MozReview Request: bz://1152314/mcomella
Attachment #8590956 - Attachment is obsolete: true
Attachment #8619985 - Flags: review+
Attachment #8619986 - Flags: review+
Created attachment 8619985 [details]
MozReview Request: Bug 1152314 - Add comment for settings back button workaround. r=liuche
Created attachment 8619986 [details]
MozReview Request: Bug 1152314 - Duplicate action bar configuration in code. r=liuche
You need to log in before you can comment on or make changes to this bug.