1141904 - Update SiteIdentity popup to match Doorhanger popup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

These are currently two separate doorhangers and in order to show/hide doorhangers, these need to be unified.

Comment 1

[Chenxia Liu [:liuche]]

Anthony, let's get some specs for this.

1. On first show, only the active/newest doorhanger is shown, not all of them.

2. How to order these doorhanger notifications?
- Site identity (always shown)
- Tracking protection
- Login
- Geolocation
- (Optionally) inputs toolike a Firefox Hello page)

3. Is there a way to dismiss individual doorhangers from the list?

Comment 2

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

As per our convo, will post specs soon.

Chenxia, could you post a comprehensive list of our doorhangers so i don't miss any major ones? We can start with the main ones and go from there.

Comment 3

[Chenxia Liu [:liuche]]

Attachment: Zip file of doorhanger screenshots

These are the doorhangers that I could find.

Comment 4

[Chenxia Liu [:liuche]]

Actually, there are some more - searching for where the doorhanger is shown:

http://mxr.mozilla.org/mozilla-central/search?string=doorhanger.show&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

- Offline data storage
- WebRTC permissions
- Plugin activation
- various ContentPermissions (which are all built like geolocation)
- Logins
- Addon that needs a restart
- Blocked popup
- Installing a lightweight-theme that's not whitelisted (e.g., on AMO)
- Safe browsing warning: "This site has been identified as containing malware or a phishing attempt. Be careful."

I haven't encountered a lot of these, but they are all styled similarly to the geolocation doorhanger because we have a single place that creates these doorhangers from their pieces (text, checkboxes, etc).

Comment 5

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

Attachment: prev_siteID_sellogin_mock1.png

Attaching mock of Site ID (adding in the favicon and URL) on stacked on top of a "select login" fallback. It's a bit long, so I have a design for collapsing this site ID but that'll be for another bug. We might have to figure out the nuances around scrolling. 

Also, it's worth noting that a large portion of the Site ID doorhanger is taken up but "Edit Site settings" which shouldn't be there unless there's something to "Edit". Bug 1161200 is filed for that.

Comment 6

[Chenxia Liu [:liuche]]

Attachment: Screenshot: site identity

Comment 7

[Chenxia Liu [:liuche]]

I know the Larry is wrong, but I'll fix that in bug 1147112. How does everything else look?

Comment 8

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

Comment on attachment 8620729 [details]
Screenshot: site identity

Looking good! some feedback:

 - The bottom corners of the buttons, could we round those?
 - Can we remove the repeated "facebook.com" ?
 - where did we land on getting that favicon next to the URL?
 - Can we make "Encrypted" Roboto Medium?

Comment 9

[Chenxia Liu [:liuche]]

Good points, some of these should be handled in separate bugs.

>  - The bottom corners of the buttons, could we round those?
Bug 1173624

>  - Can we remove the repeated "facebook.com" ?
Bug 1173887

>  - where did we land on getting that favicon next to the URL?
This still doesn't always work, I'm not sure why that is. It does work for certain sites. Filed bug 1173991.

>  - Can we make "Encrypted" Roboto Medium?
Done.

I'll also crop the Larry so it doesn't throw the alignment off.

Comment 10

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1141904 - Update SiteIdentity popup to match Doorhanger popup. r=ally

Bug 1141904 - Update SiteIdentity popup to match Doorhanger popup. r=ally

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8621296 [details]
MozReview Request: Bug 1141904 - Update SiteIdentity popup to match Doorhanger popup. r=ally

Bug 1141904 - Update SiteIdentity popup to match Doorhanger popup. r=ally

Comment 12

[Chenxia Liu [:liuche]]

Comment on attachment 8621296 [details]
MozReview Request: Bug 1141904 - Update SiteIdentity popup to match Doorhanger popup. r=ally

Bug 1141904 - Update SiteIdentity popup to match Doorhanger popup. r=ally

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8621296 [details]
MozReview Request: Bug 1141904 - Update SiteIdentity popup to match Doorhanger popup. r=ally

Bug 1141904 - Update SiteIdentity popup to match Doorhanger popup. r=ally

Comment 14

[Allison Naaktgeboren :ally]

Comment on attachment 8621296 [details]
MozReview Request: Bug 1141904 - Update SiteIdentity popup to match Doorhanger popup. r=ally

https://reviewboard.mozilla.org/r/10937/#review9639

I'm sure you can't wait until doorhangers is wrapped up. I see there is more follow up work already filed, so let's get this in and move on.

