1332546 - [CustomTab] 3 different types of displaying site info in URL bar should be designed in custom tab while there is only one in chrome

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Rachelle Yang [:ryang][[e-mail]]]

[CustomTab]  3 different types of displaying site info  in URL bar should be designed in custom tab while there is only one type in chrome.

In firefox browser we, 3 different types of displaying site info in URL Bar is possible.
refer to the attachment .

Comment 1

[Rachelle Yang [:ryang][[e-mail]]]

Attachment: chrome has only one tye of site info

chrome has only one type of site info

Comment 2

[Rachelle Yang [:ryang][[e-mail]]]

Attachment: Firefox has 3 types of site info in URL Bar- Type1

Firefox has 3 types of site info in URL Bar- Type1

Comment 3

[Rachelle Yang [:ryang][[e-mail]]]

Attachment: Firefox has 3 types of site info in URL Bar- Type2

Firefox has 3  types of site info in URL Bar- Type2

Comment 4

[Rachelle Yang [:ryang][[e-mail]]]

Attachment: Firefox has 3 types of site info in URL Bar- Type3

Firefox has 3  types of site info in URL Bar- Type3

Comment 5

[Rachelle Yang [:ryang][[e-mail]]]

Hi Jack, 

would you please kindly provide the UX spec for the reference of engineering efforts ?

Thank you !!

Comment 6

[Jack Lin[:jacklin]]

Attachment: Screen Shot 2017-01-25 at 9.08.38 pm.png

Please refer to the attachment. The attached is mockup and our visual designer is working on redesign them.

Thank you
Jack

Comment 7

[Rachelle Yang [:ryang][[e-mail]]]

Hi Julian, 

is the UX spec in comment6 enough for engineering team to work on this ? 
Thank you  very much !

Comment 8

[Julian Chu [:walkingice]]

The draft is enough to me to survey how to implement it.

Comment 9

[Julian Chu [:walkingice]]

Attachment: Bug 1332546 - Refactory ActionBar by extracting methods to another class

ActionBar of CustomTabsActivity is getting complicated. There will be
more components in ActionBar, such as Site-identity-icon.

Move some action-bar related view code to another class.

Review commit: https://reviewboard.mozilla.org/r/120038/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120038/

Comment 10

[Julian Chu [:walkingice]]

Attachment: Bug 1332546 - ensure ActionBar color is not translucent

Translucent color cause some rendering problem in Toolbar. For example,
if we set translucent color to toolbar and changes title, we will see
new title convers the old-translucent title.

Since ActionBar is usually on the top and nothing behind it, it does not
make sense use translucent color as its background.

Review commit: https://reviewboard.mozilla.org/r/120040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120040/

Comment 11

[Julian Chu [:walkingice]]

Attachment: Bug 1332546 - Apply default actionbar color

Designer provides a color code as default background color for
ActionBar.

Also get rid of `NO_COLOR`: -1 equals 0xFFFFFFFF, so it always ignores
white color.

Review commit: https://reviewboard.mozilla.org/r/120042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120042/

Comment 12

[Julian Chu [:walkingice]]

Attachment: Bug 1332546 - Add CustomView to ActionBar of CustomTabsActivity

The CustomView has three components

  * Icon - for site info to indicate whether the visited site is security
  * Title
  * Url

Icon is for site info to indicate whether the visited site is security.
Its icon type is decided by SecurityModeUtil. All of the components' color
are the same as TextView color.

When onTabChanged happens, it updates CustView of ActionBar to reflect
current site security status. Sometimes the callback will be invoked rapidly
several times in very short time. To avoid icon twinkle, to add a delay when
updating CustomView.

Review commit: https://reviewboard.mozilla.org/r/120044/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120044/

Comment 13

[Julian Chu [:walkingice]]

Attachment: Bug 1332546 - add initialization checking to SiteIdentityPopup

If there are multiple SiteIdentityPopup instances.(ie. in GeckoApp and
in CustomTabsActivity), each instances will start receiving events once
attached to window. However it might have not call init() yet.

Review commit: https://reviewboard.mozilla.org/r/120046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120046/

Comment 14

[Julian Chu [:walkingice]]

Attachment: Bug 1332546 - Add SiteIdentityPopup to CustomTabsActivity

Similiar to GeckoApp, once user click icon in ActionBar, it displays
a popup menu for site identity.

Review commit: https://reviewboard.mozilla.org/r/120048/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120048/

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

I don't really know this code, so I think :sebastian should review these patches.

Comment 16

[Julian Chu [:walkingice]]

Jim: Understood, I changed the reviewer to Sebastian, thanks!

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8847001 [details]
Bug 1332546 - Refactory ActionBar by extracting methods to another class

https://reviewboard.mozilla.org/r/120038/#review122486

Awesome. That's pretty nice!

::: commit-message-6d38a:1
(Diff revision 1)
> +Bug 1332546 - Refactory ActionBar

nit: The title of the commit could be a bit more descriptive :)

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8847002 [details]
Bug 1332546 - ensure ActionBar color is not translucent

https://reviewboard.mozilla.org/r/120040/#review122488

Are there some apps that set a translucent color? What does Chrome do?

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8847003 [details]
Bug 1332546 - Apply default actionbar color

https://reviewboard.mozilla.org/r/120042/#review122490

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8847004 [details]
Bug 1332546 - Add CustomView to ActionBar of CustomTabsActivity

