If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Bookmarks menu item does nothing if bookmarks panel is disabled

VERIFIED FIXED in Firefox 46

Status

()

Firefox for Android
General
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: mcomella, Assigned: liuche)

Tracking

unspecified
Firefox 48
All
Android
Points:
---

Firefox Tracking Flags

(firefox46 fixed, firefox47 verified, firefox48 verified, fennec46+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
We should either not show the menu item, or re-enable the bookmarks panel when the item is clicked.

I assume this also works the same way for history.

Comment 1

2 years ago
Discussion in triage is that we're not sure whether we should somehow show the bookmarks panel when you tap on this menu item, or if we should hide the menu item.

Anthony will come up with the UX for this.

Chenxia, can you own coming up with a fix for this, whichever way we decide to go?
tracking-fennec: ? → 46+
Flags: needinfo?(liuche)
Flags: needinfo?(alam)
(Assignee)

Comment 2

2 years ago
Sure - do we think this could be a mentored bug? If not, I can own taking care of it too.
Flags: needinfo?(liuche)

Comment 3

2 years ago
(In reply to Chenxia Liu [:liuche] from comment #2)
> Sure - do we think this could be a mentored bug? If not, I can own taking
> care of it too.

We need a fix we can uplift to beta soon, so no, I don't think it should be a mentored bug (unless you have someone in mind who would be prepared for a quick turnaround).
Assignee: nobody → liuche
I've thought about this more and, all things considered,  I think we should hide the menu item if they hide the panel.

---

Strictly speaking, it's true that "Hide" from within General > Home > Panels, might be seen to only apply to Home panels. But I don't think we should reinforce this type of behaviour going forward.  

The point of these items in the menu is to address a UX issue of discovery. We've seen users have trouble finding these panels, and we've seen users go into the menu looking for these items (maybe because other browsers/apps have these types of shortcuts in their menu?). 

So, as we look at these items as shortcuts, we should disable them if the destination has been disabled.

We can explore more fine grain customization in the future if we see that it's valuable. But in the mean time, I think it's important to maintain this relationship between the Bookmarks panel, and the Bookmarks menu shortcut.
Flags: needinfo?(alam) → needinfo?(liuche)

Comment 5

2 years ago
Thanks for the thoughtful response, I agree that makes sense.

Right now, if a user disabled their bookmarks panel, there's no way for them to see a list of bookmarks, and clearly they're fine with that. So I don't think we need to give them a shortcut to the bookmarks panel in this case.
(Assignee)

Comment 6

2 years ago
Created attachment 8731429 [details]
MozReview Request: Bug 1255077 - Bookmarks menu item does nothing if bookmarks panel is disabled. r=margaret

Review commit: https://reviewboard.mozilla.org/r/40589/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40589/
Attachment #8731429 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 7

2 years ago
Comment on attachment 8731429 [details]
MozReview Request: Bug 1255077 - Bookmarks menu item does nothing if bookmarks panel is disabled. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40589/diff/1-2/
(Assignee)

Comment 8

2 years ago
Margaret, is there's a better way to fetch which home panels are enabled? I'm fetching from HomePager right now (which is higher in the UI stack than we need), but looking at the HomeConfigLoader code, it's not very straightforward to add my own OnLoadListener to check which panels are enabled/disabled.

This uses SharedPreferences to keep track of enabled/hidden state of History or Bookmarks, and also updates that whenever a panel is enabled/disabled from preferences. If that pref doesn't exist, we set it from whatever the current configuration is in HomePager.

This doesn't handle the case before the HomePager is shown and the user doesn't change the history/bookmark state from settings.
(Assignee)

Updated

2 years ago
Flags: needinfo?(liuche)

Comment 9

2 years ago
(In reply to Chenxia Liu [:liuche] from comment #8)
> Margaret, is there's a better way to fetch which home panels are enabled?
> I'm fetching from HomePager right now (which is higher in the UI stack than
> we need), but looking at the HomeConfigLoader code, it's not very
> straightforward to add my own OnLoadListener to check which panels are
> enabled/disabled.

Unfortunately, I don't think there's a better way, but reading directly from HomePager like this doesn't seem great :/

> This uses SharedPreferences to keep track of enabled/hidden state of History
> or Bookmarks, and also updates that whenever a panel is enabled/disabled
> from preferences. If that pref doesn't exist, we set it from whatever the
> current configuration is in HomePager.
>
> This doesn't handle the case before the HomePager is shown and the user
> doesn't change the history/bookmark state from settings.

Given that a user can only enable/disable these things in settings, I think we should just set the SharedPreferences in settings, and we can introduce some migration logic to handle setting this when the user updates.

I think we should avoid setting these prefs in multiple places, since it can become confusing to maintain.

Comment 10

2 years ago
Comment on attachment 8731429 [details]
MozReview Request: Bug 1255077 - Bookmarks menu item does nothing if bookmarks panel is disabled. r=margaret

https://reviewboard.mozilla.org/r/40589/#review37425

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3389
(Diff revision 2)
> +                }
> +
> +                if (!prefs.contains(PanelsPreferenceCategory.PREF_KEY_HISTORY_PANEL_ENABLED)) {
> +                    prefs.edit().putBoolean(PanelsPreferenceCategory.PREF_KEY_HISTORY_PANEL_ENABLED, mHomePager.containsPanel(PanelType.HISTORY)).apply();
> +                }
> +            }

My initial reaction is that this is a bad place to do this. Can we instead include this as migration logic somewhere in the home package (perhaps in HomeConfig?), so that we can avoid needing to expose more APIs on HomePager?