::: mobile/android/base/toolbar/SiteIdentityPopup.java:287
(Diff revision 2)
> +                if (favicon == null) {

wouldnt this be more compact as

somevar = favicon ? new BitmapDrawable(mContext.getResources() : null;
 mTitle.setCompoundDrawablesWithIntrinsicBounds(somevar, null, null, null);
 
 if(somevar)
 mTitle.setCompoundDrawablePadding()...

::: mobile/android/base/locales/en-US/android_strings.dtd:549
(Diff revision 2)
> +<!ENTITY identity_encrypted "Encrypted">

I understand there can be confusion on the l10n dashboards when previously translated strings move from one place move to another (moving from identity.encrypted to identity_encrypted would still show up as a totally new string. The dashboard code isnt very smart).And they try very hard to minimize repeat work on localizers.

I think you can still land this as is on Nightly and drop a heads up note to the l10n group.

::: mobile/android/base/toolbar/SiteIdentityPopup.java:291
(Diff revision 2)
> +                    mTitle.setCompoundDrawablePadding((int) mContext.getResources().getDimension(R.dimen.doorhanger_drawable_padding));

I was under the impression that c-style casts were verboten in the fennec code base.

Comment 15

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/10937/#review9641

::: mobile/android/base/toolbar/SiteIdentityPopup.java:287
(Diff revision 2)
> +                if (favicon == null) {

I'd rather this be more clear, and doing the ternary assignment doesn't save us any effective lines of code. I added a comment.

::: mobile/android/base/toolbar/SiteIdentityPopup.java:291
(Diff revision 2)
> +                    mTitle.setCompoundDrawablePadding((int) mContext.getResources().getDimension(R.dimen.doorhanger_drawable_padding));

What's a c-style cast (versus just casting)? We know that getDimension returns an int [1]. If you look any of the code that uses findViewById, we cast everything - we use casting in places we know exactly what type to expect.

[1] http://developer.android.com/reference/android/content/res/Resources.html#getDimension%28int%29

::: mobile/android/base/locales/en-US/android_strings.dtd:549
(Diff revision 2)
> +<!ENTITY identity_encrypted "Encrypted">

Will do.

Comment 16

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/4af039964804

Comment 17

[Chenxia Liu [:liuche]]

Flod, I'm moving a string from our JS code to the Java code - let me know if I should add a localization note.

https://hg.mozilla.org/integration/fx-team/rev/4af039964804#l2.11

Comment 18

[Francesco Lodolo [:flod]]

(In reply to Chenxia Liu [:liuche] from comment #17)
> Flod, I'm moving a string from our JS code to the Java code - let me know if
> I should add a localization note.

I don't think it's necessary.

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4af039964804

Comment 20

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-06-15-16-05-04.png

Tested with:
Device: Nexus 4 (Android 5,1)
Build: Firefox for Android 41.0a1 (2015-06-14)

1. Is the site identity pop-up aligned correctly?
2. If I tap the lock and choose "Copy" from the second doorhanger: "Copy password from teodora.test?", the doorhanger is dismissed. If I tap once again the grey lock, "Copy password from teodora.test?" doorhanger is still displayed. Is it expected?
3. If I go once again to facebook.com and fill in the username "teodora.test10", a password and log in, choose to remember the log in and tap the lock, then the second doorhanger is "Copy password from teodora.test?" Is it expected?

Comment 21

[Chenxia Liu [:liuche]]

Thanks for following up, Teodora.

> 1. Is the site identity pop-up aligned correctly?
Yes, we have different alignments for tablet and phone. In terms of horizontal alignment, on phones doorhangers should be centered, and on tablets they should be anchored underneath the "back" button. (Bug 1174144 is still present for vertical alignment on some devices though.

> 2. If I tap the lock and choose "Copy" from the second doorhanger: "Copy
> password from teodora.test?", the doorhanger is dismissed. If I tap once
> again the grey lock, "Copy password from teodora.test?" doorhanger is still
> displayed. Is it expected?
Yes, the "Copy" doorhanger should always be present if there are logins associate with a site. This feature was added in bug 1126608.

> 3. If I go once again to facebook.com and fill in the username
> "teodora.test10", a password and log in, choose to remember the log in and
> tap the lock, then the second doorhanger is "Copy password from
> teodora.test?" Is it expected?

If you refresh the page and summon the doorhanger again, you'll see a link that allows you to Copy from the second login. I don't know if we want to do better, and refresh that doorhanger if a new login has been added - this would depend on updating Tab state on refresh. I filed bug 1174809.