https://reviewboard.mozilla.org/r/120044/#review122492

::: mobile/android/base/java/org/mozilla/gecko/toolbar/SecurityModeUtil.java:22
(Diff revision 1)
> +
> +/**
> + * Util class which encapsulate logic of how CustomTabsActivity treats SiteIdentity.
> + * TODO: Bug 1347037 - This class should be reusable for other components
> + */
> +public class SecurityModeUtil {

This looks like a great class for some unit tests.

Also: Let's file a follow-up bug to use this in BrowserApp too. Fragmenting the logic will bite us as soon as we make changes.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/SecurityModeUtil.java:75
(Diff revision 1)
> +                ? securityModeMap.get(securityMode)
> +                : Mode.UNKNOWN;
> +    }
> +
> +    // Security mode constants, which map to the icons / levels defined in:
> +    // http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/resources/drawable/customtabs_site_security_level.xml

This URL returns a 404 error. On the right side of MXR you can find a permalink that will always link to the version you are looking at and not change when the repository changes.

::: mobile/android/base/resources/drawable/customtabs_site_security_icon.xml:6
(Diff revision 1)
> +<level-list xmlns:android="http://schemas.android.com/apk/res/android">
> +
> +    <item
> +        android:drawable="@drawable/site_security_unknown"
> +        android:maxLevel="0"/>

Isn't this list the same as site_security_level.xml?

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8847005 [details]
Bug 1332546 - add initialization checking to SiteIdentityPopup

https://reviewboard.mozilla.org/r/120046/#review122530

::: commit-message-39ec9:1
(Diff revision 1)
> +Bug 1332546 - add initialization checking to SiteIdentityPopop

Popop :)

Comment 22

[Julian Chu [:walkingice]]

Comment on attachment 8847004 [details]
Bug 1332546 - Add CustomView to ActionBar of CustomTabsActivity

https://reviewboard.mozilla.org/r/120044/#review122492

> This looks like a great class for some unit tests.
> 
> Also: Let's file a follow-up bug to use this in BrowserApp too. Fragmenting the logic will bite us as soon as we make changes.

Agree :D

I would like to write a test for this class, however so far I don't have enough clue(spec) to do this. I created bug #1347037 as the follow-up bug. (Maybe title of the bug is not proper one?)

> This URL returns a 404 error. On the right side of MXR you can find a permalink that will always link to the version you are looking at and not change when the repository changes.

404 due to this patch haven't not landed. I think once #1347037 complete, this (for custom tabs) drawble should be removed, so I just use this link for now.

> Isn't this list the same as site_security_level.xml?

Similiar but a bit different. For example, in site_security_level.xml, maxLevel 1 and 2 refer to same drawable. And this file should be removed due to bug #1347037

Comment 23

[Julian Chu [:walkingice]]

Comment on attachment 8847001 [details]
Bug 1332546 - Refactory ActionBar by extracting methods to another class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120038/diff/1-2/

Comment 24

[Julian Chu [:walkingice]]

Comment on attachment 8847002 [details]
Bug 1332546 - ensure ActionBar color is not translucent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120040/diff/1-2/

Comment 25

[Julian Chu [:walkingice]]

Comment on attachment 8847003 [details]
Bug 1332546 - Apply default actionbar color

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120042/diff/1-2/

Comment 26

[Julian Chu [:walkingice]]

Comment on attachment 8847004 [details]
Bug 1332546 - Add CustomView to ActionBar of CustomTabsActivity

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120044/diff/1-2/

Comment 27

[Julian Chu [:walkingice]]

Comment on attachment 8847005 [details]
Bug 1332546 - add initialization checking to SiteIdentityPopup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120046/diff/1-2/

Comment 28

[Julian Chu [:walkingice]]

Comment on attachment 8847006 [details]
Bug 1332546 - Add SiteIdentityPopup to CustomTabsActivity

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120048/diff/1-2/

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8847006 [details]
Bug 1332546 - Add SiteIdentityPopup to CustomTabsActivity

https://reviewboard.mozilla.org/r/120048/#review123418

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b65a8df84b11
Refactory ActionBar by extracting methods to another class r=sebastian
https://hg.mozilla.org/integration/autoland/rev/b296196d1859
ensure ActionBar color is not translucent r=sebastian
https://hg.mozilla.org/integration/autoland/rev/4796c44dda66
Apply default actionbar color r=sebastian
https://hg.mozilla.org/integration/autoland/rev/540020bee3a6
Add CustomView to ActionBar of CustomTabsActivity r=sebastian
https://hg.mozilla.org/integration/autoland/rev/c9df402008ca
add initialization checking to SiteIdentityPopup r=sebastian
https://hg.mozilla.org/integration/autoland/rev/1edd52437ecb
Add SiteIdentityPopup to CustomTabsActivity r=sebastian

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b65a8df84b11
https://hg.mozilla.org/mozilla-central/rev/b296196d1859
https://hg.mozilla.org/mozilla-central/rev/4796c44dda66
https://hg.mozilla.org/mozilla-central/rev/540020bee3a6
https://hg.mozilla.org/mozilla-central/rev/c9df402008ca
https://hg.mozilla.org/mozilla-central/rev/1edd52437ecb

Comment 32

[Sorina Florean [:sflorean]]

Attachment: 2017_04_03_14_17_29_.png

Verified as fixed in build (2017-04-02);
Device: Huawei Honor (Android 5.1.1).