The mixed content shield notification icon in the URL bar should not go away when mixed content blocking is disabled for that page. This will make it easy to re-enable that protection (and other types of protection) after it has been disabled. When any such protection is disabled, the icon should be replaced with a shield with a strike through it (with all other behaviour the same). Mockup: attachment 8459851 [details]

Note: There should be no change in how the grey/yellow triangle for the site identity currently works.

I think this bug applies to the existing popup notifications module. Extending the module to meet our need of bundling mixed content with other types of security notifications avoids the primary/secondary action interface completely. Therefore the notification icon appears to stay in place.

While the above is true we were not acting upon the new security state when the page was reloaded protection-free. I guess because we currently do not have a different shield icon to show. The patch I've uploaded fixes that but shows the same icon. We need a new icon.

FYI we are unable to provide a re-enable mixed content protection button because of bug 1045809

Mochitests must now not only test the presence of the doorhanger but also its state through the function _IsMixedContentBlocked() exposed by the notification

Comment on attachment 8467461 [details] [diff] [review] checkIdentity following onSecurityChange now shows doorhanger on STATE_LOADED_MIXED_ACTIVE_CONTENT. Doorhanger now acknowledges this new state Now the doorhanger shows up when mixed content is being loaded (white-listed) in addition to when content is being blocked (current behavior).

Philipp: Do we have an asset for the shield with a strike through it?

Comment on attachment 8469358 [details] [diff] [review] checkIdentity following onSecurityChange now shows doorhanger on STATE_LOADED_MIXED_ACTIVE_CONTENT. Doorhanger now acknowledges this new state Review of attachment 8469358 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +6596,5 @@ > if (state & nsIWebProgressListener.STATE_BLOCKED_MIXED_ACTIVE_CONTENT) > this.showBadContentDoorhanger(state); > + > + // Ensure the doorhanger is shown when mixed active content is detected but not blocked. > + if (state & nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT) Feel like this should just be added to the above if-condition, since you'll be adding two more possibilities for tracking protection and we don't want to call showBadContentDoorhander multiple times. ::: browser/base/content/urlbarBindings.xml @@ +1594,5 @@ > <xul:description class="popup-notification-item-message" xbl:inherits="popupid"> > &mixedContentBlocked2.moreinfo; > </xul:description> > <xul:label anonid="mixedContent.helplink" class="text-link" href="" value="&mixedContentBlocked2.learnMore;"/> > + <xul:description anonid="mixedContentProtectionDisabled" hidden="true" class="popup-notification-item-message popup-notification-item-message-critical" xbl:inherits="popupid"> Wrap to ~80 characters per line? @@ +1620,5 @@ > } > + if (this.notification.options.state & Ci.nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT) { > + document.getAnonymousElementByAttribute(this, "anonid", "mixedContent").hidden = false; > + document.getAnonymousElementByAttribute(this, "anonid", "mixedContentProtectionDisabled").hidden = false; > + document.getAnonymousElementByAttribute(this, "anonid", "mixedContent.helplink").href = Services.urlFormatter.formatURLPref("app.support.baseURL") + "mixed-content"; Now that you're using these elements multiple times, you should store them in fields. @@ +1634,5 @@ > // Reload the page with the content unblocked > BrowserReloadWithFlags(nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT); > ]]></body> > </method> > + <method name="_IsMixedContentBlocked"> We generally try to avoid introducing methods that are only used for testing. ::: browser/themes/shared/badcontent-doorhanger.inc.css @@ +17,5 @@ > width: 200px; > } > > +.popup-notification-item-message-critical[popupid="bad-content"] { > + color: red; As per Chameleon, this should be #d74345 (see error red - http://people.mozilla.org/~jgruen/chameleon/#nav5)

Here are the assets for the shield

Comment on attachment 8473421 [details] [diff] [review] checkIdentity following onSecurityChange now shows doorhanger on STATE_LOADED_MIXED_ACTIVE_CONTENT. Doorhanger now acknowledges this new state Don't forget to tag me for review - too easy to miss these otherwise!

Comment on attachment 8473884 [details] [diff] [review] checkIdentity following onSecurityChange now shows doorhanger on STATE_LOADED_MIXED_ACTIVE_CONTENT. Doorhanger now acknowledges this new state Review of attachment 8473884 [details] [diff] [review]: ----------------------------------------------------------------- Addressed review comments. We do need some way to test the state of the doorhanger, either with a method or a field that aliases to 'notification.state'. Right now we are doing the first but I guess we could do the latter.

Hey Philipp, if protection is disabled and the strike-through shows in the location bar, should the strike-through also be in the doorhanger? If so, could we get a larger version of strike-through shield? https://bug1043803.bugzilla.mozilla.org/attachment.cgi?id=8473422

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #28) > Hey Philipp, if protection is disabled and the strike-through shows in the > location bar, should the strike-through also be in the doorhanger? If so, > could we get a larger version of strike-through shield? > > https://bug1043803.bugzilla.mozilla.org/attachment.cgi?id=8473422 Good point. Yes, there should be a strikethrough. I'll post an asset here as soon as I have it.

