695204 - Display site security (larry)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Erin Lancaster [:elan]]

* steal what we use to do with
* UI Needed
* Use native popup window (see twitter app)

Comment 1

[Doug Turner (:dougt)]

not needed?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I think it's a good P2

Comment 3

[:Margaret Leibovic]

What should this look like? I'm going to start by trying to imitate what we have in XUL Fennec right now, but this is an opportunity to change that if there's something better we can do.

Comment 4

[:Margaret Leibovic]

I had to switch my attention back to desktop Firefox, so I'm going to unassign myself in case someone else can get to this before me.

Comment 5

[[:fabrice] Fabrice Desré]

Julien, a good first step would be to style the background of the favicon button (ibarlow is working on a nice design).

The security state is notified in the onSecurityChange() method of the web progress listener athttp://mxr.mozilla.org/projects-central/source/birch/mobile/chrome/content/browser.js#624

Here's firefox desktop implementation:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4551

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Note: Styling the background of the favicon is bug 695477, which is a prerequisite for this bug. Check with Lucas and take bug 695477 if you want.

Comment 7

[Julien Vermet | JulienDev]

I'm waiting for Lucas answer before submitting the patch. I asked him if it's possible to take bug 695477.

Comment 8

[Julien Vermet | JulienDev]

bug 695477 has been assigned to me but as the design will change the summary has changed and I'm now waiting for sriram to fix it before posting my patch for this bug.

Comment 9

[Julien Vermet | JulienDev]

On the IRC, we had a talk about where we need to find the lock. We need a lock in the tab bar, this is sure. What do you think about a lock in the tabs menu http://www.flickr.com/photos/61892693@N03/6302522277/in/photostream/ ?

In my opinion this is useless but sriram seemed interested by this feature if I well understood.

Comment 10

[Julien Vermet | JulienDev]

Attachment: Patch v1

There was an error in the orgininal onSecurityChange function. There were 4 arguments instead of 3. As there is no "lock" ready in the BrowserToolbar, I just change the background color of the favicon.

I hope it's what you were looking for :)

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 571717 [details] [diff] [review]
Patch v1

The patch looks good, but we need to track the security state in the Tab, so when we change tabs we set the favicon color.

I'll post an update with that change

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Hmm, I'll post this on bug 695477, since this is color/indicator only

Comment 15

[Madhava Enros [:madhava]]

Are we doing a doorhanger off of the lock for this? And changing the color (or background tile) for the lock for ssl vs. EV?

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

A doorhanger-like popupwindow would be ok for implementation. Color of lock could change too. Throw up some specs.

Comment 17

[Ian Barlow (:ibarlow)]

Attachment: Mockup of doorhanger security menus

Specs and assets to follow soon

Comment 18

[Ian Barlow (:ibarlow)]

Attachment: Design specs

Assets to follow shortly

Comment 19

[Ian Barlow (:ibarlow)]

Attachment: assets for green Larry and blue Larry

Comment 20

[dynamis (Tomoya ASAI)]

From Firefox 3 we stopped using padlock in the location bar:
http://www.dria.org/wordpress/archives/2008/05/06/635/

And removed it from the status bar too (bug 558551)
> During the site identity work of Firefox 3, we decided to move away from using
> the lock icon as a symbol for SSL because users were interpreting it as meaning
> "this site is run by good people" as opposed to its actual meaning.

Our Firefox's site identity (and security) concept is "users need to check site icon only" from Firefox 3 isn't it?

Why you'll re-introduce obsolete lock icon again, only for mobile?

We should keep the same concept of site identity both with desktop Firefox and mobile Firefox. Site icon and around it (background color) should be used to show identity and security of the site.

Comment 21

[Axel Hecht [:Pike]]

Is this still 11+ ? It'd most likely come with string changes at this point.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Axel Hecht [:Pike] from comment #21)
> Is this still 11+ ? It'd most likely come with string changes at this point.

Agreed. This is not on track for Fx11. Moving to Fx12.

Comment 23

[Jennifer Morrow [:Boriss] (UX)]

Removing uiwanted since Ian posted mockups.  Feel free add back if/when more ui input is needed!

Comment 24

[:Margaret Leibovic]

Julien, have you been working on this? I'd like to take this bug if you're too busy to work on it.

Comment 25

[Julien Vermet | JulienDev]

Yes Margaret, I'm very busy right now, feel free to take it.

Comment 26

[:Margaret Leibovic]

Attachment: WIP (browser.js bits)

This puts the gecko pieces in place to get data for the UI. This is mostly a trimmed down port of XUL fennec's IdentityHandler, although I took stuff from desktop's gIdentityHandler that never made its way to mobile (and I had to revert some of the changes made for the custom browser.xul binding).

