768083 - Custom Menu, stage 2

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: mockup

Now that we have a custom menu, let's start taking advantage of that by adding in the next set of UI enhancements that have been waiting in the pipeline:

1. Special layout / larger icons for the first three page actions (Back, Forward, Reload)

2. Add a heavy divider line between page actions and other sections of the browser

3. Rounded corners!

Comment 1

[Aaron Train [:aaronmt]]

15? 16?

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Attachment: WIP patch to re-format menu items.

Here's a WIP patch to give us a big back button.  It doesn't crash tablets anymore thanks to Sriram's help, although I suspect there are still a few things to fix before it's ready to land.

(If anyone wants to take this patch, and drive it forward, please feel free to!  :)

Thanks,
Blake.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 670140 [details] [diff] [review]
WIP patch to re-format menu items.

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

There are way too many white space problems. Please fix them.

if (tab.isBookmark()) {
This is not needed here. Please remove.

public Divider(Context context, AttributeSet attrs, RelativeLayout.LayoutParams params) {
This is not one of the recommended constructor styles for views. Please use (context, attrs) format, so that, in case we want to inflate it from XML, it would work fine -- without crashes.

setLayoutParams(params);
This can be made default and set.

        public void addActionItem(View actionItem) {
            LinearLayout.LayoutParams params = new LinearLayout.LayoutParams(actionItem.getLayoutParams());
            RelativeLayout.LayoutParams params = new RelativeLayout.LayoutParams(actionItem.getLayoutParams());
            params.weight = 1.0f;
            RelativeLayout.LayoutParams dividerParams = new RelativeLayout.LayoutParams(1, 1);
            ....
This approach is highly not recommended. Please construct the 3-button layout in XML, and inflate it. We can show hide the Action-Bar, and set the icons there. That's the best way (both from readability of code standpoint, and the speed of rendering).

 	
<!-- android:layout_height="@dimen/browser_toolbar_height" -->
Comment not needed.


    <item android:id="@+id/reload"
          android:icon="@drawable/ic_menu_reload"
          android:title="@string/reload"
          android:showAsAction="always"/>
This will fail on Nexus 7. I don't think we support two action-items in the url-bar on Nexus 7.

Changing the ordering of reload in xlarge-v11 will change the order of icons shown in url-bar. UX wanted a different order. Please don't change that.

And, this will crash on Gingerbread phones. menu/browser_app_menu.xml.in requires a "back" button too!

f- for white spaces, XML inflation, and GB crash.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Attachment: WIP patch, v2.

> There are way too many white space problems. Please fix them.

Fixed.

> if (tab.isBookmark()) {
> This is not needed here. Please remove.

Reverted.

> public Divider(Context context, AttributeSet attrs,
> RelativeLayout.LayoutParams params) {
> This is not one of the recommended constructor styles for views. Please use
> (context, attrs) format, so that, in case we want to inflate it from XML, it
> would work fine -- without crashes.

Fixed.

> setLayoutParams(params);
> This can be made default and set.

Fixed.

> <!-- android:layout_height="@dimen/browser_toolbar_height" -->
> Comment not needed.

Fixed.

> Changing the ordering of reload in xlarge-v11 will change the order of icons
> shown in url-bar. UX wanted a different order. Please don't change that.

Reverted.

> And, this will crash on Gingerbread phones. menu/browser_app_menu.xml.in
> requires a "back" button too!

Fixed.

>         public void addActionItem(View actionItem) {
>             LinearLayout.LayoutParams params = new
> LinearLayout.LayoutParams(actionItem.getLayoutParams());
>             RelativeLayout.LayoutParams params = new
> RelativeLayout.LayoutParams(actionItem.getLayoutParams());
>             params.weight = 1.0f;
>             RelativeLayout.LayoutParams dividerParams = new
> RelativeLayout.LayoutParams(1, 1);
>             ....
> This approach is highly not recommended. Please construct the 3-button
> layout in XML, and inflate it. We can show hide the Action-Bar, and set the
> icons there. That's the best way (both from readability of code standpoint,
> and the speed of rendering).

I didn't fix this.  Could you perhaps give me a hand, by adding a skeleton layout to this patch, which I'll happily hack on to get displaying the way Ian has suggested?

>     <item android:id="@+id/reload"
>           android:icon="@drawable/ic_menu_reload"
>           android:title="@string/reload"
>           android:showAsAction="always"/>
> This will fail on Nexus 7. I don't think we support two action-items in the
> url-bar on Nexus 7.

I don't think I've changed that.  Can you explain it a little more?

Thanks,
Blake.

Comment 5

[Paul [pwd]]

The back button is by-and-large redundant seeing as every Android device ships with a back button (hard or soft). To make it extra large has little merit outside of aesthetics. Bug 454880 will ensure that the back button provides all of the functionality that desktop back buttons provides anyway. Also the forward button has been relegated from the primary UI on desktop due to it's lack of use.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This adds a three-button menu. The layout is specified in XML, and the containers are referred to in Java (instead of constructing everything in Java).

The menu cannot support "action-bar" for submenus -- as a normal LinearLayout. In case we need it, we might be able to support by providing a special privilege to the top level menu -- to preserve this 3-button menu.

Comment 7

[Brian Nicholson (:bnicholson)]

Are we going to have a different menu design for phones with hardware menu/back buttons? These phones won't show a soft menu button at the top right, as shown in the mockup. Users have to reach down to the bottom of the phone to make the menu appear, at which point they would just hit the back button if they need to go back. If we don't have a separate menu design, we'll be consuming valuable menu space with an oversized back button that is useless on these devices.

The most popular Android phones are the Samsung Galaxy S, SII, SIII, and Note - all of these have hardware menu/back buttons.

Comment 8

[Aaron Train [:aaronmt]]

This still necessary with all the recent custom menu changes?

Comment 9

[Ian Barlow (:ibarlow)]

Yes

Comment 10

[Lucas Rocha (:lucasr)]

Too many undefined bits on the design. Removing from hackathon list.

Comment 11

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

Comment on attachment 678401 [details] [diff] [review]
Patch

I think we need to re-evaluate what we're doing here. Even if we decide to move ahead, we need a new patch.

Comment 12

[Aaron Train [:aaronmt]]

wontfix?

Comment 13

[Ian Barlow (:ibarlow)]

bai!

Comment 14

[Sriram Ramasubramanian [:sriram]]

I cannot be more happier!

Comment 15

[Ian Barlow (:ibarlow)]

Get back to work, you