We actually already have migration logic in HomeConfigPrefsBackend. Although we're not migrating JSON here, it seems like setting the preferences could work well there.

::: mobile/android/base/java/org/mozilla/gecko/preferences/PanelsPreferenceCategory.java:25
(Diff revision 2)
>  
>  public class PanelsPreferenceCategory extends CustomListCategory {
>      public static final String LOGTAG = "PanelsPrefCategory";
>  
> +    public static final String PREF_KEY_BOOKMARKS_PANEL_ENABLED = "bookmarksPanelEnabled";
> +    public static final String PREF_KEY_HISTORY_PANEL_ENABLED = "historyPanelEnabled";

Is this the right class to own these keys? Seems like it would be better to centralize this logic in one of the home classes.
Attachment #8731429 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

2 years ago
Attachment #8731429 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 11

2 years ago
Comment on attachment 8731429 [details]
MozReview Request: Bug 1255077 - Bookmarks menu item does nothing if bookmarks panel is disabled. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40589/diff/2-3/

Comment 12

2 years ago
Comment on attachment 8731429 [details]
MozReview Request: Bug 1255077 - Bookmarks menu item does nothing if bookmarks panel is disabled. r=margaret

https://reviewboard.mozilla.org/r/40589/#review38231

::: mobile/android/base/java/org/mozilla/gecko/home/HomePager.java:358
(Diff revision 3)
>          // We should only make the banner active if the toolbar is not focused and we are on the default page
>          final boolean active = !hasFocus && getCurrentItem() == mDefaultPageIndex;
>          mHomeBanner.setActive(active);
>      }
>  
> +    private void updatePrefsFromConfigState(HomeConfig.State configState) {

Add some comments to explain why we need to do this.

::: mobile/android/base/java/org/mozilla/gecko/home/HomePager.java:361
(Diff revision 3)
>      }
>  
> +    private void updatePrefsFromConfigState(HomeConfig.State configState) {
> +        final SharedPreferences prefs = GeckoSharedPrefs.forProfile(getContext());
> +        if (!prefs.contains(HomeConfig.PREF_KEY_BOOKMARKS_PANEL_ENABLED)
> +                || !prefs.contains(HomeConfig.PREF_KEY_HISTORY_PANEL_ENABLED)) {

With this change, we're going to do this check every time the panels are loaded... I think we should do this in HomeConfigPrefsBacked, so we can at least avoid this for people who haven't customized their home panels at all.

I still think it would make more sense to put this in `maybePerformMigration`:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/HomeConfigPrefsBackend.java#186

We don't need to make this a proper migration, but we can at least do the check at the top of this method, which is only called if the user has customized their home panels.
Attachment #8731429 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

2 years ago
Attachment #8731429 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 13

2 years ago
Comment on attachment 8731429 [details]
MozReview Request: Bug 1255077 - Bookmarks menu item does nothing if bookmarks panel is disabled. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40589/diff/3-4/

Comment 14

2 years ago
Comment on attachment 8731429 [details]
MozReview Request: Bug 1255077 - Bookmarks menu item does nothing if bookmarks panel is disabled. r=margaret

https://reviewboard.mozilla.org/r/40589/#review38265

Thanks for your patience with my reviews!
Attachment #8731429 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 15

2 years ago
No problem! I think these were really relevant comments and made the patch better :)

Comment 16

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/9de25eb6d07d
(Assignee)

Comment 17

2 years ago
Created attachment 8733619 [details] [diff] [review]
Aurora/Beta patch: Disable Bookmarks/History menu item if panels are not enabled

Aurora try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05c5060a5dc0
Beta try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f501a41120c

Approval Request Comment
[Feature/regressing bug #]: Bug 1231792, added panels to menu but did not add code to remove them
[User impact if declined]: Users who have customized their panels may have panel menu items that don't do anything
[Describe test coverage new/current, TreeHerder]: local testing, try pushes
[Risks and why]: low, adds a code path that sets some prefs for users with customized home panels - default is to do the same thing as the existing code
[String/UUID change made/needed]: none
Attachment #8733619 - Flags: approval-mozilla-beta?
Attachment #8733619 - Flags: approval-mozilla-aurora?

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9de25eb6d07d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Hi Mike, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(michael.l.comella)
(Reporter)

Comment 20

2 years ago
Looks good to me!
Status: RESOLVED → VERIFIED
Flags: needinfo?(michael.l.comella)
Comment on attachment 8733619 [details] [diff] [review]
Aurora/Beta patch: Disable Bookmarks/History menu item if panels are not enabled

Fix was verified, Aurora47+
Attachment #8733619 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

2 years ago
status-firefox47: --- → affected

Updated

2 years ago
status-firefox46: --- → affected

Comment 22

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/1806435f8228
status-firefox47: affected → fixed
Comment on attachment 8733619 [details] [diff] [review]
Aurora/Beta patch: Disable Bookmarks/History menu item if panels are not enabled

Fix for recent regression in bookmarks, please uplift to beta
Attachment #8733619 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/da8775314086d5f36fdc54262a114ffa59eb5745
status-firefox46: affected → fixed
Bookmarks menu item is hidden if bookmarks panel is disabled.
Verified as fixed using:
Device: Nexus 9 (Android 6.0)
Build: Firefox for Android 47 Beta 1
status-firefox47: fixed → verified
Verified as fixed in build 48.0a2 2016-04-27;
Device: Asus Transformer Pad (Android 4.2.1).
status-firefox48: fixed → verified
You need to log in before you can comment on or make changes to this bug.