1462594 - Settings: General, Accessibility and Advanced menus cannot be accessed

Status: VERIFIED FIXED

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

Description

[Igor Lazar]

Attachment: 2018_05_18_13_41_12.mp4

[Tracking Requested - why for this release]:

Devices: Xiaomi Mi Pad 3 (Android 7.0), Samsung Galaxy Tab S3 (Android 7.0), Nexus 9 tablet (Android 7.1.1)

Build: Nightly 62.0a1 (2018-05-17)

Steps to reproduce:

1. Run Firefox
2. Navigate to settings
3. Select all settings' menus

Expected result:
All menus are accessible

Actual result:
Last menu is focus upon selecting Settings (Mozilla). Unable to access General, Accessibility and Advanced - tapping these menus will navigate the user to the Mozilla menu.

Note: Occurs only on tablet devices.

Comment 1

[Igor Lazar]

Note: Issue does NOT occur on the previous Nightly (/Beta) builds

Comment 2

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Tracking since this sounds like a new regression. 
No access to settings on tablets - this also sounds like a release blocker to me. 

Michael, can you take a look or help find an owner for this bug?

Comment 3

[Ken Riley]

This sounds like bug 1379793 that I reported, shown as fixed in nightly 57 and release 56.

Comment 4

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

Bogdan, can you verify this is fixed with bug 1379793 and if not, NI Susheel to find an owner? Thanks.

Comment 5

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Susheel, can you help out here? 

Since this was just reported on 62, I don't think we need to verify bug 1379793. Whatever fix happened then, it's apparently regressed again.

Comment 6

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Igor can you help find a more specific regression range in Nightly 62?

Comment 7

[Igor Lazar]

First bad build: 2018-05-17 02:17:20.057000

pushlog_url: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=26af09fb8be9e644be92f4dea6b94440804c13a8&tochange=e5ecf6627dfd203a2d6e3f764811fb132fe2126a

Last good build: 2018-05-17 01:31:50.631000

pushlog_url: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d25e01a0ac0963b8c7b7dfa22fbc1912828417ba&tochange=e5ecf6627dfd203a2d6e3f764811fb132fe2126a

Comment 8

[:sdaswani]

Vlad, can you get this on the high priority list? And also talk to your QA mates about getting you access to a tablet?

Comment 9

[Petru-Mugurel Lingurar [:petru]]

Indeed this bug seems to have been caused by bug 1434603 in which I basically reverted what Nevin did to resolve 1379793.

Comment 10

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1462594 - Allow accessing all Settings menus on tablets;

Bug details:
The problem stemmed from the now called GeckoPreferences.trySwitchToHeader(int id) which could be called with an invalid id, constant with the same value as the id of the last available setting.
(GeckoPreferenceFragment().getHeader() would return valid ids only for preference screens that are launched directly. Otherwise it would return: -1)
By chance the id for the last available setting - vendor was not set and so Android saw it with an invalid header id: -1.
GeckoPreferences.trySwitchToHeader(int id) would just switch to showing the vendor setting because that is what he has been instructed to whenever the user tried to access other settings than the ones which can be launched directly.

Cleaned the code a bit:
- renamed GeckoPreferences.switchToHeader(..) to trySwitchToHeader(..) as it won't always perform that action
- removed the call to activity.showBreadCrumbs(..) as in my tests it didn't have any effect and the documentation says "This will normally be called for you".

Tested on An Android 8 tablet, on an Android 8 phone, on an Android 5.0.1 phone and all works ok.

Review commit: https://reviewboard.mozilla.org/r/246386/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246386/

Comment 11

[Petru-Mugurel Lingurar [:petru]]

About the bug and why it always opened the last settings when the user would try to open certain others:

The problem stemmed from the now called GeckoPreferences.trySwitchToHeader(int id) which could be called with an invalid id, constant with the same value as the id of the last available setting.
(GeckoPreferenceFragment().getHeader() would return valid ids only for preference screens that are launched directly. Otherwise it would return: -1)
By chance the id for the last available setting - vendor was not set and so Android saw it with an invalid header id: -1.
GeckoPreferences.trySwitchToHeader(int id) would just switch to showing the vendor setting because that is what he has been instructed to whenever the user tried to access other settings than the ones which can be launched directly.

Comment 12

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

