822371 - Implement Mixed Content Blocker Doorhanger - Frontend Changes

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

Implement the frontend UI for the Drop down Doorhanger when Mixed Active Content is Blocked.

+++ This bug was initially created as a clone of Bug #782654 +++

Bug 62178 implements a mechanism to block http script/active or http display content on https pages.  This bug is to actually implement the blocking UI and enhance the patch in bug 62178 to make this feasible.

Comment 1

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - Frontend v1

This is intended to look something like http://cl.ly/image/1E2c300w1f3M, but right now, it looks more like http://people.mozilla.com/~tvyas/MixedUI3B.png.

This patch needs some CSS work.

Comment 2

[Tanvi Vyas[:tanvi]]

This bug is blocked on bug 803620 and bug 803626.  We need a visual design for the "Mixed Content Blocker Icon" and Doorhanger.

The backend works is near done, and the frontend looks like this currently:
http://people.mozilla.com/~tvyas/MixedUI3A.png
http://people.mozilla.com/~tvyas/MixedUI3B.png

We want to land this in FF20 (but pref'ed off until FF21 or later).

Stephen, can you fix up Larissa's first design attempt (http://cl.ly/image/1E2c300w1f3) and provide an icon for this, so that I can make the appropriate changes to the UI?

Thanks!

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 693107 [details] [diff] [review]
Mixed Content Blocker Doorhanger - Frontend v1

>+mixedContentBlockerMessage.message = Firefox has blocked content on this page that isn't secure.

This is ambiguous. I suggest you either write "blocked insecure content on this page" or "blocked content on this insecure page", depending on what you really mean. I assume it's the former.

Comment 4

[Larissa Co [:lco]]

(In reply to Dão Gottwald [:dao] from comment #3)
> Comment on attachment 693107 [details] [diff] [review]
> Mixed Content Blocker Doorhanger - Frontend v1
> 
> >+mixedContentBlockerMessage.message = Firefox has blocked content on this page that isn't secure.
> 
> This is ambiguous. I suggest you either write "blocked insecure content on
> this page" or "blocked content on this insecure page", depending on what you
> really mean. I assume it's the former.

We went over the messages with our copywriter and settled on the ones you see here. Actually, we initially used "Firefox has blocked insecure content on this page." but we ultimately decided that "insecure content" was a bit weird too given that "insecure" has another connotation in the English language (i.e. "unconfident").

Comment 5

[Dão Gottwald [:dao]]

(In reply to Larissa Co from comment #4)
> (In reply to Dão Gottwald [:dao] from comment #3)
> > Comment on attachment 693107 [details] [diff] [review]
> > Mixed Content Blocker Doorhanger - Frontend v1
> > 
> > >+mixedContentBlockerMessage.message = Firefox has blocked content on this page that isn't secure.
> > 
> > This is ambiguous. I suggest you either write "blocked insecure content on
> > this page" or "blocked content on this insecure page", depending on what you
> > really mean. I assume it's the former.
> 
> We went over the messages with our copywriter and settled on the ones you
> see here.

What's your take on the ambiguity then?

> Actually, we initially used "Firefox has blocked insecure content
> on this page." but we ultimately decided that "insecure content" was a bit
> weird too given that "insecure" has another connotation in the English
> language (i.e. "unconfident").

Alternatives would be "nonsecure" (there's precedent for this in IE) or "unencrypted".

Comment 6

[Larissa Co [:lco]]

Ok, after further discussion with the UX Team, we're going to change the string to:

"Firefox has blocked content that isn't secure."

I think that's not ambiguous, and it avoids the "insecure content" phrase.

Tanvi, can you edit this string?

Comment 7

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - Frontend v2

* I removed the Technical Information Button (because that is for v2).  

* I added the correct link for "How does content that isn't secure effect my safety" per bug 822373 (but it doesn't seem to work).

* I changed "Firefox has blocked content on this page that isn't secure" to "Firefox has blocked content that isn't secure" per Larissa and Dao.

You can see what it looks like now here:
https://people.mozilla.com/~tvyas/MixedUI4A.png
https://people.mozilla.com/~tvyas/MixedUI4B.png

One problem is that sometimes the doorhanger is anchored to the wrong part of the url bar (after you resize the window or open a new tab).

Once we have the visual design from Stephen in bugs 803620 and 803626, I can work with a frontend engineer to make the UI match the mockups and fix the doorhanger anchoring issue.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 695078 [details] [diff] [review]
Mixed Content Blocker Doorhanger - Frontend v2

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

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +467,5 @@
> +
> +# Mixed Content Blocker Doorhanger Notification
> +mixedContentBlockerMessage.message = Firefox has blocked content that isn't secure.
> +mixedContentBlockerMessage.label = Block Content
> +mixedContentBlockerMessage.accesskey = D

Can you try to change this accesskey to use one of the characters in the label?

Comment 9

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - Frontend v3

(In reply to Jared Wein [:jaws] from comment #8)
> Comment on attachment 695078 [details] [diff] [review]
> Mixed Content Blocker Doorhanger - Frontend v2
> > +
> > +# Mixed Content Blocker Doorhanger Notification
> > +mixedContentBlockerMessage.message = Firefox has blocked content that isn't secure.
> > +mixedContentBlockerMessage.label = Block Content
> > +mixedContentBlockerMessage.accesskey = D
> 
> Can you try to change this accesskey to use one of the characters in the
> label?

Done.  Jared, this patch needs some css help.  I'm waiting for the visual design from Stephen, and then I'll see if I can get some help from you or someone on your team to make the implementation look like the mockups.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 696946 [details] [diff] [review]
Mixed Content Blocker Doorhanger - Frontend v3

>+#mixed-content-blocker-notification {
>+  -moz-binding: url("chrome://browser/content/urlbarBindings.xml#mixed-content-blocker-notification");

Get rid of this binding and use <popupnotificationcontent> instead. (There are examples for this in browser.xul.)

Comment 11

[Tanvi Vyas[:tanvi]]

Attachment: Invoke Mixed Content Blocker Doorhanger in Browser.js v2

Transferring this patch from bug 822367.

Creates a function in browser.js called showOverrideMixedContent() which shows the Mixed Content Blocker Doorhanger when a document has Mixed Active Content that is blocked.

This can happen on an https page where all mixed content is blocked (IDENTITY_MODE_IDENTIFIED and IDENTITY_MODE_DOMAIN_VERIFIED).  Or an http page with an https iframe that has mixed content blocked (IDENTITY_MODE_UNKNOWN - see bug 824871 if you would like more details on this case).

r? to dao, and have gotten an r+ from jaws.

Comment 12

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - Frontend v4

I made some text changes (per Larissa).

Stephen has a mockup for this: http://cl.ly/image/380W0j0B3k3T.  The only adjustments to this right now are to remove the "Technical Information" button/link and the text changes that are already in the attached patch.  I need some help with the css.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 697105 [details] [diff] [review]
Invoke Mixed Content Blocker Doorhanger in Browser.js v2

>+  showOverrideMixedContent : function() {

Can you tweak this method's name? "Show override mixed content" doesn't seem to make sense.

>+    let options = { };

let options = null;

>+    PopupNotifications.show(gBrowser.selectedBrowser, "mixed-content-blocker", messageString, "mixed-content-blocker-notification-icon", action, null, options);

Format like this:

    PopupNotifications.show(gBrowser.selectedBrowser, "mixed-content-blocker",
                            messageString, "mixed-content-blocker-notification-icon",
                            action, null, options);

Can you also add a secondaryActions variable even though it's null?

Say "blocked" instead of "blocker" throughout this patch.

Comment 14

[Tanvi Vyas[:tanvi]]

Attachment: Invoke Mixed Content Blocker Doorhanger in Browser.js v3

Thanks for the review Dao!  The comments were addressed.

Comment 15

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - Frontend v5

s/mixed-content-blocker/mixed-content-blocked/

Comment 16

[Tanvi Vyas[:tanvi]]

Comment on attachment 697132 [details] [diff] [review]
Invoke Mixed Content Blocker Doorhanger in Browser.js v3

Carrying over Jared's r+.

Comment 17

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - v1

I have combined the two patches.  This version should apply without rejects on a current mozilla-central.

I will also add another patch that should be applied along with this one, so that you can test the feature.

Here is an example mixed content page you can use: https://people.mozilla.com/~tvyas/mixedcontent.html

Comment 18

[Tanvi Vyas[:tanvi]]

Attachment: Plumbing patch

This is the patch that has all the backend plumbing work needed to test the "Mixed Content Blocker Doorhanger" patch.

Once you have both patches applied and built, open Firefox and go to about:config.  Set security.mixed_content_block_active_content to true.

(Note that this patch is a work in progress in bug 822367.  This will work fine for purposes of testing the UI, but this isn't the final patch and it won't land in its current state.)

Comment 19

[Justin Dolske [:Dolske]]

Attachment: More UI tweaking

This is on top of attachment 697201 [details] [diff] [review]. Tanvi, just roll this into the next update of that patch (no need to have it separate).

Not quite done here, but a bunch of it is there.
* Will need final icons from shorlander
* The "more info" button is just a standard button. And a little hacky.
* Visual fine tuning

A bunch of the changes here are to use Dao's spiffy new <popupnotificationcontent> code from bug 823443. It simplifies a bunch, although makes the moreinfo button a little hacky.

A few questions:

Can we combine the two moreinfo strings? It would look and read smoother as a single short paragraph, something like:

  Most websites will still work properly even when this content is
  blocked. If not, email the site and ask them to secure their content.

I also wonder if the message should be more specific to the site at hand:

  This site will likely still work properly even while the insecure content
  is blocked. If not, email the site and ask them to secure their content.

(That also would make it easier to shorten the button label, by making it more implicit that this prompt is for the current page, and is not a global action.)

Finally, doorhangers were intended to ask a question -- not tell a statement -- with the button being the user's answer... Intentionally getting away from the typical "Here is a message. Ok/Other/Cancel" style of old. In that vein, it seems like the revealed moreinfo section should follow that pattern?

As a rough example (needs wordsmithing to make the Q&A flow better):

  Firefox has blocked content that isn't secure.

  Is the site still working? Most sites work properly even with the
  insecure content blocked. If it's broken, email the site and ask them
  to secure their content.

  Default action: [Keep Blocking]
  Secondary: [Load insecure content]
             [Not now] (?)


Last (phew!) question: given the outcome from the above, does the moreinfo section still need a darker background? These are typically a standard color, although the click-to-play plugin (or was it blocked-plugin?) one is precedent for the 2-tone flavor.

Comment 20

[Justin Dolske [:Dolske]]

Screenshots of patch in last comment:
  http://cl.ly/image/031J3G3v2d0r, http://cl.ly/image/3L353l24400W

Comment 21

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - v2

I've folded some of the patches together.  To build -

1) Apply the patch in bug 822366 - https://bug822366.bugzilla.mozilla.org/attachment.cgi?id=697816

2) Apply this patch from bug 822367 - https://bug822367.bugzilla.mozilla.org/attachment.cgi?id=697730

3) Apply the patch in this bug.  Then go to about:config and set security.mixed_content_block_active_content to true so that you can actually see what the doorhanger looks like on a mixed content page like https://people.mozilla.com/~tvyas/mixedcontent.html.

I think the most important thing to get done for FF 20 is the functionality so that people start trying it out.  Specifically
1) the show/hide functionality for the more info button.  
2) don't pop open the doorhanger automatically.  Wait for a user to click on it.

The text, colors, and spacing can then be tweaked as necessary.  For #2, the design specifies that the doorhanger should open automatically the first few times; we can do that in a followup bug for FF 21.
(Design doc - http://people.mozilla.com/~lco/ProjectSPF/Mixed_Content/Mixed_Content_Spec/Mixed%20Content%20Spec%20v4.pdf)

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 697819 [details] [diff] [review]
Mixed Content Blocker Doorhanger - v2

>+  showMixedContentDoorhanger : function() {
>+    /* XXX do this once, maybe repurpose _cacheElements()? */
>+    let morebutton = document.getElementById("mixed-content-blocked-more-button");
>+    morebutton.label = gNavigatorBundle.getString("mixedContentBlockerMoreButton.message");
>+    let link = document.getElementById("mixed-content-blocked-text-link");
>+    link.value = gNavigatorBundle.getString("mixedContentBlockerLearnMore.message");
>+    let formatter = Cc["@mozilla.org/toolkit/URLFormatterService;1"].getService(Ci.nsIURLFormatter);
>+    link.href = formatter.formatURLPref("browser.mixedcontent.warning.infoURL");
>+    let text1 = document.getElementById("mixed-content-blocked-expandtext1");
>+    text1.textContent = gNavigatorBundle.getString("mixedContentBlockerExpand1.message");
>+    let text2 = document.getElementById("mixed-content-blocked-expandtext2");
>+    text2.textContent = gNavigatorBundle.getString("mixedContentBlockerExpand2.message");

We can just put most of these strings directly in browser.xul as entities.

Comment 23

[Larissa Co [:lco]]

(In reply to Justin Dolske [:Dolske] from comment #19)

> A few questions:
> 
> Can we combine the two moreinfo strings? It would look and read smoother as
> a single short paragraph, something like:
> 
>   Most websites will still work properly even when this content is
>   blocked. If not, email the site and ask them to secure their content.
> 
> I also wonder if the message should be more specific to the site at hand:
> 
>   This site will likely still work properly even while the insecure content
>   is blocked. If not, email the site and ask them to secure their content.
> 
> (That also would make it easier to shorten the button label, by making it
> more implicit that this prompt is for the current page, and is not a global
> action.)
> 

I'm ok with combining the sentences (especially since we already removed the "so you can be safer on the Web" clause). I don't have strong feelings about whether we should be more specific or not; I think both could work. I'm finding the sentence "This site will likely still work properly even while the insecure content is blocked" fairly clunky though. I was trying to find a better way to say it, but I don't have any good ideas.

What if we say something like:

"Most sites still work properly even with blocked content. If this page looks broken, email the site and ask them to secure their content.


> Finally, doorhangers were intended to ask a question -- not tell a statement
> -- with the button being the user's answer... Intentionally getting away
> from the typical "Here is a message. Ok/Other/Cancel" style of old. In that
> vein, it seems like the revealed moreinfo section should follow that pattern?
> 
> As a rough example (needs wordsmithing to make the Q&A flow better):
> 
>   Firefox has blocked content that isn't secure.
> 
>   Is the site still working? Most sites work properly even with the
>   insecure content blocked. If it's broken, email the site and ask them
>   to secure their content.
> 
>   Default action: [Keep Blocking]
>   Secondary: [Load insecure content]
>              [Not now] (?)
> 

Just to make sure I understand: you're talking just about providing another action in the extended doorhanger, right? The initial doorhanger will still just have the "More Information" button? Also, will the default and secondary actions be part of the same dropdown button (like you have in your mock), or will they be separate buttons?

I'm open to discussing the UI. The current mixed content design is the result of my general philosophy for security UX wherein we automatically protect the user from threats when we can, rather than asking him whether he wants to be protected (even if we provide the safer default option). This reinforces the message that Firefox is actively taking care of the user, instead of presenting him with decisions he might not fully understand. That being said:

1. I think the way you phrased the question "Is the site still working?" is easy enough for the user to understand. I don't think it's strictly necessary to pose a direct question though if you have multiple button choices. I think we can phrase it in a way that the question is implied. 

2. We all agree that we want to dissuade the user from loading insecure content. I've tried to do that by arguing for a more subtle button treatment so that it doesn't scream out as the action we want the user to take.

Adding "Keep Blocking" as the default option is another way to address this design intent. It might even make the user feel like he's doing something positive to protect himself, rather than reading all the way down and not knowing what to do :) However, if we want to add this choice, I feel strongly that the design should be a dropdown button, where the user must intentionally click on the down arrow to remove Firefox's protection. I think that having the primary and secondary buttons side-by-side will confuse the user as to what the "right thing to do" is, especially because we already have a couple of other links in there.

3. The other thing to consider is whether presenting the choice "Keep Blocking" will confuse the user. If he doesn't click on the button, will he stop being protected? My intent was to make this a passive notification rather than one that the user has to pay attention to, especially because in most cases, he'll be fine. 

I think it's also helpful to keep in mind that most users won't see the extended doorhanger. The short doorhanger will only be shown a few times to familiarize the user with the icon. After that, the user will only see the doorhanger by clicking on the shield icon. 

> Last (phew!) question: given the outcome from the above, does the moreinfo
> section still need a darker background? These are typically a standard
> color, although the click-to-play plugin (or was it blocked-plugin?) one is
> precedent for the 2-tone flavor.

I leave it up to shorlander to make the two-tone choice (although I personally think it's quite pretty ;-) )

Comment 24

[Justin Dolske [:Dolske]]

(In reply to Larissa Co from comment #23)

> I don't have strong feelings about
> whether we should be more specific or not; I think both could work.

The more I think about it, the more I think we should. Prompts don't really work well for general user education, so it should focus on the site at hand. And if the site actually is broken it's not really relevant to the user that other sites might be working. ;-)

[At least, I assume the point of the "most sites" language is to help guide the user to (1) not unblock blindly and (2) help avoid confusion as to why we're showing a dialog when everything seems to be working just fine.]


> > As a rough example (needs wordsmithing to make the Q&A flow better):
> > 
> >   Firefox has blocked content that isn't secure.
> > 
> >   Is the site still working? Most sites work properly even with the
> >   insecure content blocked. If it's broken, email the site and ask them
> >   to secure their content.
> > 
> >   Default action: [Keep Blocking]
> >   Secondary: [Load insecure content]
> >              [Not now] (?)

> Just to make sure I understand: you're talking just about providing another
> action in the extended doorhanger, right? The initial doorhanger will still
> just have the "More Information" button?

Yes.

> Also, will the default and
> secondary actions be part of the same dropdown button (like you have in your
> mock), or will they be separate buttons?

Yes. Just like with other doorhangers, which typically have multiple actions.

> The current mixed content design is the
> result of my general philosophy for security UX wherein we automatically
> protect the user from threats when we can, rather than asking him whether he
> wants to be protected (even if we provide the safer default option). This
> reinforces the message that Firefox is actively taking care of the user,
> instead of presenting him with decisions he might not fully understand.

100% agreed. I think we're on the right track and are just fine-tuning.

> That being said:
> 
> 1. I think the way you phrased the question "Is the site still working?" is
> easy enough for the user to understand. I don't think it's strictly
> necessary to pose a direct question though if you have multiple button
> choices. I think we can phrase it in a way that the question is implied.

This is also for consistency with other doorhangers which ask questions.
 
> [...] I feel
> strongly that the design should be a dropdown button, where the user must
> intentionally click on the down arrow to remove Firefox's protection. I
> think that having the primary and secondary buttons side-by-side will
> confuse the user as to what the "right thing to do" is, especially because
> we already have a couple of other links in there.

Agreed. We don't do that in doorhangers by design. One button.

> 3. The other thing to consider is whether presenting the choice "Keep
> Blocking" will confuse the user. If he doesn't click on the button, will he
> stop being protected?

I'm not too concerned about that; it's par for all doorhangers, even for those with large privacy implications like geolocation or webrtc's prompt for using a camera/mic.
 
> I think it's also helpful to keep in mind that most users won't see the
> extended doorhanger. The short doorhanger will only be shown a few times to
> familiarize the user with the icon. After that, the user will only see the
> doorhanger by clicking on the shield icon.

This part wasn't clear to me (although now I remember Tanvi mentioning it -- it certainly isn't implemented yet. I'm a little dubious about it for being novel UI, such things are infamous for being a bit confusing to the user ("why did that thing suddenly stop doing what it's always done before?").

Although something that doesn't have to be perfect right now, though.

Comment 25

[Justin Dolske [:Dolske]]

Attachment: Mixed Content Blocker Doorhanger - v3

More UI tweaks (modified version of v2 / attachment 697819 [details] [diff] [review]).

Screenshots:

http://cl.ly/image/0G0o2K1z112j (geolocation, for comparison)
http://cl.ly/image/3X1N2A0b3R0R (first panel opening)
http://cl.ly/image/2y0p433A3A1D (after clicking More)
http://cl.ly/image/1D3b0M2q0X3o (showing secondaries)

A few remarks:

The "More" button is going to be a PITA to get styled correctly. The doorhanger implementation has all the content within a container in the panel, and that just doesn't play well with the More and 2-tone styling wanting to tweak the whole panel. It's not impossible to do, but I'm not sure it's worth the effort... Especially with the panel not being shown by default (ie just the urlbar icon shows up until you click it). [At least, that's now implemented but I'm not sure if we're still wanting to show it by default for a the first few times.]

I'd suggest that, especially if this wants to target the current train, we ditch the More button, and have clicking the panel always show as http://cl.ly/image/2y0p433A3A1D

Casting an eye towards the link in the panel... Did you consider having this just read as "Learn More..." link, as is used elsewhere? I don't feel super strongly about changing it, but neither am I sure if extra wordage adds much.

Comment 26

[Justin Dolske [:Dolske]]

(Oh, and I changed the wording along the lines as we were discussing, comments?)

Comment 27

[Justin Dolske [:Dolske]]

Attachment: More recent UX mockup

This is the most recent UX mockup I know of, attaching here for posterity and because I keep losing the link. :)

I gather from prior discussions it was already decided to not have a "Less Info" button, and the text/links are stale (current implementation is close to what is currently wanted, other than exact wording).

Comment 28

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Justin Dolske [:Dolske] from comment #25)
> The "More" button is going to be a PITA to get styled correctly. The
> doorhanger implementation has all the content within a container in the
> panel, and that just doesn't play well with the More and 2-tone styling
> wanting to tweak the whole panel. It's not impossible to do, but I'm not
> sure it's worth the effort... Especially with the panel not being shown by
> default

+1. That is also what I suggested yesterday (on IRC, IIRC).

Comment 29

[Dão Gottwald [:dao]]

(In reply to Justin Dolske [:Dolske] from comment #25)
> The "More" button is going to be a PITA to get styled correctly. The
> doorhanger implementation has all the content within a container in the
> panel, and that just doesn't play well with the More and 2-tone styling
> wanting to tweak the whole panel. It's not impossible to do, but I'm not
> sure it's worth the effort...

Yes, the current technique of customizing the anonymous content doesn't scale well. We should at some point change the base binding to incorporate that style, since as far as I know Stephen wants this for all popup notifications anyway.

> Especially with the panel not being shown by
> default (ie just the urlbar icon shows up until you click it). [At least,
> that's now implemented but I'm not sure if we're still wanting to show it by
> default for a the first few times.]

As long as the default action is "Keep Blocking", I think we're good even if we were to show the panel automatically in some situations.

> Casting an eye towards the link in the panel... Did you consider having this
> just read as "Learn More..." link, as is used elsewhere? I don't feel super
> strongly about changing it, but neither am I sure if extra wordage adds much.

This would have the advantage of having the panel pose only one question.

Comment 30

[Tanvi Vyas[:tanvi]]

The backend work is done, but still needs review.  I'm not sure if we will be uplifting this to FF20 on Aurora this week, or if we are going to have it land in FF21.  

Either way, I'd rather land this sooner than later so we get more data and bugs, even if the UI isn't perfect yet.  Remember, this is turned off by default, so only users who go to about:config and turn on the pref will see this.  Given that, the UI in Justin's attached patch seems like it will do just fine:

* It doesn't show the doorhanger by default (the user has to click on it)
* It does have the more button info button (which seems to be working, but even if it doesn't sometimes, that's okay)
* It uses the Keep Blocking/Load Insecure Content/Not now options, and they all work.  (But if you do click "Not now", you don't get an option to change your mind later if you decide the page is broken.  The user would have to know to reload the page and then choose "Load Insecure Content")

As far as the actual text, I think we should stick with what Larissa had originally, since it's already been through review with Matej, UX, and security.  We can change it once we've got something new that has been reviewed.  We should create a new bug for modifications to the UI (two-tone or one-tone, keeping the more info button or doing away with it, Keep Blocking/Load Insecure Content/Not Now or the "Disable protection" button, text changes, etc) and can discuss them there.

Justin, can we mark this patch for review?

Stephen, can you send Justin the final icons?

Thanks everyone for your help with this!

Comment 31

[Larissa Co [:lco]]

I want to reiterate some of the design decisions that went into the design of this panel and compare them with the proposed tweaks. Again, I don't mind making it better. But I want to make sure we aren't going against the intent of the design while tweaking this.

For more detailed explanations, please see pg. 13 onwards of http://people.mozilla.com/~lco/ProjectSPF/Mixed_Content/Mixed_Content_Spec/

1. Expanding doorhanger design

The reason why we show a shorter message first then allow the user to expand that message is that we are actually targeting two types of users. For the general population, the shorter message ("Firefox has blocked content that isn't secure.") is enough. It doesn't require any actions on their part, and they can disregard the message. For those users who want to understand or see that the page is broken, the expanded message gives them more actionable information. 

Since Tanvi says this is working right now, I want to keep it in, even if the visual design is not great. If we want to make it a little nicer in the interim, maybe we can match the background color of the "more info" button with the rest of the dialog.


2. Expanded doorhanger text

The text is intentionally organized as follows:
* What is FF doing to protect you?
* What is the current impact to your task?
* What can you do if your task is impacted?

I organized it this way because it's first important to give the impression that we are protecting the user above everything else. Next, it's important to respect the user's task and explain how our protection impacts them. 

I think I agree with dolske now that the message should be specific to the page, not general. But I don't like leading with the question "Do you want to unblock the content?" because it confuses the user into thinking that this is an action we condone. I also don't like eliminating the suggestion that the user email the site admin if content is broken on the page. If content is broken, the solution we should be recommending ISN'T to unblock content, therefore, I don't want to emphasize that option. 

I need to think a little bit more about how to phrase this message, especially if we want to keep the question format. For now, let's keep the message that Tanvi already has for now. 


3. SUMO link text ("How does content that isn't secure affect my safety")

After seeing the position of this link in relation to the button, I think that we can safely edit the title to just "Learn More…". I think it *would* be confusing to phrase it as a question right next to that button.


4. Button choices

I'm not sure what the "Not Now" button actually does. Since protection is on by default and the door hanger doesn't appear all the time anyway, I would remove this option. 

Also, I would keep the button text that says "Disable Protection" rather than "Load Insecure Content". I want to reinforce the message that we are discouraging this action from a security standpoint. I wanted to avoid confusing the user with phrasing that sends mixed signals ("load" sounds positive because it's empowering, while "insecure" sounds negative because it's unsafe)

Comment 32

[Justin Dolske [:Dolske]]

Just some brief comments:

(In reply to Larissa Co from comment #31)

> 1. Expanding doorhanger design
> 
> The reason why we show a shorter message first then allow the user to expand
> that message is [...]

Is this still relevant with the prompt not being shown by default? The user has to take explicit action to show it...

> Since Tanvi says this is working right now, I want to keep it in, even if
> the visual design is not great.

It's easy to remove and but hard to make correct. If we're going to land something as an intermediate step, I think we should land it without the "More" button and add it later when the design and implementation is more solid. That's 


> 2. Expanded doorhanger text
> 
> [...]
> I organized it this way because it's first important to give the impression
> that we are protecting the user above everything else. Next, it's important
> to respect the user's task and explain how our protection impacts them.

Somewhere in here I want to make sure we've considered consistency with other similar UI. We've got quite a few of them, and there's a cost (in user terms) to having differences.

> 4. Button choices
> 
> I'm not sure what the "Not Now" button actually does. Since protection is on
> by default and the door hanger doesn't appear all the time anyway, I would
> remove this option. 

All doorhangers get this. It does nothing, and is provided so users who feel compelled to make a choice have a harmless way out.

Comment 33

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - v4

Removed the moreinfo button and kept the text to what Larissa had originally in http://people.mozilla.com/~lco/ProjectSPF/Mixed_Content/Mixed_Content_Spec/

This is what it looks like: 
http://people.mozilla.com/~tvyas/MixedUI5A.png
http://people.mozilla.com/~tvyas/MixedUI5B.png

Further discussion on changing the text or design should go in bug 827595, and we can keep this bug for implementation and code related discussion.

r? to dao.

And need info for shorlander for the Final Icons.  We are trying to land this on Aurora this week.

Thanks!

Comment 34

[Dão Gottwald [:dao]]

Comment on attachment 698951 [details] [diff] [review]
Mixed Content Blocker Doorhanger - v4

>--- a/browser/base/content/browser.css
>+++ b/browser/base/content/browser.css

>+#mixed-content-blocked-notification > popupnotificationcontent > #mixed-content-blocked-more-container {
>+  display: none;
>+}

Remove obsolete code.

>+#mixed-content-blocked-helplink {
>+  margin: 0px;
>+}

This appears to be visual styling rather than application logic and therefore doesn't belong in this file.

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

>+  showMixedContentDoorhanger : function() {
>+    // XXX we will be called repeatedly, including when switching back to
>+    // a tab with mixed content. Not sure if we should suppress extra calls
>+    // here or more towards the backend.

How would you go about suppressing them more towards the backend?

>+    <popupnotification id="mixed-content-blocked-notification" hidden="true">
>+      <popupnotificationcontent orient="vertical" align="start">
>+          <description id="mixed-content-blocked-moreinfo">&mixedContentBlocker.moreinfo;</description>
>+          <spacer flex="1"/>
>+          <label id="mixed-content-blocked-helplink" class="text-link"
>+                 value="&mixedContentBlocker.helplink;"/>
>+      </popupnotificationcontent>
>+    </popupnotification>

Can you sprinkle <separator/>s around mixed-content-blocked-moreinfo instead of the 1.5em margin your adding in the theme files?

>+<!ENTITY mixedContentBlocker.helplink "Learn more...">

Call this string "mixedContentBlocked.helplink".

You should use a real ellipsis rather than three dots, except that this string probably shouldn't have an ellipsis in the first place. See bug 615483 comment 0 (last sentence).

>+<!ENTITY mixedContentBlocker.moreinfo "Most websites will still work properly even when this content is blocked.  If the page looks broken, email the site and ask them to secure their content.">

Call this string "mixedContentBlocked.moreinfo".

Should "email the site and ask them..." be "email the site _owners_ and ask them..." instead?

>+mixedContentBlocker.message = Firefox has blocked content that isn't secure.

Call this string "mixedContentBlocked.message".

>+mixedContentBlocker.blockButton.label = Keep Blocking

Call this string "mixedContentBlocked.keepBlockingButton.label".

>+mixedContentBlocker.blockButton.accesskey = B

Call this string "mixedContentBlocked.keepBlockingButton.accesskey".

>+mixedContentBlocker.unblockButton.label = Disable protection on this page

Call this string "mixedContentBlocked.unblockButton.label".

>+mixedContentBlocker.unblockButton.accesskey = D

Call this string "mixedContentBlocked.unblockButton.accesskey".

Comment 35

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - v5

Comments addressed.  Thanks Dao!

(In reply to Dão Gottwald [:dao] from comment #34)
> Comment on attachment 698951 [details] [diff] [review]
> Mixed Content Blocker Doorhanger - v4
> 
> >--- a/browser/base/content/browser.css
> >+++ b/browser/base/content/browser.css
> >+#mixed-content-blocked-helplink {
> >+  margin: 0px;
> >+}
> 
> This appears to be visual styling rather than application logic and
> therefore doesn't belong in this file.
> 
Moved to /browser/themes/*/browser.css

> >--- a/browser/base/content/browser.js
> >+++ b/browser/base/content/browser.js
> 
> >+  showMixedContentDoorhanger : function() {
> >+    // XXX we will be called repeatedly, including when switching back to
> >+    // a tab with mixed content. Not sure if we should suppress extra calls
> >+    // here or more towards the backend.
> 
> How would you go about suppressing them more towards the backend?
> 
This isn't an issue because the doorhanger is dismissed by default.  Removing this comment.

> >+<!ENTITY mixedContentBlocker.moreinfo "Most websites will still work properly even when this content is blocked.  If the page looks broken, email the site and ask them to secure their content.">
> 
> Should "email the site and ask them..." be "email the site _owners_ and ask
> them..." instead?
> 
No, Larissa and others had discussed this earlier when we came up with the text.  Users don't know who the site "owner" or "admin" is.  We can discuss this further in bug 827595.

Comment 36

[Dão Gottwald [:dao]]

(In reply to Tanvi Vyas [:tanvi] from comment #35)
> > Should "email the site and ask them..." be "email the site _owners_ and ask
> > them..." instead?
> > 
> No, Larissa and others had discussed this earlier when we came up with the
> text.  Users don't know who the site "owner" or "admin" is.

Where are they supposed to send the email then?

I originally tripped over that sentence because the pronoun "them" can't refer to "site", so the sentence doesn't really make sense as is.

Comment 37

[Justin Dolske [:Dolske]]

(In reply to Dão Gottwald [:dao] from comment #34)

> >+#mixed-content-blocked-helplink {
> >+  margin: 0px;
> >+}
> 
> This appears to be visual styling rather than application logic and
> therefore doesn't belong in this file.

Oh, indeed. It's because of the annoying |label { margin: 2px 6px; }| in global.css, when I first saw it I thought it was in one of the default content/ styles. (Without this the link looks indented from the rest of the text.)


> >+  showMixedContentDoorhanger : function() {
> >+    // XXX we will be called repeatedly, including when switching back to
> >+    // a tab with mixed content. Not sure if we should suppress extra calls
> >+    // here or more towards the backend.
> 
> How would you go about suppressing them more towards the backend?

I was assuming that something down in docshell-land was responsible for firing mixed-content notifications, it might be nice if was smart enough to only do so when the (top?) document's state transitions from "no mixed content" to "mixed content". There's no reason to tell the front-end (at least right now?) about further references in the page to insecure resources.

That would seem a little cleaner (and perhaps more performant) than having the front-end code here bail out if it's already got a doorhanger.

Actually, now that I look at the code again, I suspect it's due to this patch's change in onSecurityChange()... The (existing) comment there says "Don't need to do anything if the data we use to update the UI hasn't changed", but the code this patch adds breaks that suppression -- for all security changes -- whenever mixed content is present.

I think that needs to be fixed. One of the base patches is adding a STATE_IS_BROKEN, so it seems like onSecurityChange() shouldn't need to change at all. Is |this._state| not getting updated somewhere?

Comment 38

[Dão Gottwald [:dao]]

(In reply to Justin Dolske [:Dolske] from comment #37)
> (In reply to Dão Gottwald [:dao] from comment #34)
> 
> > >+#mixed-content-blocked-helplink {
> > >+  margin: 0px;
> > >+}
> > 
> > This appears to be visual styling rather than application logic and
> > therefore doesn't belong in this file.
> 
> Oh, indeed. It's because of the annoying |label { margin: 2px 6px; }| in
> global.css, when I first saw it I thought it was in one of the default
> content/ styles. (Without this the link looks indented from the rest of the
> text.)

This only applies to pinstripe, I believe. winstripe and gnomestripe apply the same margin to label and description.

Comment 39

[Justin Dolske [:Dolske]]

(In reply to Dão Gottwald [:dao] from comment #36)

> > No, Larissa and others had discussed this earlier when we came up with the
> > text.  Users don't know who the site "owner" or "admin" is.
> 
> Where are they supposed to send the email then?

I think I previously mentioned this in an IRC discussion, because I can't find it here... I removed the "email" thing from my last version of the patch because I was concerned about us asking the user to do something that's not really clear how to do. It would certainly take _me_ an unpleasant amount of effort to figure out, I don't think we should ask (or expect) users to do so.

Comment 40

[Larissa Co [:lco]]

(In reply to Justin Dolske [:Dolske] from comment #39)
> (In reply to Dão Gottwald [:dao] from comment #36)
> 
> > > No, Larissa and others had discussed this earlier when we came up with the
> > > text.  Users don't know who the site "owner" or "admin" is.
> > 
> > Where are they supposed to send the email then?
> 
> I think I previously mentioned this in an IRC discussion, because I can't
> find it here... I removed the "email" thing from my last version of the
> patch because I was concerned about us asking the user to do something
> that's not really clear how to do. It would certainly take _me_ an
> unpleasant amount of effort to figure out, I don't think we should ask (or
> expect) users to do so.

I'm willing to back down from this if only so that we can stop arguing about it. The intent of this phrase was really to help the user do something useful rather than choosing to be insecure because he wants to get his task done.

So go with either:

"Most websites will still work properly even when this content is blocked", or
"This site may still work properly even while insecure content is blocked."

But honestly, we have bigger things to focus on other than this string, and at some point, we just have to call it good enough. (FWIW I prefer the first string still despite being not specific because the second is wishy-washy.)

Comment 41

[Justin Dolske [:Dolske]]

So lco and I chatted about this for a bit and agreed on doing a few things:

* What happens when clicking "Keep Blocking"?

Currently, as is normal for a doorhanger, clicking a button action (other than Not Now) will dismiss the prompt and remove the icon. We should land as-is in that respect. We should spin off a followup bug to look at making it possible to have the notification's icon persist (ie, so you can bring the prompt back to click Unblock). While I'm not fully 100% sure that's what we should eventually do, lco convinced me it's a pretty sensible UI. (It's also basically what Chrome does, although wow their UI is absolutely terrible and unpolished!)

* String regarding "email the site"

We should remove this, even for initial Nightly landing. We can tweak it later, as is always fair game. (Whatever lands on Aurora shouldn't change until at least next cycle, though.) I completely agree with the intended purpose of the string, just not the specific action it suggested... It would be great if we could come up with a replacement (in bug 827595!). One interesting angle would be to try something with a bit of whimsy, since cold technical language is hard to get correct, brief, and understandable. Although whimsy in security UI does need to be approached carefully! :)

* String regarding "this site / most sites"

Either of the strings in comment 40 are fine with me, or other variations thereof. I originally raised the issue way back in comment 19 to see if it had been considered or it led to better wording... It has now been considered and there's no clearly superior wording, so I'm ok with whatever lco/tanvi/ux/matej want to roll with.

* More Info…

I mentioned that "More Info" should probably be "More Info…" (with ellipsis), and lco said she already had tanvi fix that. So we're good there. :D

Comment 42

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - v6

* I discussed OnSecurityChange with Justin on #fx-team.  We need to call OnSecurityChange in nsMixedContentBlocker.cpp to handle mixed content loads and edge cases.

* Removed the email this site text.

* Took out the Learn more elipse per Dao.  There is no More Info in this patch.

* Went with ""Most websites will still work properly even when this content is blocked." per Larissa (and Justin who said he was fine with either string).

Please let me know if there are any remaining issues.

Comment 43

[Dão Gottwald [:dao]]

Comment on attachment 699547 [details] [diff] [review]
Mixed Content Blocker Doorhanger - v6

>+    <popupnotification id="mixed-content-blocked-notification" hidden="true">
>+      <popupnotificationcontent orient="vertical" align="start">
>+          <separator/>
>+          <description id="mixed-content-blocked-moreinfo">&mixedContentBlocked.moreinfo;</description>
>+          <separator/>
>+          <spacer flex="1"/>
>+          <label id="mixed-content-blocked-helplink" class="text-link"
>+                 value="&mixedContentBlocked.helplink;"/>
>+      </popupnotificationcontent>
>+    </popupnotification>

Remove <spacer flex="1"/>

>--- a/browser/locales/en-US/chrome/browser/browser.dtd
>+++ b/browser/locales/en-US/chrome/browser/browser.dtd
>@@ -625,10 +625,14 @@ just addresses the organization to follo
> <!ENTITY social.chatBar.label "Focus chats">
> <!ENTITY social.chatBar.accesskey "c">
> 
> <!ENTITY getUserMedia.selectCamera.label "Camera to share:">
> <!ENTITY getUserMedia.selectCamera.accesskey "C">
> <!ENTITY getUserMedia.selectMicrophone.label "Microphone to share:">
> <!ENTITY getUserMedia.selectMicrophone.accesskey "M">
> 
>+<!ENTITY mixedContentBlocked.helplink "Learn more">
>+<!ENTITY mixedContentBlocked.moreinfo "Most websites will still work properly even when this content is blocked.">
>+
>+
> <!ENTITY webrtcIndicatorButton.label "Camera / Microphone Access">
> <!ENTITY webrtcIndicatorButton.tooltip "Display sites you are currently sharing your camera or microphone with">

Move the new strings to the end of this file

>--- a/browser/locales/en-US/chrome/browser/browser.properties
>+++ b/browser/locales/en-US/chrome/browser/browser.properties
>@@ -448,11 +448,19 @@ identity.loggedIn.signOut.accessKey = O
> # The number of devices can be either one or two.
> getUserMedia.shareCamera.message = Would you like to share your camera with %S?
> getUserMedia.shareMicrophone.message = Would you like to share your microphone with %S?
> getUserMedia.shareCameraAndMicrophone.message = Would you like to share your camera and microphone with %S?
> getUserMedia.shareSelectedDevices.label = Share Selected Device;Share Selected Devices
> getUserMedia.shareSelectedDevices.accesskey = S
> getUserMedia.denyRequest.label = Don't Share
> getUserMedia.denyRequest.accesskey = D
>+
>+# Mixed Content Blocker Doorhanger Notification
>+mixedContentBlocked.message = Firefox has blocked content that isn't secure.
>+mixedContentBlocked.keepBlockingButton.label = Keep Blocking
>+mixedContentBlocked.keepBlockingButton.accesskey = B
>+mixedContentBlocked.unblockButton.label = Disable protection on this page
>+mixedContentBlocked.unblockButton.accesskey = D
>+
> getUserMedia.sharingCamera.message = You are currently sharing your camera with %S.
> getUserMedia.sharingMicrophone.message = You are currently sharing your microphone with %S.
> getUserMedia.sharingCameraAndMicrophone.message = You are currently sharing your camera and microphone with %S.

- Move the new strings to the end of this file
- Need to use brandShortName rather than hardcoding "Firefox" in the message
- "Disable protection on this page" should use title capitalization
  -> "Disable Protection on This Page"
- Apparently mixedContentBlocked.unblockButton.label isn't actually used for a button, so the string name should be updated to reflect that (just mixedContentBlocked.unblock.label would be fine).

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css

>+#mixed-content-blocked-helplink {
>+  margin: 0px;
>+}

As mentioned in comment 38, only pinstripe wants this.

Comment 44

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - v7

Comments addressed.  Although I'm not sure if I did the right thing for brandShortName.

Comment 45

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - v8

Fixed brandShortName.

Comment 46

[Dão Gottwald [:dao]]

(In reply to Justin Dolske [:Dolske] from comment #37)
> Actually, now that I look at the code again, I suspect it's due to this
> patch's change in onSecurityChange()... The (existing) comment there says
> "Don't need to do anything if the data we use to update the UI hasn't
> changed", but the code this patch adds breaks that suppression -- for all
> security changes -- whenever mixed content is present.
> 
> I think that needs to be fixed. One of the base patches is adding a
> STATE_IS_BROKEN, so it seems like onSecurityChange() shouldn't need to
> change at all. Is |this._state| not getting updated somewhere?

Have you discussed this point on IRC last night? Have you reached consensus?

Comment 47

[Tanvi Vyas[:tanvi]]

(In reply to Dão Gottwald [:dao] from comment #46)
> (In reply to Justin Dolske [:Dolske] from comment #37)
> > Actually, now that I look at the code again, I suspect it's due to this
> > patch's change in onSecurityChange()... The (existing) comment there says
> > "Don't need to do anything if the data we use to update the UI hasn't
> > changed", but the code this patch adds breaks that suppression -- for all
> > security changes -- whenever mixed content is present.
> > 

That is because the UI does change in all cases where Mixed Active Content is present.  So we add the following conditions to the if statement here - https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4231:
!gBrowser.docShell.hasMixedActiveContentBlocked &&
!gBrowser.docShell.hasMixedActiveContentLoaded

(Originally we were going to add another security state in PSM for mied active content.  But bsmith advised against that, and to use the mixed content content policy to surface this instead since nsSecureBrowserUIImpl may be re-written / go away.)


> > I think that needs to be fixed. One of the base patches is adding a
> > STATE_IS_BROKEN, so it seems like onSecurityChange() shouldn't need to
> > change at all. Is |this._state| not getting updated somewhere?
> 
> Have you discussed this point on IRC last night? Have you reached consensus?

I don't see a way around calling OnSecurityChange in nsMixedContentBlocker on load eveents without missing some mixed active content cases. I could add a condition to onSecurityChange in browser.js so that if the state hasn't changed and we do have mixed content, we could bypass the code that checks the security level here: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4258

Comment 48

[Dão Gottwald [:dao]]

What I think you should do is cache gBrowser.docShell.hasMixedActiveContentBlocked just like we're caching aState.

Comment 49

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - v9

(In reply to Dão Gottwald [:dao] from comment #48)
> What I think you should do is cache
> gBrowser.docShell.hasMixedActiveContentBlocked just like we're caching
> aState.

How is this?  I created _hasMixedActiveContentBlocked and _hasMixedActiveContentLoaded.

Comment 50

[Justin Dolske [:Dolske]]

This issue that the the existing throttling in onSecurityChange (the if-check at the top of that function) is effectively now bypassed when a page is in mixed-content mode.

I did some digging. Indeed, this was added for performance reasons back in bug 397492. It's still sometimes called frequently... On a few random sites, I usually see just a few calls per page (< 5), but sometimes I see bunch more. YouTube, in particular, sometimes triggers 20+ calls during a page load, with more calls as I interact with the page.

Gavin: Any thoughts on this? I'm not sure if this is actually likely to be a real-world or talos-world problem.


In any case, I'm also trying to understand the thinking in the bug 822367 patch. It's adding the calls to onSecurityChange, but isn't adding any new state flags, so the callee can't actually see what state has changed. That seems... odd. It's essentially using this API as a hole to poke at the front end and force it check the gBrowser.docShell.hasMixedActiveContent* properties.

Seems like this _should_ be adding a STATE_IS_MIXED flag to nsIWebProgressListener (or 2 flags, one for each flavor), and using that flag to drive the front end's usual onSecurityChange code.

This seems hard to describe, let me whip up a version that tries what I'm describing to see if I can explain it better in code. :)


[Lastly: Oops, hadn't seen Dao's comment abotu the ellipsis. And, nice, we're  totally inconsistent on "Learn More" vs "Learn More…" already... I found 8 of the former, 5 of the latter. I guess I don't care! :S]

Comment 51

[Justin Dolske [:Dolske]]

Attachment: What adding a new state flag might look like

Here's an example of what I was trying to describe. Not polished code, but seems to work. (This is a diff on top of the other patches here.)

I didn't quite realize how much PSM got involved in the state management, which makes things uglier (but needed to make switching to a tab which loaded blocked content in the background work right).

Thoughts?

Comment 52

[:Gavin Sharp [email: [e-mail]]]

It was probably more of a concern back when the default-visible security UI was more complicated (e.g. "identity" block" that needed updating for SSL, as opposed to the current lock), but I think it's still code that we need to avoid de-optmizing, until we have a better onSecurityChange that more closely maps to what the UI actually cares about.

Comment 53

[Dão Gottwald [:dao]]

Comment on attachment 700171 [details] [diff] [review]
Mixed Content Blocker Doorhanger - v9

>   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._hasMixedActiveContentBlocked == gBrowser.docShell.hasMixedActiveContentBlocked &&
>+        this._hasMixedActiveContentLoaded == gBrowser.docShell.hasMixedActiveContentLoaded &&

Assign gBrowser.docShell.hasMixedActiveContentBlocked / ...Loaded to local variables at the beginning of this method and use them from then on.

>     let nsIWebProgressListener = Ci.nsIWebProgressListener;
>     if (location.protocol == "chrome:" || location.protocol == "about:") {
>       this.setMode(this.IDENTITY_MODE_CHROMEUI);
>     } else if (state & nsIWebProgressListener.STATE_IDENTITY_EV_TOPLEVEL) {
>       this.setMode(this.IDENTITY_MODE_IDENTIFIED);
>+      if (gBrowser.docShell.hasMixedActiveContentBlocked) {
>+        this._hasMixedActiveContentBlocked = gBrowser.docShell.hasMixedActiveContentBlocked;
>+        this.showMixedContentDoorhanger();
>+      }
>     } else if (state & nsIWebProgressListener.STATE_IS_SECURE) {
>       this.setMode(this.IDENTITY_MODE_DOMAIN_VERIFIED);
>+      if (gBrowser.docShell.hasMixedActiveContentBlocked) {
>+        this._hasMixedActiveContentBlocked = gBrowser.docShell.hasMixedActiveContentBlocked;
>+        this.showMixedContentDoorhanger();
>+      }
>     } else if (state & nsIWebProgressListener.STATE_IS_BROKEN) {
>       if (gBrowser.docShell.hasMixedActiveContentLoaded) {
>+        this._hasMixedActiveContentBlocked = gBrowser.docShell.hasMixedActiveContentBlocked;
>         this.setMode(this.IDENTITY_MODE_MIXED_ACTIVE_CONTENT);
>       } else {
>         this.setMode(this.IDENTITY_MODE_MIXED_CONTENT);
>       }
>     } else {
>       this.setMode(this.IDENTITY_MODE_UNKNOWN);
>-    }
>+      if (gBrowser.docShell.hasMixedActiveContentBlocked) {
>+        this._hasMixedActiveContentBlocked = gBrowser.docShell.hasMixedActiveContentBlocked;
>+        this.showMixedContentDoorhanger();
>+      }
>+    }

You never update this._hasMixedActiveContentLoaded. It should unconditionally and only once be set along with this._hasMixedActiveContentBlocked.

Comment 54

[Dão Gottwald [:dao]]

Comment on attachment 700185 [details] [diff] [review]
What adding a new state flag might look like

This also looks ok to me.

Comment 55

[Tanvi Vyas[:tanvi]]

Justin, see comments 12-15 on bug 782654: https://bugzilla.mozilla.org/show_bug.cgi?id=782654#c12

I was originally going to add a new mixed active content state, like your patch does, but I was advised against this.

Comment 56

[Justin Dolske [:Dolske]]

(In reply to Tanvi Vyas [:tanvi] from comment #55)
> Justin, see comments 12-15 on bug 782654:
> https://bugzilla.mozilla.org/show_bug.cgi?id=782654#c12
> 
> I was originally going to add a new mixed active content state, like your
> patch does, but I was advised against this.

So I guess we're really looking for bsmith's opinion on this... A lot of those comments were specific to storing state on the document instead of channel (now fixed), and I'm not sure how firm his other advice was.

Especially given that nsSecureBrowserUIImpl is known to be craptastic, but by avoiding makes changes there we're just squishing the crap around, and making it show up in the front-end as an API problem. So: given the existing issues with nsSecureBrowserUIImpl and it now having impact on the FE patch, is it still the right call to avoid sullying nsSecureBrowserUIImpl?

Another possibility (that squishes things in a different direction) would be to make a wrapper around nsSecureBrowserUIImpl, which ORs together the state from the actual nsSecureBrowserUIImpl and the mixed content bits from the docshell. This seems possible because it appears that browser.xml (?!) is responsible for creating docshell.securityUI, and so we could jam in such a wrapper instead. That would give us a sensible onSecurityChange API, but also avoid the need to modify PSM.

Comment 57

[Tanvi Vyas[:tanvi]]

Spoke to Stephen today and he said the icons in the patch are fine for now.  Removing the need info.

Comment 58

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - v10

Added a new patch that should take care of the performance issues while we discuss how and if we should change the backend (since any backend changes will take a couple of weeks and we'd like to start getting feedback from nightly users sooner than later).  With these changes, there is one call to gBrowser.docShell.hasMixedActiveContentBlocked and one call to gBrowser.docShell.hasMixedActiveContentBlocked, but only if the pref security.mixed_content.block_active_content is on.

Justin, what do you think of this?

Comment 59

[Justin Dolske [:Dolske]]

Comment on attachment 701683 [details] [diff] [review]
Mixed Content Blocker Doorhanger - v10

Tanvi+bsmith+I met to talk about this yesterday... The long term plan is to keep onSecurityChange's API around (although we very much want to clean up the nsISecureBrowserUIImpl cruft). As such, this API should work by passing the necessary state info in through flags, as my earlier POC patch did. I'm not keen on taking the current patch, because it's changing around onSecurityChange in ways that we know we'd just be reverting. Let's just do it right.

Tanvi's won't be able to update the patch for a short while, so I volunteered to make my patch less hacky to help move this along.

Comment 60

[Justin Dolske [:Dolske]]

Attachment: State flags v2

Updated my previous proof-of-concept with the 2 state flags we discussed. This is based on top of the v9 patch here, since v10 was just further changing the code this would replace anyway.

Just attaching this patch as a reference, will next fold pieces into the other relevant patches/bugs.

Comment 61

[Justin Dolske [:Dolske]]

Attachment: Mixed Content Blocker Doorhanger - v11

For those building along at home:

1) Apply attachment 698022 [details] [diff] [review] from bug 822366
2) Apply attachment 704380 [details] [diff] [review] from bug 822367
3) Apply attachment 699512 [details] [diff] [review] from bug 822367 (probably optional, but meh)
4) Apply this patch

Test page: https://people.mozilla.com/~tvyas/mixedcontent.html
(try with and without flipping the security.mixed_content.block_active_content pref

[The patch in attachment 704378 [details] [diff] [review] -- "State flags v2" -- is now folded across this patch and attachment 704380 [details] [diff] [review]]

Comment 62

[Justin Dolske [:Dolske]]

Attachment: Mixed Content Blocker Doorhanger - v12

Oops, forgot to remove a couple now-unused vars that snuck in. And zapped a couple of whitespace changes that didn't need to be here, for clarity.

Comment 63

[Tanvi Vyas[:tanvi]]

Comment on attachment 704382 [details] [diff] [review]
Mixed Content Blocker Doorhanger - v12

This patch looks good.  I like how you call showMixedContentDoorhanger() at the bottom of checkIdentity, instead of mixing it into the nested if-else's.

Comment 64

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - v13

Updated the patch with the new flag names (STATE_LOADED_MIXED_ACTIVE_CONTENT and STATE_BLOCKED_MIXED_ACTIVE_CONTENT) from the backend bug 822367.

r? to dao.

Comment 65

[Dão Gottwald [:dao]]

Comment on attachment 705690 [details] [diff] [review]
Mixed Content Blocker Doorhanger - v13

>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -466,16 +466,27 @@
>     </popupnotification>
> 
>     <popupnotification id="geolocation-notification" hidden="true">
>       <popupnotificationcontent orient="vertical" align="start">
>         <separator class="thin"/>
>         <label id="geolocation-learnmore-link" class="text-link"/>
>       </popupnotificationcontent>
>     </popupnotification>
>+
>+    <popupnotification id="mixed-content-blocked-notification" hidden="true">
>+      <popupnotificationcontent orient="vertical" align="start">
>+          <separator/>
>+          <description id="mixed-content-blocked-moreinfo">&mixedContentBlocked.moreinfo;</description>
>+          <separator/>
>+          <label id="mixed-content-blocked-helplink" class="text-link"
>+                 value="&mixedContentBlocked.helplink;"/>
>+      </popupnotificationcontent>
>+    </popupnotification>

Everything inside the popupnotificationcontent node is indented two spaces too far.

>--- a/browser/locales/en-US/chrome/browser/browser.properties
>+++ b/browser/locales/en-US/chrome/browser/browser.properties
>@@ -448,11 +448,20 @@ identity.loggedIn.signOut.accessKey = O
> # The number of devices can be either one or two.
> getUserMedia.shareCamera.message = Would you like to share your camera with %S?
> getUserMedia.shareMicrophone.message = Would you like to share your microphone with %S?
> getUserMedia.shareCameraAndMicrophone.message = Would you like to share your camera and microphone with %S?
> getUserMedia.shareSelectedDevices.label = Share Selected Device;Share Selected Devices
> getUserMedia.shareSelectedDevices.accesskey = S
> getUserMedia.denyRequest.label = Don't Share
> getUserMedia.denyRequest.accesskey = D
>+
> getUserMedia.sharingCamera.message = You are currently sharing your camera with %S.
> getUserMedia.sharingMicrophone.message = You are currently sharing your microphone with %S.
> getUserMedia.sharingCameraAndMicrophone.message = You are currently sharing your camera and microphone with %S.

Don't add this blank line.

>+# LOCALIZATION NOTE - %1$S is brandShortName
>+mixedContentBlocked.message = %1$S has blocked content that isn't secure.

Write %S instead of %1$S in the localization note and in the string itself.

>--- a/browser/themes/pinstripe/browser.css
>+++ b/browser/themes/pinstripe/browser.css

>+@media (min-resolution: 2dppx) {
>+  #mixed-content-blocked-more-button {
>+    list-style-image: url("chrome://browser/skin/[e-mail]");
>+  }
>+}

Remove this.

AFAIK this patch should apply without conflicts on mozilla-central tip since it doesn't depend on other bugs touching the same files. It looks however like it won't apply cleanly. Please attach a patch that does.

Comment 66

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - v14

Comments addressed.

Comment 67

[Dão Gottwald [:dao]]

Comment on attachment 705978 [details] [diff] [review]
Mixed Content Blocker Doorhanger - v14

(In reply to Dão Gottwald [:dao] from comment #65)
> >--- a/browser/themes/pinstripe/browser.css
> >+++ b/browser/themes/pinstripe/browser.css
> 
> >+@media (min-resolution: 2dppx) {
> >+  #mixed-content-blocked-more-button {
> >+    list-style-image: url("chrome://browser/skin/[e-mail]");
> >+  }
> >+}
> 
> Remove this.
> 
> AFAIK this patch should apply without conflicts on mozilla-central tip since
> it doesn't depend on other bugs touching the same files. It looks however
> like it won't apply cleanly. Please attach a patch that does.

Seems like you didn't actually address these points.

Comment 68

[Tanvi Vyas[:tanvi]]

Attachment: Mixed Content Blocker Doorhanger - v15

(In reply to Dão Gottwald [:dao] from comment #67)
> Comment on attachment 705978 [details] [diff] [review]
> Mixed Content Blocker Doorhanger - v14
> 
> (In reply to Dão Gottwald [:dao] from comment #65)
> > >--- a/browser/themes/pinstripe/browser.css
> > >+++ b/browser/themes/pinstripe/browser.css
> > 
> > >+@media (min-resolution: 2dppx) {
> > >+  #mixed-content-blocked-more-button {
> > >+    list-style-image: url("chrome://browser/skin/[e-mail]");
> > >+  }
> > >+}
> > 
> > Remove this.

> Seems like you didn't actually address these points.

Sorry, removed the wrong png.  I've updated this.

Dao, does the code that has changed from the reviews a couple weeks ago look okay now (the browser.js code)?  I no longer touch the checkIdentity() function.  The rest of the files in the patch haven't changed.

Comment 69

[Dão Gottwald [:dao]]

Comment on attachment 706132 [details] [diff] [review]
Mixed Content Blocker Doorhanger - v15

Again, please attach a patch that applies on mozilla-central tip.

applying https://bugzilla.mozilla.org/attachment.cgi?id=706132
patching file browser/base/content/browser.js
Hunk #1 FAILED at 6660
1 out of 1 hunks FAILED -- saving rejects to file browser/base/content/browser.js.rej
patching file browser/themes/gnomestripe/jar.mn
Hunk #1 FAILED at 24
1 out of 1 hunks FAILED -- saving rejects to file browser/themes/gnomestripe/jar.mn.rej
patching file browser/themes/pinstripe/jar.mn
Hunk #1 FAILED at 34
1 out of 1 hunks FAILED -- saving rejects to file browser/themes/pinstripe/jar.mn.rej
patching file browser/themes/winstripe/jar.mn
Hunk #1 succeeded at 34 with fuzz 2 (offset -1 lines).
Hunk #2 succeeded at 261 with fuzz 2 (offset -2 lines).
abort: patch failed to apply

Comment 70

[Tanvi Vyas[:tanvi]]

Comment on attachment 706132 [details] [diff] [review]
Mixed Content Blocker Doorhanger - v15

(In reply to Dão Gottwald [:dao] from comment #69)
> Comment on attachment 706132 [details] [diff] [review]
> Mixed Content Blocker Doorhanger - v15
> 
> Again, please attach a patch that applies on mozilla-central tip.
> 
> applying https://bugzilla.mozilla.org/attachment.cgi?id=706132
> patching file browser/base/content/browser.js
> Hunk #1 FAILED at 6660
> 1 out of 1 hunks FAILED -- saving rejects to file
> browser/base/content/browser.js.rej
> patching file browser/themes/gnomestripe/jar.mn
> Hunk #1 FAILED at 24
> 1 out of 1 hunks FAILED -- saving rejects to file
> browser/themes/gnomestripe/jar.mn.rej
> patching file browser/themes/pinstripe/jar.mn
> Hunk #1 FAILED at 34
> 1 out of 1 hunks FAILED -- saving rejects to file
> browser/themes/pinstripe/jar.mn.rej
> patching file browser/themes/winstripe/jar.mn
> Hunk #1 succeeded at 34 with fuzz 2 (offset -1 lines).
> Hunk #2 succeeded at 261 with fuzz 2 (offset -2 lines).
> abort: patch failed to apply

I did a pull and update earlier today on mozilla-central.  My tip is:
changeset:   119737:fa969919b1bb
tag:         qparent
parent:      119641:689690a17de3
parent:      119736:c71ba28efaa0
user:        Ryan VanderMeulen <[e-mail]>
date:        Fri Jan 25 12:33:43 2013 -0500
files:       browser/base/content/test/social/browser_social.js content/svg/content/src/nsSVGMaskElement.cpp content/svg/content/src/nsSVGMaskElement.h editor/libeditor/html/tests/test_bug832025_2.html gfx/thebes/woff-private.h gfx/thebes/woff.c gfx/thebes/woff.h
description:
Merge the last PGO-green inbound changeset to m-c.

Remember that you have to apply the bug in bug 822366 first (as mentioned in comment 22 and 61).  These are both applying cleanly for me.

Comment 71

[Dão Gottwald [:dao]]

Sigh. Please use bug dependencies to make such relationships clear without requiring people to wade through dozens of comments. (I have to wonder, though: If these front-end patches are tied together, why are they in separate bugs? If they are independent, why hasn't bug 822366's patch landed yet?)

Comment 72

[Tanvi Vyas[:tanvi]]

(In reply to Dão Gottwald [:dao] from comment #71)
> Sigh. Please use bug dependencies to make such relationships clear without
> requiring people to wade through dozens of comments.
Sorry, I didn't know that it was common practice to add dependencies for patches that needed to be applied first.  I had only added the dependence for the master bug.  I will do this in the future.

> (I have to wonder,
> though: If these front-end patches are tied together, why are they in
> separate bugs? If they are independent, why hasn't bug 822366's patch landed
> yet?)

See comments 131, 132, and 133 in bug 782654 - https://bugzilla.mozilla.org/show_bug.cgi?id=782654#c131.  I had everything in one bug originally, including the patch that is now in bug 822366 and was r+'ed by you.  I was advised to split these up into frontend and backend parts.  Bug 822366 is separate from this one (it uses a different icon in the site identity box for mixed active content) so I created a separate bug for it.  It has been ready to land for a while.  But I didn't want to land that separately from the rest of the mixed content stuff, since I'd rather users see one change instead of seeing our UI change once with the new icon and then change again later with the mixed content doorhanger.

Comment 73

[Tanvi Vyas[:tanvi]]

Here is a new push to try that includes this bug, bug 822366 and bug 822367:
https://tbpl.mozilla.org/?tree=Try&rev=0ac83df1ce4f

Comment 74

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #73)
> Here is a new push to try that includes this bug, bug 822366 and bug 822367:
> https://tbpl.mozilla.org/?tree=Try&rev=0ac83df1ce4f

Updated try: https://tbpl.mozilla.org/?tree=Try&rev=21a963539714

Comment 75

[Tanvi Vyas[:tanvi]]

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a74d6901fd71

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a74d6901fd71

Comment 77

[Valerio]

Attachment: missing icon

The Windows version is missing the icon for this code:

+.popup-notification-icon[popupid="mixed-content-blocked"] {
+  list-style-image: url(chrome://browser/skin/mixed-content-blocked-64.png);
+}

In the version for mac the icon is present: