965453 - Add a submenu for bookmarks

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: 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.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: 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?

Comment 2

[Lucas Rocha (:lucasr)]

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?

Comment 3

[Ian Barlow (:ibarlow)]

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

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Remove arrow from secondary submenu items

Removes the arrow on secondary action bar items in the menu.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: 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.

Comment 6

[Wesley Johnston (:wesj)]

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.

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: 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"?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 9

[Ian Barlow (:ibarlow)]

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

Comment 10

[Wesley Johnston (:wesj)]

(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 :)

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: 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.

Comment 12

[Brian Nicholson (:bnicholson)]

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

Sorry, forgot to remove a couple things from Tabs.

Comment 13

[Brian Nicholson (:bnicholson)]

Attachment: Add submenu for bookmarks, v3

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

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 15

[Brian Nicholson (:bnicholson)]

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

Comment 16

[Brian Nicholson (:bnicholson)]

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.

Comment 17

[Lucas Rocha (:lucasr)]

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?

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 19

[Ian Barlow (:ibarlow)]

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

Comment 20

[Brian Nicholson (:bnicholson)]

Attachment: Add submenu for bookmarks, v4

Updated to address above comments.

Comment 21

[Wesley Johnston (:wesj)]

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

Comment 22

[Wesley Johnston (:wesj)]

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

[:Margaret Leibovic]

(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

Comment 24

[Brian Nicholson (:bnicholson)]

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

Comment 25

[Brian Nicholson (:bnicholson)]

(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

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/500650a9eb75
https://hg.mozilla.org/mozilla-central/rev/781da8091e21