739355 - [TABLET] Title Bar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: Mockup

Let's make an awesome title bar for tablets that uses our new design, and includes

* URL Bar with Title, Favicon
* Back button
* Progress indicator (spinner + bar)
* Conditional forward button
* SSL / EV indicators
* Addons indicators
* Bookmark Page
* Refresh
* Stop
* Overflow Menu 

A mockup is attached. More detailed specs and assets will follow soon.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

-ing to keep triage lists clean

Comment 2

[Ian Barlow (:ibarlow)]

Attachment: Specs / measurements for Tablet Title Bar

On tablets, the Title Bar is always visible at the top portion of the screen.

Back and Forward
Firefox Mobile uses Android’s dedicated back button to navigate back to the previous page. On tablets, there is also a Back button in the header, close to the rest of the UI controls. A Forward Button appears beside the Back Button only when there’s a page to go forward to. 

URL Bar
When a page is displayed, the Address Bar contains its favicon and URL. Tapping on the site’s favicon displays any information provided by the website to verify its authenticity and security. A background colour behind the favicon also indicates the site's security.

Tapping the URL bar transitions the UI into AwesomeBar mode (bug 739364, bug 739365, bug 739366, but 739369)

Additional Title Bar Features
Primary actions like Bookmark and Refresh are kept in the Title Bar, and the rest are contained within the Overflow Menu

Comment 3

[Ian Barlow (:ibarlow)]

Attachment: UI Specs

Reposting this, as the previous one was missing a few details

Comment 4

[Eitan Isaacson [:eeejay]]

Could we also get a focused state for buttons in the spec?

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (1/3): Stylize the overflow menu

This patch adds a overflow menu only for tablets. Phones and Galaxy Nexus are left intact. This also replaces icons, adds backgrounds as per need.

There is a bug in android that doesnt allow action-bar icons. Though I've specified the property in XML properly, the android source code doesn't seem to use it. Hence, we cannot do anything about it. Only the menu button takes the background property properly.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2/3): Layout for tablet

This patch adds a layout for the tablet menu. The items are placed as per the spec. I've modified few places in code to take care of back and forward.

Comment 7

