1379793 - Settings>General>Home displays panel for Settings>Mozilla

Status: VERIFIED FIXED

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

Description

[Ken Riley]

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.

Comment 1

[Oana Horvath [:ohorvath - out of office]]

Attachment: Screenshot_2017-07-13-11-43-29.png

I can reproduce this issue with Nightly 56 (2017-07-13), on devices with a specific settings menu, as in the screenshot attached.

Comment 2

[Oana Horvath [:ohorvath - out of office]]

Last good build: 2017-07-07
First bad build: 2017-07-10
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a0b5515b13eb&tochange=a418121d4625

Comment 3

[Ken Riley]

Now that everything has advanced a version, this bug now affects beta 56 and nightly 57.

Comment 6

[Jan Henning [:JanH]]

[Tracking Requested - why for this release]:
A part of the settings cannot be accessed on Tablets.

Comment 7

[Jan Henning [:JanH]]

Bug 1374630 caused this - can you please fix this before 56 hit Release?

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1379793 - Only allow pre header as title.

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

Comment 9

[Nevin Chen(Not active on Bugzilla)]

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.

Comment 10

[Julien Cristau [:jcristau]]

ni liz FYI, new regression in fennec 56

Comment 11

[Liz Henry (:lizzard)]

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?

Comment 12

[Liz Henry (:lizzard)]

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.

Comment 13

[Mike Kaply [:mkaply]]

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?

Comment 14

[Jan Henning [:JanH]]

But the wrong text will be only when coming in via a deep link, right?

Comment 15

[:Grisha Kruglov]

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/

Comment 16

[Nevin Chen(Not active on Bugzilla)]

Hi Jing Wei
Please help review this. I'll follow up in bug 1401130

Comment 17

[Nevin Chen(Not active on Bugzilla)]

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

[Jing-wei Wu [:jwu]]

Comment on attachment 8909267 [details]
Bug 1379793 - Only allow pre header as title.

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

Comment 19

[Nevin Chen(Not active on Bugzilla)]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88ee1e36eb17
Only allow pre header as title. r=jwu

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/88ee1e36eb17

Comment 22

[Mike Kaply [:mkaply]]

Please request uplift for this to release.

Comment 23

[Jan Henning [:JanH]]

^^ re uplift

Comment 24

[Nevin Chen(Not active on Bugzilla)]

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

Comment 25

[Oana Horvath [:ohorvath - out of office]]

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)

Comment 26

[Aris]

Samsung Galaxy Tab S2 (9,7', Android 7.0)

- fixed on Nightly 57 (2017-09-20)
- not fixed on Firefox 56 beta 13

Comment 27

[Liz Henry (:lizzard)]

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.

Comment 28

[Liz Henry (:lizzard)]

Nevin, i wonder if this is related to the report in bug 1401337 (which no one has reproduced yet )? What do you think?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Nevin Chen(Not active on Bugzilla)]

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.