896048 - Don't show site identity info when a page has loaded mixed content

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

When a site has blocked mixed active content, we always show the shield icon. Currently, when the user taps that icon, we always show the site identity information above the blocked mixed content notification, but this is misleading if there is also loaded mixed display content on the page (mixed display content is always loaded by default).

We also shouldn't show this site identity info if the user has chosen to load mixed active content (the case where we show the yellow triange icon).

Desktop does not need to worry about this because the mixed content notification icon and site identity icon are separate, so they can show both the shield and a globe, but we only have the one icon to work with on Android.

We could also explore replacing the site identity info with a message about how the website isn't fully secure, but we currently don't show any site identity icon for sites with just loaded mixed display content, so this would be inconsistent.

Comment 1

[:Margaret Leibovic]

Attachment: patch

I need to get some feedback from ibarlow about what this state should look like, but this patch just hides the identity data when we're in a broken state.

The corresponding desktop code is here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6553

Unfortunately, our code needs to be different because we combine the site identity/mixed content notification icons into one icon. This patch maintains the idea the the identity mode corresponds to the icon, but JS doesn't pass along identity data if it can't be trusted.

Comment 2

[:Margaret Leibovic]

Attachment: screenshot: loaded mixed active content

Comment 3

[:Margaret Leibovic]

Ian, can you help me figure out what the site identity popup should look like in the cases when we want to show a blocked/loaded mixed content notification, but when we don't have any trusted identity data to show? Right now, I'm just hiding the identity data, but this looks kinda awkward because the notification is a darker color, but the arrow is still a lighter color. I think a good solution would be to show a message instead of the identity data, similar to the messages that desktop shows. 

For the loaded mixed content case, desktop shows:
This website contains interactive content that isn't encrypted (such as scripts). Other people can view or modify the website's behavior.

For the loaded mixed display content case, desktop shows:
The connection to this website is not fully secure because it contains unencrypted elements (such as images).

This would require me to make some changes to my patch to pass more information along to Java.

Comment 4

[:Margaret Leibovic]

We need to fix this before shipping mixed content pref'ed on.

Also, if we want to add new strings to the UI, we should try to do that before the end of next week so that they can make it into 25.

Comment 5

[Ian Barlow (:ibarlow)]

I could see a few options here:

1. Making a darker arrow (this makes me a little nervous, one more asset to keep track of...)
2. Adding an rectangle of light blue, say 5dp tall, to the top of the doorhanger so it matches the arrow colour.
3. Lightening the background of the Mixed Content message. The point of the design is more to provide a way to visually separate messages, not necessarily to make the mixed content message dark.
4. Your option of adding a message, which also seems possible. Would you mind sketching what you mean though, to make sure I understand how you would display this? I'm struggling with why we would do this here but not for sites where we show trusted identity info, since it may actually be useful for both cases.

Comment 6

[:Margaret Leibovic]

