Closed Bug 958517 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox30 --- verified

People

(Reporter: mfinkle, Assigned: mfinkle)

Details

Attachments

(3 files, 3 obsolete files)

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"
Wording change: "Page options" -> "Page"
Attached patch Make page menu v0.1 (obsolete) — Splinter Review
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.
Assignee: nobody → mark.finkle
Attachment #8358600 - Flags: feedback?(wjohnston)
Attachment #8358600 - Flags: feedback?(lucasr.at.mozilla)
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
Attachment #8358600 - Flags: feedback?(lucasr.at.mozilla) → feedback+
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.
Later patch, maybe we could even move those in there with a "quick" button for the most frequent/recently used action?
(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.
Attached patch Make page menu v0.2 (obsolete) — Splinter Review
Updated to use new MenuUtils.safeSetVisible and MenuUtils.safeSetEnabled methods.

I still need to look at what tests will be broken by this patch.
Attachment #8358600 - Attachment is obsolete: true
Attachment #8358600 - Flags: feedback?(wjohnston)
Attachment #8360215 - Flags: feedback?(wjohnston)
Attachment #8360215 - Flags: feedback?(lucasr.at.mozilla)
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+?
Attachment #8360215 - Flags: feedback?(wjohnston) → feedback+
Comment on attachment 8360215 [details] [diff] [review]
Make page menu v0.2

Oops, changed the wrong review request flag.
Attachment #8360215 - Flags: feedback?(lucasr.at.mozilla) → feedback?(wjohnston)
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?
Attachment #8360215 - Flags: feedback?(wjohnston) → feedback+
(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.
Attached patch pageload-trace.patch (obsolete) — Splinter Review
Updated
Attachment #8360215 - Attachment is obsolete: true
Attachment #8372283 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8372283 [details] [diff] [review]
pageload-trace.patch

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

Wrong patch? :-)
Attachment #8372283 - Flags: review?(lucasr.at.mozilla)
Attached patch make-page-menuSplinter Review
Here we go!
Attachment #8372283 - Attachment is obsolete: true
Attachment #8372290 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8372290 - Flags: review?(lucasr.at.mozilla) → review+
Simple fix needed for a test
Attachment #8372353 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #8372353 - Flags: review?(lucasr.at.mozilla) → review+
(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)
Attached image fennec-page-menu.png
screenshot
Why the change from 'site' to 'page'?
(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.
https://hg.mozilla.org/mozilla-central/rev/36ba4edc2416
https://hg.mozilla.org/mozilla-central/rev/95bb0412c878
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Flags: in-moztrap?(fennec)
Status: RESOLVED → VERIFIED
OS: Linux → Android
Hardware: x86_64 → ARM
Moztrap: 
Suite: https://moztrap.mozilla.org/manage/cases/?filter-suite=704
TC added: https://moztrap.mozilla.org/manage/case/13913/
Flags: in-moztrap?(fennec) → in-moztrap+
You need to log in before you can comment on or make changes to this bug.