Closed
Bug 739355
Opened 13 years ago
Closed 13 years ago
[TABLET] Title Bar
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(blocking-fennec1.0 -, fennec14+)
RESOLVED
FIXED
Firefox 15
People
(Reporter: ibarlow, Assigned: sriram)
References
Details
(Whiteboard: [tablet])
Attachments
(5 files, 1 obsolete file)
1.07 MB,
image/png
|
Details | |
367.56 KB,
image/png
|
Details | |
81.51 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
360.53 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
1.11 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•13 years ago
|
tracking-fennec: --- → 14+
Reporter | ||
Comment 2•13 years ago
|
||
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
Reporter | ||
Updated•13 years ago
|
Attachment #610154 -
Attachment is obsolete: true
Assignee | ||
Comment 5•13 years ago
|
||
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)
Assignee | ||
Comment 6•13 years ago
|
||
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)
Assignee | ||
Comment 7•13 years ago
|
||
(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.
Assignee | ||
Updated•13 years ago
|
Attachment #626676 -
Attachment description: Patch (1/2): Stylize the overflow menu → Patch (1/3): Stylize the overflow menu
Comment 8•13 years ago
|
||
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 9•13 years ago
|
||
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•13 years ago
|
||
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+
Assignee | ||
Comment 11•13 years ago
|
||
(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
Assignee | ||
Comment 12•13 years ago
|
||
(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?
Assignee | ||
Comment 13•13 years ago
|
||
(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•13 years ago
|
||
(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.
Assignee | ||
Comment 15•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/6c7b3e86fb8a http://hg.mozilla.org/integration/mozilla-inbound/rev/7b8ccf1b854e
Assignee | ||
Comment 16•13 years ago
|
||
(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 20•13 years ago
|
||
(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!
Assignee | ||
Comment 21•13 years ago
|
||
(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•13 years ago
|
||
(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.
Assignee | ||
Comment 23•13 years ago
|
||
How do I test if they are focusable? I've filed a bug 758431 only for tablet UI, where there are visible.
Comment 24•13 years ago
|
||
(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.
Assignee | ||
Comment 25•13 years ago
|
||
I forgot to make this change while addressing the review comments. :(
Assignee | ||
Updated•13 years ago
|
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)
Updated•13 years ago
|
Attachment #627030 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 27•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/e76794008819
Comment 28•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e76794008819
Assignee: nobody → sriram
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Comment 29•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6c7b3e86fb8a https://hg.mozilla.org/mozilla-central/rev/7b8ccf1b854e
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...
Assignee | ||
Comment 31•13 years ago
|
||
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.
Assignee | ||
Comment 32•13 years ago
|
||
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. :)
Updated•12 years ago
|
Whiteboard: [tablet]
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•