1177576 - Update icons for existing Mixed Content Blocking states

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Desktop isn't going to be using the shield for mixed content anymore, and we should make sure we're updating our UI similarly.

Comment 1

[Martyn Haigh (:mhaigh)]

antlam: can you supply assets?

Comment 2

[Chenxia Liu [:liuche]]

We don't have a separate urlbar icon/slot for tracking protection vs mixed content blocking.

Do we want to update the UI for displaying tracking protection vs mixed content? What should we display if a page has both tracking and mixed content?

Comment 3

[Chenxia Liu [:liuche]]

Anthony says he'll talk to Desktop and Tanvi about this.

What icon does Desktop use for mixed content right now? What are the repercussions for switching the mixed content protection icon to something new (will that be confusing to users who expect the old icon?); and what about co-opting an existing icon (mixed content) and using it for tracking protection?

Comment 4

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

Due to our lack of space in the Toolbar and other design constraints that come with a mobile platform, we might have to split up the lock and yield sign that Desktop merges.

I'm in mid conversation with Aislinn right now, will post designs soon. But, it's currently looking like we will put the yield sign in the doorhanger.

Comment 5

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

Attachment: Screen Shot 2015-07-05 at 12.03.39 AM.png

Attaching screenshot that we used in our convo via IRC/email/Vidyo.

Icons to follow

Comment 6

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

Attachment: icon_MCB.zip

Here they are, for the door hangers

Comment 7

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

I think the http:// one (top left) shouldn't have a red 'x'. Just seems like overkill.

Comment 8

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

Attachment: icon_doorhanger_pack2.zip

Here are updated ones of those I originally attached to bug 1147112

Comment 9

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

Attachment: Mock: Mixed Content states

Just had a meeting with Tanvi, updating the states a little bit.

tl;dr 
 - we no longer have the grey caution sign IN the doorhanger, only yellow
 - we no longer show a grey lock in the toolbar
 - we're adding a grey caution sign in the toolbar

Comment 10

[Tanvi Vyas[:tanvi]]