[Sriram Ramasubramanian [:sriram]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #6)
> Created attachment 626677 [details] [diff] [review]
> Patch (2/3): Layout for tablet
> 
> This patch adds a layout for the tablet menu. The items are placed as per
> the spec. I've modified few places in code to take care of back and forward.

This patch does not stylize the url-bar with orange color. I will post a separate patch for the same, as it involves adding 9 png (18 of them! :( ) and making sure it works fine in all devices.

Conditional forward and SSL indicators could be taken as a followup patch, I guess.

Comment 8

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

Comment on attachment 626676 [details] [diff] [review]
Patch (1/3): Stylize the overflow menu

r+ but a few nits:
* the amount of images and the affect on the APK worries me. we should remove any unused images and try to reduce the size of any newly added images.
* "sw600dp" is specifically for 7" tablets? if so, we should make some comments in the XML files and Makefile.in so we remember.

Comment 9

[Eitan Isaacson [:eeejay]]

Comment on attachment 626677 [details] [diff] [review]
Patch (2/3): Layout for tablet

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

::: mobile/android/base/resources/layout-land-v14/browser_toolbar.xml
@@ +14,5 @@
> +        <ImageButton android:id="@+id/back"
> +                     style="@style/AddressBar.ImageButton.Unused"/>
> +
> +        <ImageButton android:id="@+id/forward"
> +                     style="@style/AddressBar.ImageButton.Unused"/>

Could you please provide text descriptions in all new image buttons you introduce? "Back" and "Forward" are perfect here.

http://developer.android.com/guide/topics/ui/accessibility/apps.html#label-ui

Comment 10

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

Comment on attachment 626677 [details] [diff] [review]
Patch (2/3): Layout for tablet

>diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java

>-                if (Build.VERSION.SDK_INT >= 11) {
>-                    if (GeckoApp.mOrientation == Configuration.ORIENTATION_PORTRAIT)
>-                        text.setTextSize(24);
>-                    else
>-                        text.setTextSize(20);
>+                if (Build.VERSION.SDK_INT >= 14) {
>+                    if (!GeckoApp.mAppContext.isTablet()) {
>+                        if (GeckoApp.mOrientation == Configuration.ORIENTATION_PORTRAIT)
>+                            text.setTextSize(24);
>+                        else
>+                            text.setTextSize(20);
>+                    } else {
>+                        text.setTextSize(26);
>+                    }
>+                } else if (Build.VERSION.SDK_INT >= 11) {
>+                    text.setTextSize(24);
>                 } else {
>-                    text.setTextSize(22);
>+                    text.setTextSize(24);

Why change from 22 to 24 in this condition?

>                 }

Can't we put this textsize in a values.xml file and then we wouldn't need this Java code?

>+        mBack = (ImageButton) mLayout.findViewById(R.id.back);
>+        mBack.setOnClickListener(new Button.OnClickListener() {
>+            public void onClick(View view) {
>+                Tabs.getInstance().getSelectedTab().doBack();
>+            }
>+        });
>+
>+        mForward = (ImageButton) mLayout.findViewById(R.id.forward);
>+        mForward.setOnClickListener(new Button.OnClickListener() {
>+            public void onClick(View view) {
>+                Tabs.getInstance().getSelectedTab().doForward();
>+            }
>+        });
>+

I almost feel like adding a Tablet specific class to handle any overflow, unless we can make it very generic. I mean, mBack and mForward is OK, but what if we need to add 3 more buttons?

>+    public void updateBackButton(boolean enabled) {
>+         mBack.setColorFilter(enabled ? 0 : 0xFF999999);
>+         mBack.setEnabled(enabled);
>+    }
>+
>+    public void updateForwardButton(boolean enabled) {
>+         mForward.setColorFilter(enabled ? 0 : 0xFF999999);
>+         mForward.setEnabled(enabled);
>+    }

Do we have to hardcode these?

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>         mMainHandler.post(new Runnable() {
>             public void run() {
>                 if (Tabs.getInstance().isSelectedTab(tab)) {
>                     mBrowserToolbar.setSecurityMode(tab.getSecurityMode());
>+                    mBrowserToolbar.updateBackButton(tab.canDoBack());
>+                    mBrowserToolbar.updateForwardButton(tab.canDoForward());
>                     if (showProgress && tab.getState() == Tab.STATE_LOADING)
>                         mBrowserToolbar.setProgressVisibility(true);

Why not call refresh() here? Because of "showProgesss" ? I suppose we can look at that later. Brian might have a patch to fix it.

r+, but I'd like to move away from the Java code and use resources whenever possible. Also, I don't see some of the code from bug 730775 being removed since we don't need to worry about the urlbar corruption. File new bugs.

Comment 11

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 626676 [details] [diff] [review]
> Patch (1/3): Stylize the overflow menu
> 
> r+ but a few nits:
> * the amount of images and the affect on the APK worries me. we should
> remove any unused images and try to reduce the size of any newly added
> images.
> * "sw600dp" is specifically for 7" tablets? if so, we should make some
> comments in the XML files and Makefile.in so we remember.

I am happy to add this, but this is going to break my patch queue. :(
I will add this as a part of last patch in my queue that hits the Makefile.in

Comment 12

[Sriram Ramasubramanian [:sriram]]

(In reply to Eitan Isaacson [:eeejay] from comment #9)
> Comment on attachment 626677 [details] [diff] [review]
> Patch (2/3): Layout for tablet
> 
> Review of attachment 626677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/layout-land-v14/browser_toolbar.xml
> @@ +14,5 @@
> > +        <ImageButton android:id="@+id/back"
> > +                     style="@style/AddressBar.ImageButton.Unused"/>
> > +
> > +        <ImageButton android:id="@+id/forward"
> > +                     style="@style/AddressBar.ImageButton.Unused"/>
> 
> Could you please provide text descriptions in all new image buttons you
> introduce? "Back" and "Forward" are perfect here.
> 
> http://developer.android.com/guide/topics/ui/accessibility/apps.html#label-ui

These are pulled from menu. Forward doesn't have a content-description there and back used to be android's back button. This would involve string change, and I am scared to do it. Could you please file a followup?
Also, these aren't visible to the user, and these are phone specific. Do we need contentDescription for invisible content?

Comment 13

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> Comment on attachment 626677 [details] [diff] [review]
> Patch (2/3): Layout for tablet
> 
> >diff --git a/mobile/android/base/BrowserToolbar.java b/mobile/android/base/BrowserToolbar.java
> 
> >-                if (Build.VERSION.SDK_INT >= 11) {
> >-                    if (GeckoApp.mOrientation == Configuration.ORIENTATION_PORTRAIT)
> >-                        text.setTextSize(24);
> >-                    else
> >-                        text.setTextSize(20);
> >+                if (Build.VERSION.SDK_INT >= 14) {
> >+                    if (!GeckoApp.mAppContext.isTablet()) {
> >+                        if (GeckoApp.mOrientation == Configuration.ORIENTATION_PORTRAIT)
> >+                            text.setTextSize(24);
> >+                        else
> >+                            text.setTextSize(20);
> >+                    } else {
> >+                        text.setTextSize(26);
> >+                    }
> >+                } else if (Build.VERSION.SDK_INT >= 11) {
> >+                    text.setTextSize(24);
> >                 } else {
> >-                    text.setTextSize(22);
> >+                    text.setTextSize(24);
> 
> Why change from 22 to 24 in this condition?

Ah, I should be 22. I'll change it back. Copy-paste error. :D

> 
> >                 }
> 
> Can't we put this textsize in a values.xml file and then we wouldn't need
> this Java code?

We can have it in XML. There are 2 ways.
1. add "dimens.xml" into folders values/ , values-xlarge/, values-v14/ , values-land-v14.
Get it from resources everytime and use it.
2. Create 4 different layout XMLs, have it for pre-honeycomb, ics-landscape, ics-portrait and tablets and inflate it.

Is that a followup patch?

> 
> >+        mBack = (ImageButton) mLayout.findViewById(R.id.back);
> >+        mBack.setOnClickListener(new Button.OnClickListener() {
> >+            public void onClick(View view) {
> >+                Tabs.getInstance().getSelectedTab().doBack();
> >+            }
> >+        });
> >+
> >+        mForward = (ImageButton) mLayout.findViewById(R.id.forward);
> >+        mForward.setOnClickListener(new Button.OnClickListener() {
> >+            public void onClick(View view) {
> >+                Tabs.getInstance().getSelectedTab().doForward();
> >+            }
> >+        });
> >+
> 
> I almost feel like adding a Tablet specific class to handle any overflow,
> unless we can make it very generic. I mean, mBack and mForward is OK, but
> what if we need to add 3 more buttons?

I am happy with moving them into Tablet specific class. However, they should all be specified in XML (as dummys), so that we do end up crashing. Tablet specific component can be an inner class inside BrowserToolbar, thereby everything is intact. If this approach is fine, I'll do a cleanup as a followup.

> 
> >+    public void updateBackButton(boolean enabled) {
> >+         mBack.setColorFilter(enabled ? 0 : 0xFF999999);
> >+         mBack.setEnabled(enabled);
> >+    }
> >+
> >+    public void updateForwardButton(boolean enabled) {
> >+         mForward.setColorFilter(enabled ? 0 : 0xFF999999);
> >+         mForward.setEnabled(enabled);
> >+    }
> 
> Do we have to hardcode these?

Yup. This is the best way and the easiest way. It's just a color value. 0 - transparent and the other is #999999. I didn't feel like creating a member variable and using it. Also, with conditional forward button, that filter will go away, leaving only back button.
A better approach is to put it in colors.xml, use resources, get it everytime and use it. (getResources().getColor(R.color.disableButtonFilter); ) If we are caching this in a member variable, I would say, 0xFF999999 is better.


Regarding url-bar corruption, the background of overflow menu items are set from action-bar as android doesn't allow backgrounds for those items. Hence, we need to have a background for action-bar, unless we do a custom menu in action-bar.

Comment 14

[Eitan Isaacson [:eeejay]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #12)
> (In reply to Eitan Isaacson [:eeejay] from comment #9)
> > Comment on attachment 626677 [details] [diff] [review]
> > Patch (2/3): Layout for tablet
> > 
> > Review of attachment 626677 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/resources/layout-land-v14/browser_toolbar.xml
> > @@ +14,5 @@
> > > +        <ImageButton android:id="@+id/back"
> > > +                     style="@style/AddressBar.ImageButton.Unused"/>
> > > +
> > > +        <ImageButton android:id="@+id/forward"
> > > +                     style="@style/AddressBar.ImageButton.Unused"/>
> > 
> > Could you please provide text descriptions in all new image buttons you
> > introduce? "Back" and "Forward" are perfect here.
> > 
> > http://developer.android.com/guide/topics/ui/accessibility/apps.html#label-ui
> 
> These are pulled from menu. Forward doesn't have a content-description there
> and back used to be android's back button.

They didn't have descriptions because they have android:title. One of them is needed, not both.

> This would involve string change,
> and I am scared to do it. Could you please file a followup?

No string change necessary for forward, just use the same string that is in the menu: "@string/forward". As for back, I didn't know we were in a string freeze.

> Also, these aren't visible to the user, and these are phone specific. Do we
> need contentDescription for invisible content?

Not sure what you mean by not visible. I guess I just wanted to give a friendly reminder to please give text alternatives to all new UI that is introduced. I see a whole bunch of other ImageButtons in this patch, please use your judgement as to what the user sees and what needs to get labelled.

Comment 15

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6c7b3e86fb8a
http://hg.mozilla.org/integration/mozilla-inbound/rev/7b8ccf1b854e

Comment 16

[Sriram Ramasubramanian [:sriram]]

(In reply to Eitan Isaacson [:eeejay] from comment #14)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #12)
> > (In reply to Eitan Isaacson [:eeejay] from comment #9)
> > > Comment on attachment 626677 [details] [diff] [review]
> > > Patch (2/3): Layout for tablet
> > > 
> > > Review of attachment 626677 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: mobile/android/base/resources/layout-land-v14/browser_toolbar.xml
> > > @@ +14,5 @@
> > > > +        <ImageButton android:id="@+id/back"
> > > > +                     style="@style/AddressBar.ImageButton.Unused"/>
> > > > +
> > > > +        <ImageButton android:id="@+id/forward"
> > > > +                     style="@style/AddressBar.ImageButton.Unused"/>
> > > 
> > > Could you please provide text descriptions in all new image buttons you
> > > introduce? "Back" and "Forward" are perfect here.
> > > 
> > > http://developer.android.com/guide/topics/ui/accessibility/apps.html#label-ui
> > 
> > These are pulled from menu. Forward doesn't have a content-description there
> > and back used to be android's back button.
> 
> They didn't have descriptions because they have android:title. One of them
> is needed, not both.
> 
> > This would involve string change,
> > and I am scared to do it. Could you please file a followup?
> 
> No string change necessary for forward, just use the same string that is in
> the menu: "@string/forward". As for back, I didn't know we were in a string
> freeze.
> 
> > Also, these aren't visible to the user, and these are phone specific. Do we
> > need contentDescription for invisible content?
> 
> Not sure what you mean by not visible. I guess I just wanted to give a
> friendly reminder to please give text alternatives to all new UI that is
> introduced. I see a whole bunch of other ImageButtons in this patch, please
> use your judgement as to what the user sees and what needs to get labelled.

Oh sure, I'll use "@string/forward".
I have added a back button in this patch and will be adding a menu button soon.
The accessibility strings are needed for views that are visible (or might turn visible over the course of use of the application) and that have some "actions" with it right?
Some of my patch would have "spacer" views, just to space elements properly.

I'll file a new bug for contentDescription for tablet UI.

Comment 17

[Sriram Ramasubramanian [:sriram]]

Filed bug 758431 for accessibility strings.

Comment 19

[Sriram Ramasubramanian [:sriram]]

These patches might require clobbering.

Comment 20

[Eitan Isaacson [:eeejay]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #16)
> The accessibility strings are needed for views that are visible (or might
> turn visible over the course of use of the application) and that have some
> "actions" with it right?
> Some of my patch would have "spacer" views, just to space elements properly.
> 

If it is focusable, it should have a description. Typically, only interesting/actionable items are focusable. If you could navigate to a spacer with the dpad then that is a separate problem.

> I'll file a new bug for contentDescription for tablet UI.

Thanks!

Comment 21

[Sriram Ramasubramanian [:sriram]]

(In reply to Eitan Isaacson [:eeejay] from comment #20)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #16)
> > The accessibility strings are needed for views that are visible (or might
> > turn visible over the course of use of the application) and that have some
> > "actions" with it right?
> > Some of my patch would have "spacer" views, just to space elements properly.
> > 
> 
> If it is focusable, it should have a description. Typically, only
> interesting/actionable items are focusable. If you could navigate to a
> spacer with the dpad then that is a separate problem.
> 
> > I'll file a new bug for contentDescription for tablet UI.
> 
> Thanks!


https://bugzilla.mozilla.org/attachment.cgi?id=626677&action=diff#a/mobile/android/base/resources/layout/browser_toolbar.xml_sec1 
These buttons are added only for phones. Their size is 0x0. I am not sure if they are focusable, but they are never visible for phones. Do they need accessibility strings?

Comment 22

[Eitan Isaacson [:eeejay]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #21)
> (In reply to Eitan Isaacson [:eeejay] from comment #20)
> > (In reply to Sriram Ramasubramanian [:sriram] from comment #16)
> > > The accessibility strings are needed for views that are visible (or might
> > > turn visible over the course of use of the application) and that have some
> > > "actions" with it right?
> > > Some of my patch would have "spacer" views, just to space elements properly.
> > > 
> > 
> > If it is focusable, it should have a description. Typically, only
> > interesting/actionable items are focusable. If you could navigate to a
> > spacer with the dpad then that is a separate problem.
> > 
> > > I'll file a new bug for contentDescription for tablet UI.
> > 
> > Thanks!
> 
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=626677&action=diff#a/mobile/
> android/base/resources/layout/browser_toolbar.xml_sec1 
> These buttons are added only for phones. Their size is 0x0. I am not sure if
> they are focusable, but they are never visible for phones. Do they need
> accessibility strings?

Ooh, they are stubs so the common Java code doesn't break? No, if they are not visible they don't need strings. But it is worth testing if they are focusable, because if they are that is bad.

Comment 23

[Sriram Ramasubramanian [:sriram]]

How do I test if they are focusable?
I've filed a bug 758431 only for tablet UI, where there are visible.

Comment 24

[Eitan Isaacson [:eeejay]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #23)
> How do I test if they are focusable?
> I've filed a bug 758431 only for tablet UI, where there are visible.

Quickest way is to pair a bluetooth keyboard and arrow around. There might be issues with focus not being visible like i said in comment #4. So you may have to enable TalkBack in the system accessibility settings and hear what it speaks.

Comment 25

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2.5/3): Revert text-size in phones.

I forgot to make this change while addressing the review comments. :(

Comment 26

[Wesley Johnston (:wesj)]

I hate magic numbers! (even though I use them far to often)

Comment 27

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e76794008819

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e76794008819

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6c7b3e86fb8a
https://hg.mozilla.org/mozilla-central/rev/7b8ccf1b854e

Comment 30

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This seems to break mine and mfinkle's builds -- it uses @android:color/transparent, which is only available in SDK 14 or higher.  Our build docs say 13; we should either switch to 14 in all docs, or handle this differently...

Comment 31

[Sriram Ramasubramanian [:sriram]]

http://developer.android.com/reference/android/graphics/Color.html#TRANSPARENT
It seems to be available since API level 1. And I remember using this in many other places.

Comment 32

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/mozilla-central/file/caea66e968bf/mobile/android/base/resources/values/styles.xml#l53
This particular piece of code hits every phone's UI starting level 5. And it works till date.

Comment 33

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

No, Color.TRANSPARENT is available, but whatever @android:color/transparent references is not.  finkle found the docs saying that it's API level 14, and indeed switching my build to 14 makes it work.  Since the tinderboxes are using 14, I just changed the build docs to say 14 instead. :)