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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Settings and Preferences, defect)

Description

[Flaviu Cos]

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

Comment 1

[Flaviu Cos]

Inbound regression window:

Last good revision: ab01fd91bc02
First bad revision: 870046adb53a

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ab01fd91bc02&tochange=870046adb53a

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: regression

bug 1132751 looks suspicious, ni to Mike

Comment 3

[Ioana Chiorean]

Reproduced too on ZTE x86 too (Android 4.0.4)

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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?

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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/

Comment 6

[Chenxia Liu [:liuche]]

Attachment: 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.

Comment 7

[Chenxia Liu [:liuche]]

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?

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 38+

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

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 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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/

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 12

[Richard Newman [:rnewman]]

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.

Comment 13

[Catalin Suciu [:csuciu]]

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

Comment 14

[Catalin Suciu [:csuciu]]

Attachment: logfennec1.txt

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

How about this one, Catalin?

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

Comment 18

[Richard Newman [:rnewman]]

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

Comment 19

[Catalin Suciu [:csuciu]]

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

Comment 20

[Catalin Suciu [:csuciu]]

Attachment: log1152314_03.txt

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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)

Comment 23

[Chenxia Liu [:liuche]]

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+?

Comment 24

[Catalin Suciu [:csuciu]]

(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

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/1fe760f5c298

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

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!

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1fe760f5c298

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Looks like we're good - thanks, Liz!

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8590956 [details]
MozReview Request: bz://1152314/mcomella

Taking it for 38 as it is an important regression.

Comment 33

[Flaviu Cos]

Verified as fixed in build 40.0a1 (2015-04-28);
Device: Asus Transformer Tab (Android 4.0.3).

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9b92167bac18

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 36

[Sylvestre Ledru [:Sylvestre]]

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

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/cdfd06d73d17

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cdfd06d73d17

Comment 39

[Flaviu Cos]

Verified as fixed in build 49.0a2 (2015-04-29);
Device: Asus Transformer Tab (Android 4.0.3).

Comment 40

[Flaviu Cos]

Verified as fixed in Firefox 38 Beta 10 build 2;
Device: Asus Transformer Tab (Android 4.0.3).

Comment 41

[Flaviu Cos]

Verified as fixed in Firefox 38.0.5 RC.
Device: Asus Transformer Tab (Android 4.0.3).

Comment 43

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1152314 - Add comment for settings back button workaround. r=liuche

Comment 44

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1152314 - Duplicate action bar configuration in code. r=liuche