(In reply to Anthony Lam (:antlam) from comment #9)
> Created attachment 8631147 [details]
> Screen Shot 2015-07-08 at 11.24.22 AM.png
> 

One more note that came up in a conversation with Aislinn about the Desktop UI.

In the 4th column, you should remove the verified by, the Yahoo Inc, and the city.  Once mixed content is loaded on a page, that information is no longer provided to the user since the connection is no longer fully secure.

Comment 11

[Chenxia Liu [:liuche]]

Got it, I'll unblock this from our bug tracking spreadsheet.

Comment 12

[Chenxia Liu [:liuche]]

Splitting out passive mixed content icons into bug 1185600, because no differentiation currently exists between passive mixed content vs (unblocked) active mixed content vs non-secure connections.

Comment 13

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1177576 - Update icons for different Mixed Content Blocking states. r=ally

Bug 1177576 - Update icons for different Mixed Content Blocking states. r=ally

Comment 14

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1177576 - Update styling for Owner text. r=ally

Bug 1177576 - Update styling for Owner text. r=ally

Comment 15

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Secure, Active MC blocked

Comment 16

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Insecure

Comment 17

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Insecure, Active MC loaded (about:config pref)

Comment 18

[Chenxia Liu [:liuche]]

This omits the differentiation of loaded passive content, and simply displays them as insecure.

I tested using these three sites from Tanvi, and by flipping the Active MC blocking pref (security.mixed_content.block_active_content):

https://people.mozilla.com/~tvyas/mixedboth.html
- Both active mixed content and passive mixed content - the active content is blocked by default, the passive is allowed

https://people.mozilla.org/~tvyas/mixeddisplay.html
- passive mixed content that gets loaded by default

https://people.mozilla.org/~tvyas/mixedcontent.html
- This has just active mixed content, and gets blocked by default

Comment 19

[Allison Naaktgeboren :ally]

Comment on attachment 8636907 [details]
MozReview Request: Bug 1177576 - Update styling for Owner text. r=ally

https://reviewboard.mozilla.org/r/13753/#review12361

Assuming you have a good reason for the width change, r+.

::: mobile/android/base/resources/layout/site_identity.xml:69
(Diff revision 1)
> -                          android:layout_width="wrap_content"
> +                          android:layout_width="match_parent"

Why do you need to switch from wrap_content to match_parent for both verifier & owner? What does this add?

Comment 20

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/13753/#review12361

> Why do you need to switch from wrap_content to match_parent for both verifier & owner? What does this add?

I don't know why it was wrap_content originally - we want to use match_parent whenever possible, so we don't need to do the extra width calculation.

Comment 21

[Chenxia Liu [:liuche]]

Also, antlam, when you have a chance - is there a place for a "Learn more" link in the new mixed content mocks? We used to have display a link [1] in the mixed content doorhanger, explaining what mixed content was.

[1] https://support.mozilla.org/kb/how-does-insecure-content-affect-safety-android

Comment 22

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

Comment on attachment 8636909 [details]
Screenshot: Insecure

The padding below looks a bit tall. What's this set at Chenxia?

Comment 23

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

Comment on attachment 8636908 [details]
Screenshot: Secure, Active MC blocked

I <3 these new doorhangers!

Comment 24

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

Comment on attachment 8636910 [details]
Screenshot: Insecure, Active MC loaded (about:config pref)

Same question about the padding at the bottom. But also, did we have red 'x' we wanted to add to the left of "insecure connection"? or do I remember a conversation about removing that but forgot to update my design files? :P

Comment 25

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

(In reply to Chenxia Liu [:liuche] from comment #21)
> Also, antlam, when you have a chance - is there a place for a "Learn more"
> link in the new mixed content mocks? We used to have display a link [1] in
> the mixed content doorhanger, explaining what mixed content was.
> 
> [1]
> https://support.mozilla.org/kb/how-does-insecure-content-affect-safety-
> android

Can we place it just underneath the content just before the "Edit site settings"? Stacking two blue links one after the other should work (padding and layout wise). Can we see how this looks in practice? I'm not sure how often this would appear either.

Comment 26

[Chenxia Liu [:liuche]]

> Screenshot: Insecure
> 
> The padding below looks a bit tall. What's this set at Chenxia?

OMG PADDING IS THE WORST ALWAYS

Okay you caught me, it's 40dp. Apparently I changed it for all the doorhangers but this one. I'll fix that up and make it 20dp.

> But also, did we have red 'x'
> we wanted to add to the left of "insecure connection"? or do I remember a
> conversation about removing that but forgot to update my design files? :P

Yeah, I think we decided to remove the red "X", see comment #7. Let me know what you end up wanting though, now that you've seen the screenshots.

> Can we place it just underneath the content just before the "Edit site
> settings"? Stacking two blue links one after the other should work (padding
> and layout wise). Can we see how this looks in practice? I'm not sure how
> often this would appear either.

Sounds good! Bug 1187107 for this.

Comment 27

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1177576 - Update padding. r=ally

Bug 1177576 - Update padding. r=ally

Comment 28

[Chenxia Liu [:liuche]]

Comment on attachment 8636906 [details]
MozReview Request: Bug 1177576 - Update icons for different Mixed Content Blocking states. r=ally

Bug 1177576 - Update icons for different Mixed Content Blocking states. r=ally

Comment 29

[Chenxia Liu [:liuche]]

Comment on attachment 8638270 [details]
MozReview Request: Bug 1177576 - Update padding. r=ally

Bug 1177576 - Update padding. r=ally

Comment 30

[Chenxia Liu [:liuche]]

I'll update the commit messages to be margaret - I'm having some hg problems atm.

Comment 31

[:Margaret Leibovic]

Comment on attachment 8636906 [details]
MozReview Request: Bug 1177576 - Update icons for different Mixed Content Blocking states. r=ally

https://reviewboard.mozilla.org/r/13751/#review12927

::: mobile/android/base/toolbar/SiteIdentityPopup.java
(Diff revision 1)
> -        final DoorhangerConfig config = new DoorhangerConfig(DoorHanger.Type.MIXED_CONTENT, mContentButtonClickListener);

If we're getting rid of the ability to toggle mixed content blocking on a per-site basis, we should also get rid of this doorhanger type, and the corresponding logic. This includes the "Session:Reload" logic here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1744

However, after disucssing this a bit in person, I'm not sure that we should get rid of this ability to toggle the mixed content blocking... it looks like the desktop UI isn't fully implemented, but we should touch base to make sure we're coordinating here.

Comment 32

[:Margaret Leibovic]

Comment on attachment 8636906 [details]
MozReview Request: Bug 1177576 - Update icons for different Mixed Content Blocking states. r=ally

https://reviewboard.mozilla.org/r/13751/#review12933

::: mobile/android/base/toolbar/SiteIdentityPopup.java
(Diff revision 1)
> -        final DoorhangerConfig config = new DoorhangerConfig(DoorHanger.Type.MIXED_CONTENT, mContentButtonClickListener);

We can drop this issue for now and create a follow-up bug to create a new button for this.

::: mobile/android/base/toolbar/SiteIdentityPopup.java:132
(Diff revision 1)
>              updateIdentityInformation(siteIdentity);

Looking at this logic, at first I wondered how the identity info is hidden in the case where it's not known, and I only later noticed the `toggleIdentityKnownContainerVisibility` method. I feel like it would be more straigtforward to just call `updateIdentityInformation` always, and let that take care of togging visibility of the identity container.

But... the code was already this way, so maybe this could just be a follow-up mentor bug.

::: mobile/android/chrome/content/browser.js:7099
(Diff revision 1)
> -         Services.prefs.getBoolPref("security.mixed_content.block_active_content")) {
> +         !Services.prefs.getBoolPref("security.mixed_content.block_active_content")) {

Sigh. I wonder if we could write a test for this... maybe we can create a test similar to testTrackingProtection that just listens for messages to make sure they're passing along the right state (rather than trying to actually test the UI itself).

Comment 33

[:Margaret Leibovic]

Comment on attachment 8638270 [details]
MozReview Request: Bug 1177576 - Update padding. r=ally

https://reviewboard.mozilla.org/r/14085/#review12937

If antlam approves of a screenshot of this, sounds good to me.

Comment 34

[Chenxia Liu [:liuche]]

I filed bug 1188699 for handling controls of Active Mixed Content from the doorhanger.

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/64726ba3ceed
https://hg.mozilla.org/integration/fx-team/rev/58c7a17a21c2
https://hg.mozilla.org/integration/fx-team/rev/137551feecdd

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/64726ba3ceed
https://hg.mozilla.org/mozilla-central/rev/58c7a17a21c2
https://hg.mozilla.org/mozilla-central/rev/137551feecdd

Comment 37

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/13751/#review13625

Hi Chenxia,

This is probably not the right name for this string, since we don't block mixed display content by default:
<string name="mixed_content_blocked_all">&mixed_content_blocked_all;</string>

You may consider changing it to:
<string name="mixed_content_blocked_active">&mixed_content_blocked_active;</string>

Comment 38

[Tanvi Vyas[:tanvi]]

Also, what are you referring to by mixed content activity?  Mixed content has loaded on the page?
<TextView android:id="@+id/mixed_content_activity"

private TextView mMixedContentActivity;

Does getMixedMode() in mobile/android/chrome/content/browser.js  need to take mixed display content into account?

Sorry for the late drive by; I know this is already landed.

Thanks!

Comment 39

[Chenxia Liu [:liuche]]

No problem, Tanvi, I'm happy to respond.

I would disagree; this is the right name for the string because we display it when all mixed content is blocked, and in this case, when active mixed content is blocked, and passive mixed content isn't present - the work in this bug doesn't handle loaded passive content specially and just displays the "Insecure content" doorhanger.

See the screenshot "Screenshot: Secure, Active MC blocked".

The rest of the work for these mocks is in bug 1185173, which handles the passive content states.

As for the term "MixedContentActivity", this refers to what Firefox is doing with respect to mixed content - so this would be a string that says "some insecure elements present" (passive loaded), "fx has blocked some insecure elements" (passive loaded, active blocked), "fx has blocked all insecure elements" (all mixed content blocked).

Comment 40

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 43.0a1 (2015-08-13)

Tested with these three sites and by changing the security.mixed_content.block_active_content pref from about:config

1.https://people.mozilla.com/~tvyas/mixedboth.html
- pref disabled: http://i.imgur.com/rAOuBYD.png
- pref enabled: http://i.imgur.com/ziUqnXm.png

2.https://people.mozilla.org/~tvyas/mixeddisplay.html
- pref disabled: http://i.imgur.com/ybwWlTj.png
- pref enabled: http://i.imgur.com/MSffsb1.png

3.https://people.mozilla.org/~tvyas/mixedcontent.html
- pref disabled: http://i.imgur.com/gM80hS8.png
- pref enabled: http://i.imgur.com/1LW9Ake.png

Comment 41

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot from 2015-09-22 18:18:58.png

Verified as fixed using:
Device: Samsung s6 (Android 5.1)
Build: Firefox for Android 42 Beta 1