Closed Bug 1462594 Opened 2 years ago Closed 2 years ago

Settings: General, Accessibility and Advanced menus cannot be accessed

Categories

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

Firefox 62
All
Android
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 62
Tracking Status
firefox60 --- unaffected
firefox61 --- verified
firefox62 --- verified

People

(Reporter: igor.lazar, Assigned: petru)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached video 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.
Target Milestone: Firefox 62 → ---
tracking-fennec: --- → ?
Note: Issue does NOT occur on the previous Nightly (/Beta) builds
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?
Flags: needinfo?(michael.l.comella)
Keywords: regression
This sounds like bug 1379793 that I reported, shown as fixed in nightly 57 and release 56.
Bogdan, can you verify this is fixed with bug 1379793 and if not, NI Susheel to find an owner? Thanks.
Flags: needinfo?(michael.l.comella) → needinfo?(bogdan.surd)
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.
Flags: needinfo?(bogdan.surd) → needinfo?(sdaswani)
Igor can you help find a more specific regression range in Nightly 62?
Flags: needinfo?(igor.lazar)
Vlad, can you get this on the high priority list? And also talk to your QA mates about getting you access to a tablet?
Flags: needinfo?(sdaswani) → needinfo?(vlad.baicu)
Blocks: 1434603
Hardware: ARM → All
Indeed this bug seems to have been caused by bug 1434603 in which I basically reverted what Nevin did to resolve 1379793.
Flags: needinfo?(vlad.baicu)
Assignee: nobody → petru.lingurar
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.
Attachment #8980234 - Flags: review?(sdaswani) → review?(michael.l.comella)
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.
Attachment #8980234 - Flags: review?(michael.l.comella) → review+
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/276a007b27f2
Allow accessing all Settings menus on tablets; r=mcomella
https://hg.mozilla.org/mozilla-central/rev/276a007b27f2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Flags: qe-verify+
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
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
This also needs a Beta approval request before we can take bug 1434603.
Flags: needinfo?(petru.lingurar)
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
Flags: needinfo?(petru.lingurar)
Attachment #8980234 - Flags: approval-mozilla-beta?
Comment on attachment 8980234 [details]
Bug 1462594 - Allow accessing all Settings menus on tablets;

Fixes a regression from bug 1434603. Approved for 61.0b11.
Attachment #8980234 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed in Beta 61.0b11 on Google Pixel C and on Nightly 62.0a1 as per the above comment from Andrei.
Status: VERIFIED → RESOLVED
tracking-fennec: ? → ---
Closed: 2 years ago2 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.