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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

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?

Comment 1

[Richard Newman [:rnewman]]

Makes sense to me!

Comment 2

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

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 :)

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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.

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

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: No favicon in toolbar

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Doorhanger, no favicon

Doorhangers do look a bit fishy without the favicon though - Yuan, what do you think?

Comment 6

[Lucas Rocha (:lucasr)]

(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 7

[Lucas Rocha (:lucasr)]

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.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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?

Comment 9

[Lucas Rocha (:lucasr)]

(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.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Also, do you have specs for the favicon size, margins, etc.?

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Favicons

After some eyeballing, I ended up with:
  * favicon: 20dp
  * favicon's right margin: 5dp

Anthony, what do you think?

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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?

Comment 15

[Lucas Rocha (:lucasr)]

(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 16

[Lucas Rocha (:lucasr)]

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

Comment 17

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: spec_structure_favicon1.png

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.

Comment 18

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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.

Comment 19

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_favicon.zip

Try these if you need some images for the "empty" favicon :)

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8488376 [details] [diff] [review]
Part 1: Remove favicon from BrowserToolbar

Doorhanger can be part 3.

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Put favicons in the tab strip

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.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Favicons in tab strip

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.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 24

[Lucas Rocha (:lucasr)]

Comment on attachment 8488376 [details] [diff] [review]
Part 1: Remove favicon from BrowserToolbar

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

Nice.

Comment 25

[Lucas Rocha (:lucasr)]

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?

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Lucas Rocha (:lucasr) from comment #25)
> Dimensions provided by antlam?

Size of the no-favicon favicon, provided by antlam. :D

Comment 27

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

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. :)

Comment 28

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: spec_structure_favicon2.png

Specs for the favicon.

Comment 29

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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.

Comment 30

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/298a2cd455fc
https://hg.mozilla.org/integration/fx-team/rev/603dad513d7c

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Favicon in tab strip v2

Oops - should have checked with Anthony before landing!

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8488379 [details]
Doorhanger, no favicon

Let's do doorhangers in a followup: bug 1071267

Comment 33

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8493366 [details]
Favicon in tab strip v2

+!

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/298a2cd455fc
https://hg.mozilla.org/mozilla-central/rev/603dad513d7c