958517 - Create a 'Page' menu and move some of the URL context menu actions there

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

We currently use the URLBar context menu for "page" level actions, but it's too hard to discover and use. In addition, the actions are probably more important than a context menu implies.

We'd like to try making a "Page options" main menu item and move some of the URLBar context menu actions to it.

The current context menu is here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/menu/titlebar_contextmenu.xml

Let's move:
* subscribe
* add_search_engine
* site_settings
* add_to_launcher

Let's remove:
* share (since it's already on the main menu)

Let's keep:
* pasteandgo
* paste
* copyurl
* add_to_launcher (yes it's in both places)

Let's add the "Page options" menu above "Tools"

Comment 1

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

Wording change: "Page options" -> "Page"

Comment 2

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

Attachment: Make page menu v0.1

What do you guys think of this approach? Having the menu items on the context menu (gingerbread) and main menu (newer) makes the code a little ugly, but this works and is not too complicated.

I need to update some Robocop tests before landing too.

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8358600 [details] [diff] [review]
Make page menu v0.1

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

As per IRC discussion, maybe add a couple of util static methods (in a MenuUtils class?) that conditionally set a menu item enabled/visible depending on its availability. Something like:
- setMenuItemVisibleIfExists()
- setMenuItemEnabledIfExists()

And a comment somewhere explaining why the menuitem is not necessarily available. Bonus: write a test that confirms that menu items are in their expected spots :-)

::: mobile/android/base/resources/menu-large-v11/browser_app_menu.xml
@@ +46,5 @@
>            android:icon="@drawable/ic_menu_desktop_mode_off"
>            android:title="@string/desktop_mode"
>            android:checkable="true" />
>  
> +

nit: remove extra empty line

Comment 4

[Wesley Johnston (:wesj)]

Our menus feel really ad-hoc to me. Should Request Desktop site, Find in page, Character Encoding, or Save as PDF also be in here? Why/why not?

I've been trying to get a MenuHelper/Utils class in for a bit to help with the quick share code (i.e. consolidate the Share providers). "Helper" felt a bit better for what I was doing which included maintaining a HashTable with some info about menu items to show.

Comment 5

[Wesley Johnston (:wesj)]

Later patch, maybe we could even move those in there with a "quick" button for the most frequent/recently used action?

Comment 6

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

(In reply to Wesley Johnston (:wesj) from comment #4)
> Our menus feel really ad-hoc to me. Should Request Desktop site, Find in
> page, Character Encoding, or Save as PDF also be in here? Why/why not?

We need to get some telemetry on the menu actions (bug 958519) and determine if some menu actions are used enough to move to the top tevel, or not used enough and pushed onto the submenu.

I am fine with putting much used actions in the top level.

Comment 7

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

Attachment: Make page menu v0.2

Updated to use new MenuUtils.safeSetVisible and MenuUtils.safeSetEnabled methods.

I still need to look at what tests will be broken by this patch.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8360215 [details] [diff] [review]
Make page menu v0.2

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

I would still like to have a comment *somewhere* explaining why we need to use safeSetVisible/safeSetEnabled for only a subset of the menuitems. I assume ibarlow has given his f+?

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8360215 [details] [diff] [review]
Make page menu v0.2

Oops, changed the wrong review request flag.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 8360215 [details] [diff] [review]
Make page menu v0.2

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

This reminds me how much I hate that we have 3 different menus full of mostly repeated content (I yearn to bring the pre-processor back...). Other than that, the approach seems fine. What happens on < v11 devices?

::: mobile/android/base/BrowserApp.java
@@ +2098,5 @@
> +            MenuUtils.safeSetEnabled(aMenu, R.id.page, false);
> +            MenuUtils.safeSetEnabled(aMenu, R.id.subscribe, false);
> +            MenuUtils.safeSetEnabled(aMenu, R.id.add_search_engine, false);
> +            MenuUtils.safeSetEnabled(aMenu, R.id.site_settings, false);
> +            MenuUtils.safeSetEnabled(aMenu, R.id.add_to_launcher, false);

Is just disabling page not enough?

Comment 11

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

(In reply to Wesley Johnston (:wesj) from comment #10)
> Comment on attachment 8360215 [details] [diff] [review]
> Make page menu v0.2
> 
> Review of attachment 8360215 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This reminds me how much I hate that we have 3 different menus full of
> mostly repeated content (I yearn to bring the pre-processor back...). Other
> than that, the approach seems fine. What happens on < v11 devices?
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +2098,5 @@
> > +            MenuUtils.safeSetEnabled(aMenu, R.id.page, false);
> > +            MenuUtils.safeSetEnabled(aMenu, R.id.subscribe, false);
> > +            MenuUtils.safeSetEnabled(aMenu, R.id.add_search_engine, false);
> > +            MenuUtils.safeSetEnabled(aMenu, R.id.site_settings, false);
> > +            MenuUtils.safeSetEnabled(aMenu, R.id.add_to_launcher, false);
> 
> Is just disabling page not enough?

I am just being overly explicit. I was worried someone might check the state of the menu actions themselves.

(In reply to Lucas Rocha (:lucasr) from comment #8)

> I would still like to have a comment *somewhere* explaining why we need to
> use safeSetVisible/safeSetEnabled for only a subset of the menuitems. I
> assume ibarlow has given his f+?

Comments added. Ian has f+'d.

Comment 12

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

Attachment: pageload-trace.patch

Updated

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8372283 [details] [diff] [review]
pageload-trace.patch

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

Wrong patch? :-)

Comment 14

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

Attachment: make-page-menu

Here we go!

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 8372290 [details] [diff] [review]
make-page-menu

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

Go!

::: mobile/android/base/util/MenuUtils.java
@@ +11,5 @@
> +public class MenuUtils {
> +    /*
> +     * This method looks for a menuitem and sets it's visible state, if
> +     * it exists.
> +    */

nit: align the last * with the ones on top

@@ +22,5 @@
> +
> +    /*
> +     * This method looks for a menuitem and sets it's enabled state, if
> +     * it exists.
> +    */

Ditto.

Comment 16

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

Attachment: pagemenu-test v0.1

Simple fix needed for a test

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 8372353 [details] [diff] [review]
pagemenu-test v0.1

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

Wow, that was simple indeed. All green on try?

Comment 18

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

(In reply to Lucas Rocha (:lucasr) from comment #17)
> Comment on attachment 8372353 [details] [diff] [review]
> pagemenu-test v0.1
> 
> Review of attachment 8372353 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wow, that was simple indeed. All green on try?

Green on local build: Nexus S (Gingerbread) and Galaxy Nexus (Jellybean)

Comment 19

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/36ba4edc2416
remote:   https://hg.mozilla.org/integration/fx-team/rev/95bb0412c878

Comment 20

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

Attachment: fennec-page-menu.png

screenshot

Comment 21

[Aaron Train [:aaronmt]]

Why the change from 'site' to 'page'?

Comment 22

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

(In reply to Aaron Train [:aaronmt] from comment #21)
> Why the change from 'site' to 'page'?

No "Site" text was changed in the making of this menu. "Page" is what you are looking at. "Site"' is more generic and usually based on the domain. In fact, the "Site Settings" are not for the "Page" but the "Site".

"Edit Site Settings" is an odd ball no matter where it is located. The other actions on the "Page" menu (and the Toolbar context menu) are for the current page.

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/36ba4edc2416
https://hg.mozilla.org/mozilla-central/rev/95bb0412c878

Comment 24

[Ioana Chiorean]

Moztrap: 
Suite: https://moztrap.mozilla.org/manage/cases/?filter-suite=704
TC added: https://moztrap.mozilla.org/manage/case/13913/