Comment 27

[:Margaret Leibovic]

(In reply to Axel Hecht [:Pike] from comment #21)
> Is this still 11+ ? It'd most likely come with string changes at this point.

Also, I should note, strings for this already exist, so we wouldn't need to make any string changes. (Some should probably be moved to Java eventually, but we wouldn't need to do that to make the UI work.)

Comment 28

[:Margaret Leibovic]

Attachment: WIP (mostly done)

Issues left to sort out:
-Arrow points to the wrong place on non-ICS devices (and in landscape mode) because its position is hard-coded (I'll probably have to make multiple layout files)
-Popup doesn't get dismissed when you switch away from the app
-Tapping on the lock icon while the popup is open won't dismiss the popup, it will make it re-show

Comment 29

[:Margaret Leibovic]

Attachment: screenshot of SSL website (blue larry)

Comment 30

[:Margaret Leibovic]

Attachment: screenshot of EV website (green larry)

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 619088 [details] [diff] [review]
WIP (mostly done)


>diff --git a/mobile/android/base/SiteIdentityPopup.java b/mobile/android/base/SiteIdentityPopup.java
>+    public static final String VERIFIED_COLOR = "#77BAFF";
>+    public static final String IDENTIFIED_COLOR = "#B7D46A";

We might be able to put these in colors.xml

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd
>+<!-- Localization note (iidentity_run_by) : This string appears between a

identity_run_by
>diff --git a/mobile/android/base/resources/drawable-hdpi/larry_blue.png b/mobile/android/base/resources/drawable-hdpi/larry_blue.png


These files seem kinda large, just from what I see in base64. We might want to crunch them, or I could be off base and the size is not too bad.

>diff --git a/mobile/android/base/resources/layout/site_identity_popup.xml b/mobile/android/base/resources/layout/site_identity_popup.xml

Not blocking you to land, but I wonder if we could reuse the doorhanger "outer" XML, or share it between larry and doorhangers.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
>   onSecurityChange: function(aWebProgress, aRequest, aState) {
>+    // Don't need to do anything if the data we use to update the UI hasn't
>+    // changed
>+    if (this._state == aState &&
>+        !this._hostChanged) {

One line please

>+  getIdentityData : function() {
>+    var result = {};
>+    var status = this._lastStatus.QueryInterface(Components.interfaces.nsISSLStatus);
>+    var cert = status.serverCert;

Could be 'let'

>+  }
>+};
> function OverscrollController(aTab) {

Add some blank lines


Let's get some UX feedback and land this!

Comment 32

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #31)

> >diff --git a/mobile/android/base/resources/drawable-hdpi/larry_blue.png b/mobile/android/base/resources/drawable-hdpi/larry_blue.png
> 
> 
> These files seem kinda large, just from what I see in base64. We might want
> to crunch them, or I could be off base and the size is not too bad.

Ian, can you help me with this?

> >diff --git a/mobile/android/base/resources/layout/site_identity_popup.xml b/mobile/android/base/resources/layout/site_identity_popup.xml
> 
> Not blocking you to land, but I wonder if we could reuse the doorhanger
> "outer" XML, or share it between larry and doorhangers.

Yeah, that sounds like a good follow-up.

Comment 33

[:Margaret Leibovic]

Attachment: patch

I added code to hide the popup in onResume, and I fixed the arrow positioning issues. It points to the correct place in portrait/landscape on my Neuxs S, and it points to the correct place in portrait on my Galaxy Nexus, but it's still a tiny bit off in landscape mode on the Galaxy Nexus, so I need to look into that more. I decided to just hide it on orientation change, so that we don't need to deal with re-adjusting on the fly.

Comment 34

[:Margaret Leibovic]

Attachment: patch w/ crushed pngs

Updated with new pngs that Ian gave me.

Comment 35

[:Margaret Leibovic]

https://hg.mozilla.org/mozilla-central/rev/5430670457ac

Also filed some follow-ups for the small issues with this patch.

Comment 36

[Adrian Tamas (:AdrianT)]

Behavior covered in test case:

https://moztrap.mozilla.org/manage/cases/_detail/6298/

The test case was added in the BFTs run in the Security feature suite

Comment 37

[Adrian Tamas (:AdrianT)]

Security larry is accessible on Nightly 18.0a1 2012-09-02, Aurora 17.0a2 2012-09-02 using the Samsung Galaxy Tab 2 7.0 (Android 4.0.4) and Samsung Galaxy R (Android 2.3.4).