Status

()

Firefox for Android
General
RESOLVED WONTFIX
6 years ago
2 years ago

People

(Reporter: ibarlow, Assigned: sriram)

Tracking

15 Branch
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox16 affected, fennec+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 636381 [details]
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!

Updated

6 years ago
tracking-fennec: --- → ?
status-firefox16: --- → affected
OS: Mac OS X → Android
Hardware: x86 → ARM
15? 16?
tracking-fennec: ? → 17+
Created attachment 670140 [details] [diff] [review]
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.
Attachment #670140 - Flags: feedback?(sriram)
(Assignee)

Comment 3

5 years ago
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-
Created attachment 670150 [details] [diff] [review]
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.
Attachment #670140 - Attachment is obsolete: true
Attachment #670150 - Flags: feedback?(sriram)

Comment 5

5 years ago
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+
(Assignee)

Comment 6

5 years ago
Created attachment 678401 [details] [diff] [review]
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.
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?
(Reporter)

Comment 9

5 years ago
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?
(Reporter)

Comment 13

4 years ago
bai!
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 14

4 years ago
I cannot be more happier!
(Reporter)

Comment 15

4 years ago
Get back to work, you
You need to log in before you can comment on or make changes to this bug.