Closed Bug 768083 Opened 7 years ago Closed 6 years ago

Custom Menu, stage 2

Categories

(Firefox for Android :: General, defect)

15 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox16 --- affected
fennec + ---

People

(Reporter: ibarlow, Assigned: sriram)

Details

Attachments

(2 files, 2 obsolete files)

Attached image 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!
tracking-fennec: --- → ?
OS: Mac OS X → Android
Hardware: x86 → ARM
15? 16?
tracking-fennec: ? → 17+
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.
Attachment #670140 - Flags: feedback?(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.
Attachment #670140 - Flags: feedback?(sriram) → feedback-
Attached patch WIP patch, v2. (obsolete) — Splinter Review
> 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.
Attachment #670140 - Attachment is obsolete: true
Attachment #670150 - Flags: feedback?(sriram)
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.
Assignee: nobody → sriram
tracking-fennec: 17+ → 19+
Attached patch PatchSplinter Review
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.
Attachment #670150 - Attachment is obsolete: true
Attachment #670150 - Flags: feedback?(sriram)
Attachment #678401 - Flags: review?(mark.finkle)
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.
This still necessary with all the recent custom menu changes?
Yes
tracking-fennec: 19+ → 21+
tracking-fennec: 21+ → +
Whiteboard: ui-hackathon
Too many undefined bits on the design. Removing from hackathon list.
Whiteboard: ui-hackathon
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.
Attachment #678401 - Flags: review?(mark.finkle)
wontfix?
bai!
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
I cannot be more happier!
Get back to work, you
You need to log in before you can comment on or make changes to this bug.