(In reply to gkontaxis from comment #32) > Created attachment 8476795 [details] [diff] [review] > checkIdentity following onSecurityChange now shows doorhanger on > STATE_LOADED_MIXED_ACTIVE_CONTENT. Doorhanger now acknowledges this new state This one carries over changes made after bug 1043797 was reviewed. Minor stuff (renaming, space trimming)

(In reply to gkontaxis from comment #36) > Created attachment 8477535 [details] [diff] [review] > checkIdentity following onSecurityChange now shows doorhanger on > STATE_LOADED_MIXED_ACTIVE_CONTENT. Doorhanger now acknowledges this new state minor (major? ) fix. Broken shield icon did not appear on Windows when an Aero theme was used. Entry was missing from jar.mn

Comment on attachment 8477004 [details] [diff] [review] checkIdentity following onSecurityChange now shows doorhanger on STATE_LOADED_MIXED_ACTIVE_CONTENT. Doorhanger now acknowledges this new state Review of attachment 8477004 [details] [diff] [review]: ----------------------------------------------------------------- Here are comments I had saved while working on the version of this patch posted yesterday... I'll start reviewing the new patch now, but I won't copy the comments below there. ::: browser/base/content/browser.js @@ +6589,5 @@ > + // - mixed active content is blocked > + // - mixed active content is loaded (detected but not blocked) > + if (state & > + (nsIWebProgressListener.STATE_BLOCKED_MIXED_ACTIVE_CONTENT > + |nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT)) { Nit: Please put the | at the end of the previous line with a space before it. @@ +6596,4 @@ > }, > > showBadContentDoorhanger : function(state) { > + let nsIWebProgressListener = Ci.nsIWebProgressListener; Nit: I don't think this is helpful. ::: browser/base/content/urlbarBindings.xml @@ +1617,5 @@ > </xul:menupopup> > </xul:button> > </xul:hbox> > + <xul:hbox anonid="mixedContentProtectionDisabled" hidden="true" > + class="popup-notification-footer" xbl:inherits="popupid"> Nit: We usually indent so that the first attribute names on all the lines line up, like: <xul:hbox anonid="" hidden="" class="" xbl:inherits="" foo=""> @@ +1647,5 @@ > "mixedContentAction.unblock") > </field> > + <field name="_mixedContentProtectionDisabledWarning"> > + document.getAnonymousElementByAttribute(this, "anonid", > + "mixedContentProtectionDisabled") Nit: Please indent the third arg with the open paren, or at least two spaces if that's what your patch in bug 1043797 mostly does. @@ +1671,5 @@ > Services.urlFormatter.formatURLPref("app.support.baseURL") > + "mixed-content"; > } > + if (this.notification.options.state & > + Ci.nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT) { Nit: Please indent the second line with the open paren.

Comment on attachment 8477535 [details] [diff] [review] checkIdentity following onSecurityChange now shows doorhanger on STATE_LOADED_MIXED_ACTIVE_CONTENT. Doorhanger now acknowledges this new state Review of attachment 8477535 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_bug822367.js @@ +57,2 @@ > PopupNotifications.panel.firstChild.disableMixedContentProtection(); > + notification.remove(); Why does this patch make it necessary to call remove()? ::: browser/base/content/urlbarBindings.xml @@ +1653,1 @@ > <field name="_mixedContentHelplink"> Nit: Would be nice to capitalize Link. @@ +1674,5 @@ > + if (this.notification.options.state & > + Ci.nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT) { > + _mixedContent.hidden = false; > + _mixedContentProtectionDisabledWarning.hidden = false; > + _mixedContentHelplink.href = Nit: This link is set in both this case and the STATE_BLOCKED_MIXED_ACTIVE_CONTENT case above. Since each case is an `if`, instead of the second case being an `else if`, at first glance it kind of looks like an error. It happens that the URL is the same in both cases, so the intent would be a little clearer if you wrote if (state & (BLOCKED | LOADED)) { _mixedContentHelplink.href = ...; } I'm really nitpicking here, though. @@ +1692,5 @@ > BrowserReloadWithFlags( > nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT); > ]]></body> > </method> > + <method name="_IsMixedContentBlocked"> I see that Blair said we don't usually expose things for tests like this. You can access the .notifications property from test code and then do this check there. But, I actually think that having a "public" (meaning non-underscored) isMixedContentBlocked property is perfectly reasonable, even if it is only used in tests. So how about changing the name to that and making it a read-only property instead of a method?

(In reply to Drew Willcoxon :adw from comment #39) > Comment on attachment 8477535 [details] [diff] [review] > checkIdentity following onSecurityChange now shows doorhanger on > STATE_LOADED_MIXED_ACTIVE_CONTENT. Doorhanger now acknowledges this new state > > Review of attachment 8477535 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/test/general/browser_bug822367.js > @@ +57,2 @@ > > PopupNotifications.panel.firstChild.disableMixedContentProtection(); > > + notification.remove(); > > Why does this patch make it necessary to call remove()? > Is there way to undo the outcome of notification.reshow()? It's not necessary everywhere but I had tests fail because of the shift in focus. > ::: browser/base/content/urlbarBindings.xml > @@ +1653,1 @@ > > <field name="_mixedContentHelplink"> > > Nit: Would be nice to capitalize Link. > > @@ +1674,5 @@ > > + if (this.notification.options.state & > > + Ci.nsIWebProgressListener.STATE_LOADED_MIXED_ACTIVE_CONTENT) { > > + _mixedContent.hidden = false; > > + _mixedContentProtectionDisabledWarning.hidden = false; > > + _mixedContentHelplink.href = > > Nit: This link is set in both this case and the > STATE_BLOCKED_MIXED_ACTIVE_CONTENT case above. Since each case is an `if`, > instead of the second case being an `else if`, at first glance it kind of > looks like an error. It happens that the URL is the same in both cases, so > the intent would be a little clearer if you wrote > > if (state & (BLOCKED | LOADED)) { > _mixedContentHelplink.href = ...; > } > I already have a bug which adds two more blocked/loaded states for tracking protection. So I think it's cleaner to have four blocks of code each containing everything that should apply under the respective state. > I'm really nitpicking here, though. > > @@ +1692,5 @@ > > BrowserReloadWithFlags( > > nsIWebNavigation.LOAD_FLAGS_ALLOW_MIXED_CONTENT); > > ]]></body> > > </method> > > + <method name="_IsMixedContentBlocked"> > > I see that Blair said we don't usually expose things for tests like this. > You can access the .notifications property from test code and then do this > check there. > > But, I actually think that having a "public" (meaning non-underscored) > isMixedContentBlocked property is perfectly reasonable, even if it is only > used in tests. So how about changing the name to that and making it a > read-only property instead of a method?

(In reply to gkontaxis from comment #41) > Created attachment 8477636 [details] [diff] [review] > checkIdentity following onSecurityChange now shows doorhanger on > STATE_LOADED_MIXED_ACTIVE_CONTENT. Doorhanger now acknowledges this new state This addresses both reviews from above.

https://tbpl.mozilla.org/?tree=Try&rev=abb816b7e846 just to make sure tests are working after the _IsMixedContentBlocked() -> isMixedContentBlocked change

(In reply to gkontaxis from comment #40) > > ::: browser/base/content/test/general/browser_bug822367.js > > @@ +57,2 @@ > > > PopupNotifications.panel.firstChild.disableMixedContentProtection(); > > > + notification.remove(); > > > > Why does this patch make it necessary to call remove()? > > > Is there way to undo the outcome of notification.reshow()? It's not > necessary everywhere but I had tests fail because of the shift in focus. I'm not very familiar with this, but removing after reshowing sounds like a reasonable thing to do, it's just that it's not clear why, if it wasn't necessary before this patch, it now is.

Comment on attachment 8477636 [details] [diff] [review] checkIdentity following onSecurityChange now shows doorhanger on STATE_LOADED_MIXED_ACTIVE_CONTENT. Doorhanger now acknowledges this new state Review of attachment 8477636 [details] [diff] [review]: ----------------------------------------------------------------- Carrying over adw's r+ from comment 39.

I've tested this bug using: FF 34 Build Id: 20140831030206 OS: Win 7 x64, Ubuntu 13.04 x64, Mac Os X 10.9.4 The shield icon notification disappeared on the https://people.mozilla.org/~mkelly/mixed_test.html page. Just Disable the protection on the page and the enable it back adn the shield will disappear. I did not manage to reproduce this using other pages. Is this an issue with the page or an issue with the implementation?

I see it, too, on the page you linked, but I cannot reproduce on https://people.mozilla.org/~mchew/show_ads.html. The first page has the green site identity section in the location bar but Monica's page doesn't. Maybe that's related? Could you please file a new bug?

Tanvi, I suspect this has to do with the underlying implementation of Mixed Content Blocker, not the shield being non-dismissible. Could you take a look?

Filled bug https://bugzilla.mozilla.org/show_bug.cgi?id=1063390 .

Cancelling need info, I'm following up in bug 1063390.

As the only issue encountered during testing was logged in a different bug. Marking this issue as Verified on: FF 34 Build Id: 20140831030206 OS: Win 7 x64, Ubuntu 13.04 x64, Mac Os X 10.9.4

(Bug 1063831 is a Fennec bug, and this is a desktop bug)