612193 - Move Find In Page, Share Page, and Save As PDF to app menu on Android

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Find In Page, Share Page, and Save As PDF are currently located in the Site Options menu in Fennec.

On Android, the application menu (bug 606565) is a better place for these commands.  Logically, they are page actions (not "site options").  Practically, I expect they will be more discoverable if they are in the main app menu.  And finally, it will be more consistent with the default Android browser, and with the Android platform in general.

The new menu structure on Android would be:

App Menu:
Find In Page, Share Page, Site Options, Preferences, Add-ons, Downloads

Site Options:
identity panel, Forget Password, Reset Permissions, Add Search Engine

Adding these two options (Find, Share) would bring the number of app menu items to six.  This is maximum that can appear in the main Android-style menu.  If we implement Save As PDF on Android (bug 595919), or if add-ons extend the app menu (see my "Full Screen" add-on for an example), then we will definitely need to implement the overflow UI (bug 610784).

One downside of this proposal is that we cannot use the same UI on Maemo (which lacks a system "Menu" button).  This would complicate documentation and code maintenance.  However, we already deal with difference in menu structure between desktop platforms, so I don't think this is a deal-breaker.

Comment 1

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

I'm not convinced moving these commands to the App menu is a good idea. The Site menu is a good place for page actions. The App menu is a good place for application actions.

Comment 2

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

(In reply to comment #1)
> I'm not convinced moving these commands to the App menu is a good idea. The
> Site menu is a good place for page actions. The App menu is a good place for
> application actions.

And now I have changed my mind. Need UX input too.

Comment 3

[Madhava Enros [:madhava]]

Cross platform is one issue here, but feels like it's going to be less of a concern for the next couple of releases; other platforms seem to increasingly have some standard "app menu" mechanism.

I think the division mbrubeck suggests makes sense, esp for things that are more browser actions like Find in Page and Save as PDF.  Share Page still straddles that line in my mind, but it's true that Android users will look in the Android menu first, and that's top-tier functionality.

This gives us some room back in the Site Menu for giving more granular access to things like the permissions you've given a site, and for the identity relationship you're managing with a site.

It would be great to avoid going to overflow, by default, in the Android menu, if we can think of some way to avoid that.

Comment 4

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

(In reply to comment #3)

> It would be great to avoid going to overflow, by default, in the Android menu,
> if we can think of some way to avoid that.

We plan to add a "More" mechanism to the Android menu

Comment 5

[Madhava Enros [:madhava]]

(In reply to comment #4)
> (In reply to comment #3)
> 
> > It would be great to avoid going to overflow, by default, in the Android menu,
> > if we can think of some way to avoid that.
> 
> We plan to add a "More" mechanism to the Android menu

Well, yes -- we'd need one, especially given that add-ons will extend the set no matter what we do.  If we had it by default, maybe it's "Downloads" that would get demoted - need to think about this a bit more.

Comment 6

[Matt Brubeck (:mbrubeck)]

If we do this, we'll need Android menu icons for Find In Page, Share Page, and Save As PDF.

Comment 7

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

(In reply to comment #6)
> If we do this, we'll need Android menu icons for Find In Page, Share Page, and
> Save As PDF.

If we decide to use icons for the "More" overflow. I don't think we have room for those to all appear on the main app menu.

Comment 8

[Ian Barlow (:ibarlow)]

Attachment: App menu icons

Attached icon sets, one for Gingerbread, one for Froyo :)

Comment 9

[Matt Brubeck (:mbrubeck)]

I think we still need an icon for "Save as PDF."

Comment 10

[Matt Brubeck (:mbrubeck)]

(In reply to comment #9)
> I think we still need an icon for "Save as PDF."

...or we can just put it last, so it will be in the "More" menu and does not need an icon.

Comment 11

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

(In reply to comment #10)
> (In reply to comment #9)
> > I think we still need an icon for "Save as PDF."
> 
> ...or we can just put it last, so it will be in the "More" menu and does not
> need an icon.

That is exactly what the current patch does. Just in case, I reuse the "download" image for it in CSS.

Comment 12

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

Attachment: patch

This patch does the simple stuff (adds the menu items to the overflow list) and some hard stuff (move the overflow list into it's own container)

* appmenu-*.png images have been updated in froyo and gingerbread
* 2 new appmenu-*.png images for findinpage and share were added to froyo and gingerbread
* AppMenuOverflow JS class was aded to AppMenu.js to handle the new overflow list
* XUL was added to browser.xul to host the overflow list
* appmenu-overflow items were added to browser.css in froyo and gingerbread to style the overflow list
* bumped the row height for popup menus, context menus and appmenu overflow items to be more inline with Android on froyo and gingerbread

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 535709 [details] [diff] [review]
patch

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

mobile/chrome/tests/browser_appmenu.js needs to be updated too.

::: mobile/themes/core/browser.css
@@ +1094,5 @@
>    -moz-border-left-colors: white;
>    border-style: solid;
>    border-width: @border_width_tiny@ !important;
> +  height: @touch_button_xlarge@;
> +  min-height: @touch_button_xlarge@;

This causes misalignment with the :hover:active background image for these elements in the Froyo theme.  (The easiest solution is to get a new background image that matches the larger size.)

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 535709 [details] [diff] [review]
patch

>+    let overflow = this.overflow;
>+    let listbox = overflow.lastChild;
>+    while (listbox.firstChild)
>+      listbox.removeChild(listbox.firstChild);

Any reason not to get the actual list here?

Comment 15

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

Attachment: patch for tests

This patch fixes the appmenu tests:
* adds .hidden where needed
* removes some menu items so we start with the more button hidden (adds the menu items back at end of test)
* uses the new element for the more menu list

Comment 16

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

Attachment: patch for tests v2

This patch also fixes the browser_find tests. Instead of always assuming Find in Page is in the Site Menu, the test now uses the App Menu, which is available on all platforms. Find in Page is not on the Site Menu for Android.

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 535902 [details] [diff] [review]
patch for tests v2

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

Comment 18

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

Comment on attachment 535709 [details] [diff] [review]
patch

low-risk, android only, been baking on trunk for a while and really helps with android integration

Comment 19

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

pushed:
http://hg.mozilla.org/mozilla-central/rev/aa1c53df226f
http://hg.mozilla.org/mozilla-central/rev/cd0fb5103ee9

Comment 20

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

noting that I had to push a followup for missing files:
http://hg.mozilla.org/mozilla-central/rev/aad412c52fab

Comment 21

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

http://hg.mozilla.org/releases/mozilla-aurora/rev/283bad4b1eca

Comment 22

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Verified : 
Mozilla/5.0 (Android; Linux armv71; rv6.0a2) Gecko/20110623 Firefox/6.0a2 Fennec/6.0a2
Device: Thunderbolt
OS: Android 2.2

Mozilla/5.0 (Android; Linux armv71; rv6.0a2) Gecko/20110623 Firefox/6.0a2 Fennec/6.0a2
Device: HTC Flyer
OS: Android 2.3