Closed Bug 1177576 Opened 4 years ago Closed 4 years ago

Update icons for existing Mixed Content Blocking states

Categories

(Firefox for Android :: General, defect)

35 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- verified

People

(Reporter: Margaret, Assigned: liuche)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 1 obsolete file)

Desktop isn't going to be using the shield for mixed content anymore, and we should make sure we're updating our UI similarly.
antlam: can you supply assets?
Flags: needinfo?(alam)
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?
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?
Blocks: 1102539
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.
Attaching screenshot that we used in our convo via IRC/email/Vidyo.

Icons to follow
Attached file icon_MCB.zip
Here they are, for the door hangers
Flags: needinfo?(alam) → needinfo?(liuche)
I think the http:// one (top left) shouldn't have a red 'x'. Just seems like overkill.
Here are updated ones of those I originally attached to bug 1147112
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
Attachment #8630185 - Attachment is obsolete: true
(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.
Got it, I'll unblock this from our bug tracking spreadsheet.
Flags: needinfo?(liuche)
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Blocks: 1185162
Depends on: 1185173
Summary: Update icons for different Mixed Content Blocking states → Update icons for existing Mixed Content Blocking states
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.
Bug 1177576 - Update icons for different Mixed Content Blocking states. r=ally
Attachment #8636906 - Flags: review?(ally)
Bug 1177576 - Update styling for Owner text. r=ally
Attachment #8636907 - Flags: review?(ally)
Attached image Screenshot: Insecure
Attachment #8636909 - Flags: review?(alam)
Attachment #8636908 - Flags: review?(alam)
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
Attachment #8631147 - Attachment description: Screen Shot 2015-07-08 at 11.24.22 AM.png → Mock: Mixed Content states
Attachment #8636907 - Flags: review?(ally) → review+
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?
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.
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
Flags: needinfo?(alam)
Comment on attachment 8636909 [details]
Screenshot: Insecure

The padding below looks a bit tall. What's this set at Chenxia?
Flags: needinfo?(liuche)
Attachment #8636909 - Flags: review?(alam) → review-
Comment on attachment 8636908 [details]
Screenshot: Secure, Active MC blocked

I <3 these new doorhangers!
Attachment #8636908 - Flags: review?(alam) → review+
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
Attachment #8636910 - Flags: review?(alam) → review-
(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.
Flags: needinfo?(alam)
Blocks: 1187107
> 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.
Bug 1177576 - Update padding. r=ally
Attachment #8638270 - Flags: review?(ally)
Flags: needinfo?(liuche)
Attachment #8636906 - Flags: review?(ally) → review?(margaret.leibovic)
Attachment #8638270 - Flags: review?(ally) → review?(margaret.leibovic)
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 on attachment 8638270 [details]
MozReview Request: Bug 1177576 - Update padding. r=ally

Bug 1177576 - Update padding. r=ally
I'll update the commit messages to be margaret - I'm having some hg problems atm.
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.
Attachment #8636906 - Flags: review?(margaret.leibovic)
Attachment #8636906 - Flags: review+
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 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.
Attachment #8638270 - Flags: review?(margaret.leibovic) → review+
I filed bug 1188699 for handling controls of Active Mixed Content from the doorhanger.
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>
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!
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).
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
Verified as fixed using:
Device: Samsung s6 (Android 5.1)
Build: Firefox for Android 42 Beta 1
You need to log in before you can comment on or make changes to this bug.