(In reply to Ian Barlow (:ibarlow) from comment #5)

> 4. Your option of adding a message, which also seems possible. Would you
> mind sketching what you mean though, to make sure I understand how you would
> display this? I'm struggling with why we would do this here but not for
> sites where we show trusted identity info, since it may actually be useful
> for both cases.

We wouldn't need a message for sites with trusted identity info, since in that case we just show that trusted info.

Sketching is hard for me, so I just hacked my patch to demonstrate, I'll upload a screenshot.

I'm in favor of this approach, since it gives users more information about what's going on, and it's consistent with what desktop does.

Comment 7

[:Margaret Leibovic]

Attachment: screenshot: blocked active content w/ loaded display content

Reading the text of this, maybe taking the desktop strings directly isn't so great, since we're saying this page has unencrypted elements (e.g. images), but then below we're saying we blocked insecure content (e.g. scripts).

I just realized that showing the loaded mixed content message would be redundant with the content of the "This page is displaying content that isn't secure" notification that we already have. So maybe this approach doesn't work well after all :(

I also recognize thinking about the matrix of different cases here is confusing, especially since these really are edge cases.

Comment 8

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #7)
> I just realized that showing the loaded mixed content message would be
> redundant with the content of the "This page is displaying content that
> isn't secure" notification that we already have.'

That's kind of what I was wondering about, yeah. How about this, the #2 suggestion I had: http://cl.ly/image/2E272e1B3q1m, where we just add a 5dp band of light blue across the top, so the arrow has something to blend into but we don't have to create a custom style for each new menu combination.

Comment 9

[:Margaret Leibovic]

(In reply to Ian Barlow (:ibarlow) from comment #8)
> (In reply to :Margaret Leibovic from comment #7)
> > I just realized that showing the loaded mixed content message would be
> > redundant with the content of the "This page is displaying content that
> > isn't secure" notification that we already have.'
> 
> That's kind of what I was wondering about, yeah. How about this, the #2
> suggestion I had: http://cl.ly/image/2E272e1B3q1m, where we just add a 5dp
> band of light blue across the top, so the arrow has something to blend into
> but we don't have to create a custom style for each new menu combination.

That solution sounds like a good compromise. I'll work on doing that.

Comment 10

[:Margaret Leibovic]

Comment on attachment 779503 [details] [diff] [review]
patch

Downgrading review to feedback, until I can update the style at the top of the popup.

Comment 11

[:Margaret Leibovic]

Attachment: patch v2

This creates the band of light blue above the mixed content notification when there is no identity data to show.

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 781844 [details] [diff] [review]
patch v2

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

::: mobile/android/base/SiteIdentityPopup.java
@@ +92,4 @@
>      }
>  
>      private void setIdentity(JSONObject identityData) {
> +        String host = identityData.optString("host");

Out of curiosity, are these really optional? When do we have identity data but no host? I wonder if we'd rather just throw and leave this not-visible in those cases than show some data...

Comment 13

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #12)
> Comment on attachment 781844 [details] [diff] [review]
> patch v2
> 
> Review of attachment 781844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/SiteIdentityPopup.java
> @@ +92,4 @@
> >      }
> >  
> >      private void setIdentity(JSONObject identityData) {
> > +        String host = identityData.optString("host");
> 
> Out of curiosity, are these really optional? When do we have identity data
> but no host? I wonder if we'd rather just throw and leave this not-visible
> in those cases than show some data...

I think this was just me being lazy about using try/catch. It probably is better to just throw and have the hide identity data bit in the catch. I'll update my patch.

Comment 14

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17523d827afa

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/17523d827afa

Comment 16

[Tanvi Vyas[:tanvi]]

Hey Margaret,

I'm a little late to this bug and I wanted to make sure I understood the end result.

If a user visits a page that has both mixed active and mixed display content (https://people.mozilla.com/~tvyas/mixedboth.html), what identify information will the user see?  It sounds like they won't see any identify information.  They'll see the shield and be able to disable protection, but that's it.

If a user visits a page with mixed active content and they disable protection, they will see the following: https://bug896048.bugzilla.mozilla.org/attachment.cgi?id=779504.  Is that right?

If a page is fully encrypted (no mixed content), what does the site identity box show.

Thanks!

Comment 17

[:Margaret Leibovic]

(In reply to Tanvi Vyas [:tanvi] from comment #16)

> If a user visits a page that has both mixed active and mixed display content
> (https://people.mozilla.com/~tvyas/mixedboth.html), what identify
> information will the user see?  It sounds like they won't see any identify
> information.  They'll see the shield and be able to disable protection, but
> that's it.

Correct, a shield will show in the location bar. Tapping it will open a popup with no identity information, but with a notification offering to disable protection.

> If a user visits a page with mixed active content and they disable
> protection, they will see the following:
> https://bug896048.bugzilla.mozilla.org/attachment.cgi?id=779504.  Is that
> right?

Yes.

> If a page is fully encrypted (no mixed content), what does the site identity
> box show.

A fully encrypted page shows a lock icon, with the same identity information that desktop would:
https://dl.dropboxusercontent.com/u/3358452/ssl.png

FYI, all these changes are in both Nightly and Aurora now, so you can test them if you have one of those versions installed on your phone.