Closed Bug 739355 Opened 8 years ago Closed 8 years ago

[TABLET] Title Bar

Categories

(Firefox for Android :: General, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 15
Tracking Status
blocking-fennec1.0 --- -
fennec 14+ ---

People

(Reporter: ibarlow, Assigned: sriram)

References

Details

(Whiteboard: [tablet])

Attachments

(5 files, 1 obsolete file)

Attached image 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.
-ing to keep triage lists clean
blocking-fennec1.0: --- → -
tracking-fennec: --- → 14+
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
Attached image UI Specs
Reposting this, as the previous one was missing a few details
Attachment #610154 - Attachment is obsolete: true
Could we also get a focused state for buttons in the spec?
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.
Attachment #626676 - Flags: review?(mark.finkle)
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.
Attachment #626677 - Flags: review?(mark.finkle)
(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.
Attachment #626676 - Attachment description: Patch (1/2): Stylize the overflow menu → Patch (1/3): Stylize the overflow menu
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.
Attachment #626676 - Flags: review?(mark.finkle) → review+
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 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.
Attachment #626677 - Flags: review?(mark.finkle) → review+
(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
(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?
(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.
(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.
(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.
Filed bug 758431 for accessibility strings.
Duplicate of this bug: 758375
These patches might require clobbering.
(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!
(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?
(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.
How do I test if they are focusable?
I've filed a bug 758431 only for tablet UI, where there are visible.
(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.
I forgot to make this change while addressing the review comments. :(
Attachment #627030 - Attachment description: Patch (2/3): Revert text-size in phones. → Patch (2.5/3): Revert text-size in phones.
Attachment #627030 - Flags: review?(mark.finkle)
I hate magic numbers! (even though I use them far to often)
Attachment #627030 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/e76794008819
Assignee: nobody → sriram
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
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...
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.
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.
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. :)
Duplicate of this bug: 758978
Duplicate of this bug: 758974
Whiteboard: [tablet]
You need to log in before you can comment on or make changes to this bug.