Closed Bug 1141904 Opened 5 years ago Closed 5 years ago

Update SiteIdentity popup to match Doorhanger popup

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: liuche, Assigned: liuche)

References

Details

Attachments

(5 files)

These are currently two separate doorhangers and in order to show/hide doorhangers, these need to be unified.
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?
Flags: needinfo?(alam)
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.
Flags: needinfo?(liuche)
These are the doorhangers that I could find.
Flags: needinfo?(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).
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.
Flags: needinfo?(alam)
Summary: Unify SiteIdentityPopup with general DoorhangerPopup → Update SiteIdentity popup to match Doorhanger popup
Assignee: nobody → liuche
Attachment #8620729 - Flags: review?(alam)
I know the Larry is wrong, but I'll fix that in bug 1147112. How does everything else look?
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?
Attachment #8620729 - Flags: review?(alam) → review-
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.
Bug 1141904 - Update SiteIdentity popup to match Doorhanger popup. r=ally
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
Attachment #8621296 - Flags: review?(ally)
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
Attachment #8621296 - Flags: review?(ally)
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
Attachment #8621296 - Flags: review?(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.
Attachment #8621296 - Flags: review?(ally) → review+
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.
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
Flags: needinfo?(francesco.lodolo)
(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.
Flags: needinfo?(francesco.lodolo)
https://hg.mozilla.org/mozilla-central/rev/4af039964804
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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?
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.
You need to log in before you can comment on or make changes to this bug.