Settings>General>Home displays panel for Settings>Mozilla

VERIFIED FIXED in Firefox 56

Status

()

defect
P2
normal
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: sandken, Assigned: cnevinchen)

Tracking

({regression})

56 Branch
Firefox 57
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+, firefox55 unaffected, firefox56blocking fixed, firefox57+ verified)

Details

(Whiteboard: [FNC][SPT57.3][INT])

Attachments

(2 attachments)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170710100249

Steps to reproduce:

1. Choose Settings from 3-dot menu.
2. Under General, tap Home.


Actual results:

Highlight changes to Settings>Mozilla, and that panel is displayed.


Expected results:

Panel for customizing homepage should have been displayed.

Note that with an earlier Nightly I had hidden Top Sites and made Bookmarks the default.
I can't check whether bug occurs without that change, since I can't bring up the Customize
Homepage panel.
I can reproduce this issue with Nightly 56 (2017-07-13), on devices with a specific settings menu, as in the screenshot attached.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
OS: Unspecified → Android
Hardware: Unspecified → All
tracking-fennec: ? → +
Priority: -- → P2
Last good build: 2017-07-07
First bad build: 2017-07-10
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a0b5515b13eb&tochange=a418121d4625
Now that everything has advanced a version, this bug now affects beta 56 and nightly 57.
Duplicate of this bug: 1394113
Duplicate of this bug: 1400687
[Tracking Requested - why for this release]:
A part of the settings cannot be accessed on Tablets.
Bug 1374630 caused this - can you please fix this before 56 hit Release?
Blocks: 1374630
Flags: needinfo?(cnevinchen)
Assignee: nobody → cnevinchen
Flags: needinfo?(cnevinchen)
Comment on attachment 8909267 [details]
Bug 1379793 - Only allow pre header as title.

https://reviewboard.mozilla.org/r/180862/#review185948

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferenceFragment.java
(Diff revision 1)
>          final int res = getResource();
>          if (res == R.xml.preferences) {
>              return getString(R.string.settings_title);
>          }
>  
> -        if (res == R.xml.preferences_general) {

I remove the code originally added here https://reviewboard.mozilla.org/r/150882/diff/3#index_header

This is because showing the title will also make the logic in update tile goes to 

activity.showBreadCrumbs(newTitle, newTitle);
activity.switchToHeader(getHeader());

when we are in Tablet.

I think the best way is to seperate the logic of update title for locale change and normal title udpate.

After I remove this two, the deeplink will still work, but showing the wrong activity title ("Setting instead" of "home") for phones and tablet.

If it's not urgent, we can follow up this bug and refine later.
ni liz FYI, new regression in fennec 56
Flags: needinfo?(lhenry)
This sounds like a fairly major problem to me. We found it 2 months ago, why would we plan to ship a new regression in 56?    At least 5 people triaged this bug, but no one marked it as a new regression. 

Do we have data on how many people use the customize menu?
Flags: needinfo?(lhenry)
Flags: needinfo?(jcheng)
Flags: needinfo?(cnevinchen)
Keywords: regression
If we can quickly come up with a fix here, we can still get it into a mobile 56 RC build this week. 
So, if you can come up with a patch, please request review and testing and uplift fast. Thanks.
So if I understand correctly, we are broken on tablet only, but if you do your fix, the function will work but we'll have the wrong text on tablet and phone?
But the wrong text will be only when coming in via a deep link, right?
For the affected users, this bug makes the new Activity Stream preferences inaccessible (e.g. disabling Pocket recommendations).

FWIW I've also seen this reported on reddit: https://www.reddit.com/r/firefox/comments/6wxfp3/how_to_disable_the_recommended_by_pocket_section/dn174an/
Blocks: 1386735
Hi Jing Wei
Please help review this. I'll follow up in bug 1401130
Flags: needinfo?(cnevinchen) → needinfo?(topwu.tw)
(In reply to Mike Kaply [:mkaply] from comment #13)
> So if I understand correctly, we are broken on tablet only, but if you do
> your fix, the function will work but we'll have the wrong text on tablet and
> phone?

Yes. The wrong text will be only when coming in via a deep link since the activity's title is not set.
But we are not using this deep link so I think it's fine for now. I'll fix this in bug 1401130.
Comment on attachment 8909267 [details]
Bug 1379793 - Only allow pre header as title.

https://reviewboard.mozilla.org/r/180862/#review186382
Attachment #8909267 - Flags: review?(topwu.tw) → review+
Comment on attachment 8909267 [details]
Bug 1379793 - Only allow pre header as title.

https://reviewboard.mozilla.org/r/180862/#review186376

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferenceFragment.java
(Diff revision 1)
>          final int res = getResource();
>          if (res == R.xml.preferences) {
>              return getString(R.string.settings_title);
>          }
>  
> -        if (res == R.xml.preferences_general) {

follow this in https://bugzilla.mozilla.org/show_bug.cgi?id=1401130
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88ee1e36eb17
Only allow pre header as title. r=jwu
https://hg.mozilla.org/mozilla-central/rev/88ee1e36eb17
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Please request uplift for this to release.
Whiteboard: [FNC][SPT57.3][INT]
Flags: needinfo?(topwu.tw)
^^ re uplift
Flags: needinfo?(cnevinchen)
Comment on attachment 8909267 [details]
Bug 1379793 - Only allow pre header as title.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1374630 
[User impact if declined]: Setting will not work properly
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: It rollback previous fix. So the behavior will be back to previous release
[String changes made/needed]: no
Flags: needinfo?(cnevinchen)
Attachment #8909267 - Flags: approval-mozilla-release?
Verified as fixed on Nightly 57 (2017-09-20).
Devices: 
Xiaomi Mi Pad 2 (Android 5.1)
Lenovo Yoga Tablet 2 (Android 4.4.2)
Status: RESOLVED → VERIFIED
Samsung Galaxy Tab S2 (9,7', Android 7.0)

- fixed on Nightly 57 (2017-09-20)
- not fixed on Firefox 56 beta 13
Comment on attachment 8909267 [details]
Bug 1379793 - Only allow pre header as title.

OK for uplift, fix for release blocking issue for Fennec, verified in Nightly.
Attachment #8909267 - Flags: approval-mozilla-release? → approval-mozilla-release+
Nevin, i wonder if this is related to the report in bug 1401337 (which no one has reproduced yet )? What do you think?
Flags: needinfo?(cnevinchen)
I think they are not related. Since bug 1379793 here is only about the setting page (Java UI) functionality and not related to web content.
Flags: needinfo?(cnevinchen)
Flags: needinfo?(jcheng)
You need to log in before you can comment on or make changes to this bug.