Add a submenu for bookmarks

RESOLVED FIXED in Firefox 30

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: ibarlow, Assigned: bnicholson)

Tracking

Trunk
Firefox 30
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap ?

Firefox Tracking Flags

(fennec30+)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 8367515 [details]
Mockup

We have a few ways in which users can save things in Firefox. They can
* Bookmark
* Add to Reading List
* Add to Android home screen. 
* and soon, we will be able to add RSS feeds to the home screen. 

"Bookmark" is the only function that is easy to discover in the primary UI, everything else relies on some strange interactions like long pressing, or following a link through a toast. 

I'd like to propose bundling these items into a submenu that is triggered when a user taps the bookmark star. A mockup is attached here. This is something I've been wanting to try for a while, but came up this week in relation to our homepage customization / RSS feed integration, so it seemed like a good reason to pull the trigger.

-------

Bonus points if we can instrument this. Extra points if we can instrument what we currently have, and measure before-and-after, to see if there is an uptick of users adding things to their Reading List or saving to their Homescreen with this new UI.
(Reporter)

Updated

5 years ago
tracking-fennec: --- → ?

Updated

5 years ago
Assignee: nobody → bnicholson
tracking-fennec: ? → 30+
(Assignee)

Comment 1

5 years ago
Created attachment 8373679 [details] [diff] [review]
WIP patch

Here's a WIP patch that covers the functionality described above. Build here: https://dl.dropboxusercontent.com/u/35559547/fennec/fennec-submenu.apk

Some questions:
* What should we do on tablets? Will the star still open a menu from the URL bar?
* Should we have a checkbox for Bookmarks/Reading list menu items?
* I noticed that the mockup doesn't include capitalization for menu items other than the first word. Intentional?
(Assignee)

Updated

5 years ago
Flags: needinfo?(ibarlow)
One more question: it seems this new submenu overlaps a bit with the new 'Page' menu (e.g. Subscribe to feed, Add to Android home screen). Maybe we should avoid the repeated items?
(Reporter)

Comment 3