Comment on attachment 8980234 [details]
Bug 1462594 - Allow accessing all Settings menus on tablets;

https://reviewboard.mozilla.org/r/246386/#review253072

This seems reasonable to me. Not sure nits are worth the churn so I'm going to drop them and land.

::: mobile/android/app/src/main/res/xml-v11/preference_headers.xml:70
(Diff revision 1)
>              android:value="preferences_default_browser_tablet"/>
>      </header>
>  
>      <header android:fragment="org.mozilla.gecko.preferences.GeckoPreferenceFragment"
> -            android:title="@string/pref_header_vendor">
> +            android:title="@string/pref_header_vendor"
> +            android:id="@+id/pref_header_vendor">

nit: maybe add a comment in this file that IDs must be set.

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:472
(Diff revision 1)
> +        if (id == GeckoPreferenceFragment.HEADER_ID_UNDEFINED) {
> +            return;

I wonder if we should just crash instead... Assuming this won't happen without additional programmer error.

Then again, I don't know that there's a test for this and it's a low access area so it'd be risky to crash.

Maybe we should validate the preference XML on startup to make sure they all have IDs? I'm just thinking maybe there's a way to make sure we don't ever get invalid IDs.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/276a007b27f2
Allow accessing all Settings menus on tablets; r=mcomella

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/276a007b27f2

Comment 15

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8980234 [details]
Bug 1462594 - Allow accessing all Settings menus on tablets;

https://reviewboard.mozilla.org/r/246386/#review253072

> nit: maybe add a comment in this file that IDs must be set.

Indeed, that could have been helpful.
Found it a bit strange that all other prefs had an id, just this last one didn't.
With now all the prefs having one I think a rule could be observed..

> I wonder if we should just crash instead... Assuming this won't happen without additional programmer error.
> 
> Then again, I don't know that there's a test for this and it's a low access area so it'd be risky to crash.
> 
> Maybe we should validate the preference XML on startup to make sure they all have IDs? I'm just thinking maybe there's a way to make sure we don't ever get invalid IDs.

My understanding was that the functionality from GeckoPreferences.trySwitchToHeader() would only be helpful on tablets, where a certain preference screen is launched directly. This method would focus the parent setting header (in the left of the split layout).
For this, GeckoPreferenceFragment.getHeader() [1] would be used which would either return a valid ID (for the screens which can be launched directly) or a default, "invalid" one.
GeckoPreferences.trySwitchToHeader() would iterate through the list of available ID's (to which we now just added the ID for the vendor setting). If a valid ID - it would switch to it, otherwise nothing would happen.

So indeed, getting an invalid ID is possible by design and could be seen as a programmer error - if a certain setting is to be launched directly but GeckoPreferenceFragment.getHeader() would not be updated to return a valid header id. 
There is no test for this (haven't seen any tests for the settings screens) but it is something that could easily be observed when manually testing a new implementation (..for a screen which is launched directly).

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferenceFragment.java#133

Comment 16

[Andrei Bodea[:andrei]]

Hello,

I've tried to reproduce this issue following the steps from the description but i could not reproduce it.
Note that i managed to access the General, Accessibility and Advanced tabs.

Devices:
Xiaomi Mi Pad 3 (Android 7.0)
Nexus 9 tablet (Android 7.1.1)

Build:
Nightly 62.0a1

Thanks,
Andrei

Comment 17

[Ryan VanderMeulen [:RyanVM]]

This also needs a Beta approval request before we can take bug 1434603.

Comment 18

[Petru-Mugurel Lingurar [:petru]]

Comment on attachment 8980234 [details]
Bug 1462594 - Allow accessing all Settings menus on tablets;

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1434603
[User impact if declined]: When trying to access certain settings while on a tablet the user would get directed to the "vendor" setting on the bottom list of settings, not to the intended one.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Has been verified by QE
[List of other uplifts needed for the feature/fix]: Bug 1434603
[Is the change risky?]: No
[Why is the change risky/not risky?]: Minor change, verified by QE
[String changes made/needed]: None

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8980234 [details]
Bug 1462594 - Allow accessing all Settings menus on tablets;

Fixes a regression from bug 1434603. Approved for 61.0b11.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/55923d758142

Comment 21

[Levente Sacal]

Verified as fixed in Beta 61.0b11 on Google Pixel C and on Nightly 62.0a1 as per the above comment from Andrei.