Closed Bug 1066253 Opened 8 years ago Closed 7 years ago

Display favicon in tab strip instead of toolbar in new tablet UI

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: lucasr, Assigned: mcomella)

References

Details

Attachments

(5 files, 6 obsolete files)

5.67 KB, patch
lucasr
: review+
lucasr
: feedback+
Details | Diff | Splinter Review
1.77 KB, application/zip
Details
10.81 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
37.74 KB, image/png
Details
2.34 MB, image/png
antlam
: feedback+
Details
This way we align our UI with desktop and cleanup the toolbar a bit. Toolbar would only show the security state (again, just like desktop). Thoughts?
Makes sense to me!
Agreed! The space for favicon on the tab strip can also be used for display loading indicator, like the throbber on desktop FX. 

And this will make even more sense when we support pinning a tab on the tab strip :)
I don't want to add a implementation-specific version for all of our files, if we can help it, so I opted for the simple solution which is to use ifs within the code. Let me know if you disagree with this approach.

Lucas, do you want to deal with part 2 (adding the favicon to the tab strip),
or shall I?
Attachment #8488376 - Flags: review?(lucasr.at.mozilla)
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Attached image Doorhanger, no favicon (obsolete) —
Doorhangers do look a bit fishy without the favicon though - Yuan, what do you think?
Attachment #8488379 - Flags: feedback?(ywang)
(In reply to Michael Comella (:mcomella) from comment #5)
> Created attachment 8488379 [details]
> Doorhanger, no favicon
> 
> Doorhangers do look a bit fishy without the favicon though - Yuan, what do
> you think?

I think we'll need keep the security icon always visible and have 'default' state for when it doesn't have any special identity/security state i.e. the equivalent of the gray globe on desktop.
Comment on attachment 8488376 [details] [diff] [review]
Part 1: Remove favicon from BrowserToolbar

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

Looks good. But I think we should sort out the design for the security doorhanger as part of this bug. I think we should keep the security icon always visible and have a generic state for it (see comment #6).

(In reply to Michael Comella (:mcomella) from comment #3)
> Created attachment 8488376 [details] [diff] [review]
> Part 1: Remove favicon from BrowserToolbar
> 
> I don't want to add a implementation-specific version for all of our files,
> if we can help it, so I opted for the simple solution which is to use ifs
> within the code. Let me know if you disagree with this approach.

Yeah, this approach is fine in this case.

> Lucas, do you want to deal with part 2 (adding the favicon to the tab strip),
> or shall I?

Feel free to do it. Let me just land bug 1015447 so that you can write the patch on top of the latest tab strip code.
Attachment #8488376 - Flags: review?(lucasr.at.mozilla) → feedback+
(In reply to Lucas Rocha (:lucasr) from comment #6)
> I think we'll need keep the security icon always visible and have 'default'
> state for when it doesn't have any special identity/security state i.e. the
> equivalent of the gray globe on desktop.

I assume phone would not be changing though, right? Thus we'd be adding more device specific code to ToolbarDisplayLayout?
(In reply to Michael Comella (:mcomella) from comment #8)
> (In reply to Lucas Rocha (:lucasr) from comment #6)
> > I think we'll need keep the security icon always visible and have 'default'
> > state for when it doesn't have any special identity/security state i.e. the
> > equivalent of the gray globe on desktop.
> 
> I assume phone would not be changing though, right? Thus we'd be adding more
> device specific code to ToolbarDisplayLayout?

Yep. We can plan for a refactoring if ToolbarDisplayLayout starts to look too messy with these changes.
Note to self: try to adjust the padding around the site security icon (and placeholder), as per https://bug1058909.bugzilla.mozilla.org/attachment.cgi?id=8480237.
(In reply to Lucas Rocha (:lucasr) from comment #7)
> Looks good. But I think we should sort out the design for the security
> doorhanger as part of this bug. I think we should keep the security icon
> always visible and have a generic state for it (see comment #6).

Any ideas here, Anthony? Also, what should the state be for a website that doesn't have a favicon? According to [1], a rectangle should be fine. If so, I believe I can manage to draw that in code.

[1]: https://bug1060413.bugzilla.mozilla.org/attachment.cgi?id=8490916
Flags: needinfo?(alam)
Also, do you have specs for the favicon size, margins, etc.?
Attached image Favicons (obsolete) —
After some eyeballing, I ended up with:
  * favicon: 20dp
  * favicon's right margin: 5dp

Anthony, what do you think?
Attachment #8491918 - Flags: feedback?(alam)
Lucas, the apple logo from the screenshot in comment 13 appears to have some (alpha?) aliasing issues - any idea why?
Attachment #8491920 - Flags: feedback?(lucasr.at.mozilla)
(In reply to Michael Comella (:mcomella) from comment #14)
> Created attachment 8491920 [details] [diff] [review]
> Part 2: Put favicons in the tab strip
> 
> Lucas, the apple logo from the screenshot in comment 13 appears to have some
> (alpha?) aliasing issues - any idea why?

Maybe it's just a problem with the image itself? I suggest downloading the image to confirm. It might be a bug in the way our Favicons framework handle bitmaps though.
Comment on attachment 8491920 [details] [diff] [review]
Part 2: Put favicons in the tab strip

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

Looking good.

::: mobile/android/base/tabs/TabStripItemView.java
@@ +19,5 @@
>  import android.graphics.PorterDuff.Mode;
>  import android.graphics.PorterDuffXfermode;
>  import android.graphics.Region;
>  import android.util.AttributeSet;
> +import android.util.Log;

Unused?

@@ -22,5 @@
> -import org.mozilla.gecko.Tab;
> -import org.mozilla.gecko.Tabs;
> -import org.mozilla.gecko.widget.ThemedImageButton;
> -import org.mozilla.gecko.widget.ThemedLinearLayout;
> -import org.mozilla.gecko.widget.ThemedTextView;

Oopsie :-)

@@ +191,5 @@
>          }
>  
>          id = tab.getId();
> +        // TODO: default if null.
> +        faviconView.setImageBitmap(tab.getFavicon());

You will probably do something similar to ToolbarDisplayLayout here:
http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ToolbarDisplayLayout.java#353
Attachment #8491920 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Attached image spec_structure_favicon1.png (obsolete) —
hopefully this helps! I've tried to spec out the favicon a bit specifically around the left and right margins.

I'll attach the empty state favicon (that I used here) after this too.

Not sure if this is helpful, but the page titles are 14dp in the design and hard capped at 109dp width (where fading happens just before the length hits this cap). This allows for 9dp margins on either side of the 'x' as well for the active tab.
Flags: needinfo?(alam) → needinfo?(michael.l.comella)
Comment on attachment 8491918 [details]
Favicons

just attached some specs, let's try that first. I think this looks like we could use more space to the right of the favicon.
Attachment #8491918 - Flags: feedback?(alam) → feedback-
Attached file icon_favicon.zip
Try these if you need some images for the "empty" favicon :)
Comment on attachment 8488376 [details] [diff] [review]
Part 1: Remove favicon from BrowserToolbar

Doorhanger can be part 3.
Attachment #8488376 - Flags: review?(lucasr.at.mozilla)
I can file a followup to draw empty favicon icons in code to cut down on apk size, if you think this might be useful.
Attachment #8492507 - Flags: review?(lucasr.at.mozilla)
Attachment #8491920 - Attachment is obsolete: true
Attached image Favicons in tab strip (obsolete) —
Anthony, what do you think? I can only space a tab's favicon to the close button on tab to its left, not the curve - do you happen to know how many dps it is between those two? I used 27dp, as that's the distance to the screen edge for the first tab in the leftmost scrolled position.
Attachment #8491918 - Attachment is obsolete: true
Attachment #8492510 - Flags: feedback?(alam)
Flags: needinfo?(michael.l.comella)
Anthony, what should we do about the security doorhanger (comment 5)? Should I use the globe icon that's in the tree (for favicons) [1] in place of the security icon, like desktop does?

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-xhdpi/favicon.png
Flags: needinfo?(alam)
Comment on attachment 8488376 [details] [diff] [review]
Part 1: Remove favicon from BrowserToolbar

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

Nice.
Attachment #8488376 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8492507 [details] [diff] [review]
Part 2: Put favicons in the tab strip

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

Nice.

::: mobile/android/base/resources/values-large-v11/dimens.xml
@@ +9,5 @@
>      <dimen name="browser_toolbar_button_padding">16dp</dimen>
>      <dimen name="tabs_counter_size">26sp</dimen>
>      <dimen name="panel_grid_view_column_width">200dp</dimen>
>  
> +    <dimen name="tab_strip_favicon_size">16dp</dimen>

Dimensions provided by antlam?
Attachment #8492507 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #25)
> Dimensions provided by antlam?

Size of the no-favicon favicon, provided by antlam. :D
Comment on attachment 8492510 [details]
Favicons in tab strip

This actually looks pretty close! I measured it on my file and it looks like it's 25dp. let me attach the spec again. :)
Attachment #8492510 - Flags: feedback?(alam) → feedback-
Flags: needinfo?(alam)
Specs for the favicon.
Attachment #8492290 - Attachment is obsolete: true
(In reply to Michael Comella (:mcomella) from comment #5)
> Created attachment 8488379 [details]
> Doorhanger, no favicon
> 
> Doorhangers do look a bit fishy without the favicon though - Yuan, what do
> you think?

Yeah this does look a bit weird. Let me think about this. Seems to kind of defeat the purpose if we just have another icon in the URL bar.. but I could be wrong.
Attached image Favicon in tab strip v2
Oops - should have checked with Anthony before landing!
Attachment #8492510 - Attachment is obsolete: true
Attachment #8493366 - Flags: feedback?(alam)
Attachment #8488378 - Attachment is obsolete: true
Comment on attachment 8488379 [details]
Doorhanger, no favicon

Let's do doorhangers in a followup: bug 1071267
Attachment #8488379 - Attachment is obsolete: true
Attachment #8488379 - Flags: feedback?(ywang)
Comment on attachment 8493366 [details]
Favicon in tab strip v2

+!
Attachment #8493366 - Flags: feedback?(alam) → feedback+
https://hg.mozilla.org/mozilla-central/rev/298a2cd455fc
https://hg.mozilla.org/mozilla-central/rev/603dad513d7c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.