5 years ago
(In reply to Brian Nicholson (:bnicholson) from comment #1)
> Created attachment 8373679 [details] [diff] [review]
> WIP patch
> 
> Here's a WIP patch that covers the functionality described above. Build
> here: https://dl.dropboxusercontent.com/u/35559547/fennec/fennec-submenu.apk

Couple things: 
* Let's lose the "more" arrow
* You probably know this but the menu items don't actually work -- touching "bookmark" adds a new tab, etc
* Let's make sure we only show the "subscribe" item when we know there is a feed we to which we can subscribe.

> Some questions:
> * What should we do on tablets? Will the star still open a menu from the URL
> bar?

Yes

> * Should we have a checkbox for Bookmarks/Reading list menu items?

That's not a bad idea.

> * I noticed that the mockup doesn't include capitalization for menu items
> other than the first word. Intentional?

Yes, but maybe should wait until we land bug 893836 which is about changing everything in our menus to sentence case.




(In reply to Lucas) 
> One more question: it seems this new submenu overlaps a bit with the new 'Page' 
> menu (e.g. Subscribe to feed, Add to Android home screen). Maybe we should avoid 
> the repeated items?

I don't mind having them in both places. Much like we offer different entry points to creating new tabs in our UI, it makes sense to provide different paths to saving content as well. 

In fact I'd actually like to keep both, and instrument to see what paths our users tend to favor more.
Flags: needinfo?(ibarlow)
(Assignee)

Updated

5 years ago
Depends on: 971413
(Assignee)

Comment 4

5 years ago
Created attachment 8374585 [details] [diff] [review]
Remove arrow from secondary submenu items

Removes the arrow on secondary action bar items in the menu.
Attachment #8373679 - Attachment is obsolete: true
Attachment #8374585 - Flags: review?(wjohnston)
(Assignee)

Comment 5

5 years ago
Created attachment 8374597 [details] [diff] [review]
Add submenu for bookmarks

Build here: http://people.mozilla.org/~bnicholson/fennec_builds/bookmark-submenu.apk

(In reply to Ian Barlow (:ibarlow) from comment #3)
> * You probably know this but the menu items don't actually work -- touching
> "bookmark" adds a new tab, etc

I didn't know that actually -- looks like there was a problem with the last build I posted on tablets. This patch apparently needs a clobber.

> * Let's make sure we only show the "subscribe" item when we know there is a
> feed we to which we can subscribe.

Should be working, hopefully just another clobber-related issue.
Attachment #8374597 - Flags: review?(wjohnston)
(Assignee)

Updated

5 years ago
Flags: needinfo?(ibarlow)
Comment on attachment 8374597 [details] [diff] [review]
Add submenu for bookmarks

Review of attachment 8374597 [details] [diff] [review]:
-----------------------------------------------------------------

r- because I don't really think we need to move all this stuff to Tab.java. I could probably be talked out of that. Also some other questions, like should be change the "Options" button toast to an "Edit" one.

::: mobile/android/base/BrowserApp.java
@@ +664,2 @@
>          Resources res = getResources();
>          items[0] = new Prompt.PromptListItem(res.getString(R.string.contextmenu_edit_bookmark));

If we're down to one menu item here, I'd rather we just put "Edit" in the toast and opened the EditDialog straight from that.

@@ +732,5 @@
>              return true;
>          }
>  
>          if (itemId == R.id.subscribe) {
> +            Tabs.getInstance().getSelectedTab().subscribeToFeeds();

You removed a if (tab != null) check here. Is  that safe?

@@ +2134,5 @@
>              exitGuestMode.setVisible(true);
>          else
>              enterGuestMode.setVisible(true);
>  
> +        addToReadingList.setCheckable(true);

I see we do it multiple places, but this doesn't need to be done every time we show the menu

@@ +2271,5 @@
>              return true;
>          }
>  
> +        if (itemId == R.id.launcher_add) {
> +            tab.addToLauncher();

Do we really want to move these actions to tabs. They're actions on url's, not tabs. I worry a bit that Tab is becoming a dumping ground.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +237,5 @@
>  <!ENTITY contextmenu_open_new_tab "Open in New Tab">
>  <!ENTITY contextmenu_open_private_tab "Open in Private Tab">
>  <!ENTITY contextmenu_open_in_reader "Open in Reader">
>  <!ENTITY contextmenu_remove "Remove">
> +<!ENTITY contextmenu_add_to_launcher2 "Add to Android Home Screen">

We should get QA to verify this string isn't too long.

::: mobile/android/base/resources/menu-large-v11/browser_app_menu.xml
@@ +24,2 @@
>            android:icon="@drawable/ic_menu_bookmark_add"
> +          android:showAsAction="ifRoom">

You'll need a content description on this.
Attachment #8374597 - Flags: review?(wjohnston) → review-
Attachment #8374585 - Flags: review?(wjohnston) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 8374620 [details]
Android 2.3 screenshot

Just remembered that older versions of Android need a label for the menu -- see screenshot. Ian, what should this label be? "Save"?
(In reply to Wesley Johnston (:wesj) from comment #6)

> 
> r- because I don't really think we need to move all this stuff to Tab.java.
> I could probably be talked out of that. Also some other questions, like
> should be change the "Options" button toast to an "Edit" one.

I agree with Wes. We need less non-Tab state in Tab.java, not more.
(Reporter)

Comment 9

5 years ago
(In reply to Brian Nicholson (:bnicholson) from comment #7)
> Created attachment 8374620 [details]
> Android 2.3 screenshot
> 
> Just remembered that older versions of Android need a label for the menu --
> see screenshot. Ian, what should this label be? "Save"?

Oh right, forgot about that. "Save" sounds reasonable.
Flags: needinfo?(ibarlow)
(In reply to Mark Finkle (:mfinkle) from comment #8)
> (In reply to Wesley Johnston (:wesj) from comment #6)
> 
> > 
> > r- because I don't really think we need to move all this stuff to Tab.java.
> > I could probably be talked out of that. Also some other questions, like
> > should be change the "Options" button toast to an "Edit" one.
> 
> I agree with Wes. We need less non-Tab state in Tab.java, not more.

Since its basically orthogonal to this patch, sounds like something that we could work out in a different bug :)
(Assignee)

Comment 11

5 years ago
Created attachment 8375141 [details] [diff] [review]
Add submenu for bookmarks, v2

(In reply to Wesley Johnston (:wesj) from comment #6)
> @@ +732,5 @@
> >              return true;
> >          }
> >  
> >          if (itemId == R.id.subscribe) {
> > +            Tabs.getInstance().getSelectedTab().subscribeToFeeds();
> 
> You removed a if (tab != null) check here. Is  that safe?

Yes; ever since we've had tab stubs, we're guaranteed to have at least one tab by the end of BrowserApp#onCreate.

> @@ +2271,5 @@
> >              return true;
> >          }
> >  
> > +        if (itemId == R.id.launcher_add) {
> > +            tab.addToLauncher();
> 
> Do we really want to move these actions to tabs. They're actions on url's,
> not tabs. I worry a bit that Tab is becoming a dumping ground.

After some discussion on IRC, I created an addToHomeScreen method in BrowserApp that we may eventually want to move elsewhere. I was going to create a ReaderList class for reader-related code, but then I saw that we already have ReaderModeUtils. This seemed good enough, so I moved addToReadingList/removeFromReadingList into there.

> ::: mobile/android/base/locales/en-US/android_strings.dtd
> @@ +237,5 @@
> >  <!ENTITY contextmenu_open_new_tab "Open in New Tab">
> >  <!ENTITY contextmenu_open_private_tab "Open in Private Tab">
> >  <!ENTITY contextmenu_open_in_reader "Open in Reader">
> >  <!ENTITY contextmenu_remove "Remove">
> > +<!ENTITY contextmenu_add_to_launcher2 "Add to Android Home Screen">
> 
> We should get QA to verify this string isn't too long.

OK. I tested on Droid Pro and it fits, but Kevin said there are even smaller devices so that's a good idea.

> ::: mobile/android/base/resources/menu-large-v11/browser_app_menu.xml
> @@ +24,2 @@
> >            android:icon="@drawable/ic_menu_bookmark_add"
> > +          android:showAsAction="ifRoom">
> 
> You'll need a content description on this.

Beat me to it! Updated with "Save".

Incoming: Updated tests.
Attachment #8374597 - Attachment is obsolete: true
Attachment #8374620 - Attachment is obsolete: true
Attachment #8375141 - Flags: review?(wjohnston)
(Assignee)

Comment 12

5 years ago
Comment on attachment 8375141 [details] [diff] [review]
Add submenu for bookmarks, v2

Sorry, forgot to remove a couple things from Tabs.
Attachment #8375141 - Flags: review?(wjohnston)
(Assignee)

Comment 13

5 years ago
Created attachment 8375310 [details] [diff] [review]
Add submenu for bookmarks, v3

Build here: http://people.mozilla.org/~bnicholson/fennec_builds/bookmark-submenu2.apk
Attachment #8375141 - Attachment is obsolete: true
Attachment #8375310 - Flags: review?(wjohnston)
Comment on attachment 8375310 [details] [diff] [review]
Add submenu for bookmarks, v3

>-<!ENTITY contextmenu_add_to_launcher "Add to Home Screen">
>+<!ENTITY contextmenu_add_to_launcher2 "Add to Android Home Screen">

The "Android" part seems a it odd
(Assignee)

Comment 15

5 years ago
(In reply to Mark Finkle (:mfinkle) from comment #14)
> The "Android" part seems a it odd

We call our home panel "Home" too, so it makes sense to differentiate them to avoid confusion. Maybe we could use another term -- "Launcher" is what immediately comes to mind, but do users know what that is?
(Assignee)

Comment 16

5 years ago
Assuming the string doesn't change, can QA verify that "Add to Android Home Screen" fits on the smallest supported Android screens? You can test using the build from comment 13, clicking the star in the menu. Thanks.
Keywords: qawanted
Had a quick look at the apk. The submenu with just two items (one of them being a toggle item) looks/feels kinda clunky. Maybe it's because this submenu only looks good when it has more than 2 options?
Home screen = the Android one
Home panel = the Firefox Hub one

Also, in the Tools submenu and new Page submenu, we disable actions that are not possible. We do not hide them. Assuming this for the Bookmark submenu, all action would be visible, but some could be disabled.
(Reporter)

Comment 19

5 years ago
(In reply to Mark Finkle (:mfinkle) from comment #18)
> Home screen = the Android one
> Home panel = the Firefox Hub one
>

I would be ok with simplifing it down to just "Home screen" for the Android launcher one. I think that's fine because I can't see us ever creating a generic "save to Firefox Home screen" command, since we would want to be more panel-specific -- eg, "Add to reading list", "Add to Pocket", etc.

 
> Also, in the Tools submenu and new Page submenu, we disable actions that are
> not possible. We do not hide them. Assuming this for the Bookmark submenu,
> all action would be visible, but some could be disabled.

That seems reasonable. Could also help the discoverability of features like RSS / Reading list that may not be immediately obvious.
(Assignee)

Comment 20

5 years ago
Created attachment 8375730 [details] [diff] [review]
Add submenu for bookmarks, v4

Updated to address above comments.
Attachment #8375310 - Attachment is obsolete: true
Attachment #8375310 - Flags: review?(wjohnston)
Attachment #8375730 - Flags: review?(wjohnston)
(Assignee)

Updated

5 years ago
Keywords: qawanted
Comment on attachment 8375730 [details] [diff] [review]
Add submenu for bookmarks, v4

Review of attachment 8375730 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +1983,5 @@
>              share.setActionProvider(provider);
>          }
>  
> +        menu.findItem(R.id.bookmark).setCheckable(true);
> +        menu.findItem(R.id.reading_list_add).setCheckable(true);

This can be specified in the xml. Do you know why we have this in here at all?

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/menu/browser_app_menu.xml#44
Attachment #8375730 - Flags: review?(wjohnston) → review+
brian, one last thing. Can you add an id to this in browser.js so that its possible for addons to extend this menu? Something like what we did with the tools menu?

Comment 23

5 years ago
(In reply to Wesley Johnston (:wesj) from comment #22)
> brian, one last thing. Can you add an id to this in browser.js so that its
> possible for addons to extend this menu? Something like what we did with the
> tools menu?

And update the docs here!
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/NativeWindow/menu#Attributes
(Assignee)

Updated

5 years ago
Depends on: 975267
(Assignee)

Comment 24

5 years ago
(In reply to Wesley Johnston (:wesj) from comment #22)
> brian, one last thing. Can you add an id to this in browser.js so that its
> possible for addons to extend this menu? Something like what we did with the
> tools menu?

I think I get the general idea looking at the tools submenu, but I'm not comfortable enough with the code to throw this in as a last-minute change, so I filed bug 975267 to take care of this.
(Assignee)

Comment 25

5 years ago
(In reply to Wesley Johnston (:wesj) from comment #21)
> Comment on attachment 8375730 [details] [diff] [review]
> Add submenu for bookmarks, v4
> 
> Review of attachment 8375730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +1983,5 @@
> >              share.setActionProvider(provider);
> >          }
> >  
> > +        menu.findItem(R.id.bookmark).setCheckable(true);
> > +        menu.findItem(R.id.reading_list_add).setCheckable(true);
> 
> This can be specified in the xml. Do you know why we have this in here at
> all?

Good question. Fixed.

https://hg.mozilla.org/integration/fx-team/rev/500650a9eb75
https://hg.mozilla.org/integration/fx-team/rev/781da8091e21
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/500650a9eb75
https://hg.mozilla.org/mozilla-central/rev/781da8091e21
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30

Updated

5 years ago
Flags: in-moztrap?(fennec)

Updated

5 years ago
Depends on: 976006

Updated

5 years ago
Depends on: 976033

Updated

5 years ago
Depends on: 977669
Depends on: 984677
(Assignee)

Updated

5 years ago
Depends on: 979545
(Assignee)

Updated

5 years ago
Depends on: 987722
You need to log in before you can comment on or make changes to this bug.