1043805 - Blocked content doorhanger should show a large red shield when both types of protection are disabled

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

The blocked content notification is becoming non-dismissible (bug 1043803). This means that we'll be showing the doorhanger when a type of protection is disabled on a page - and this should be made more obvious.

We can do that by adding a red "Protection is disabled" warning. And to make it more useful, add a "Report broken site" link to allow people to report when a page is broken when this protection is active.

Mockup: attachment 8459851 [details]

Comment 1

[Drew Willcoxon :adw]

Blair, there are two such warnings, one for mixed content and one for tracking, plus a shield icon struck through with a red line.  Is this bug for all of that?

Georgios, your screenshot in bug 1043803 shows one of the red "protection disabled" warnings, so have you implemented this bug, or part of this bug, in another one?  No problem if so, I'm just trying to understand everything.

Comment 2

[Drew Willcoxon :adw]

Philipp, could we get image(s) for the shield icon struck through with a red line in this mockup: attachment 8459851 [details]?

Comment 3

[Georgios Kontaxis]

I think everything but the alternate shield icon are implemented in bug 1043803. Making the shield non-dismissible presents the need to reflect its different states (protected, unprotected) so that bug includes the warning label to indicate the unprotected state.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Drew Willcoxon :adw from comment #1)
> Blair, there are two such warnings, one for mixed content and one for
> tracking, plus a shield icon struck through with a red line.  Is this bug
> for all of that?

I'd intended for this to cover that warning for both block types. But IMO it's fine if bug 1043803 does it for mixed content and bug 1043801 does it for the tracking protection blocks.

Bug 1043803 should include the icon (moving the needinfo to there).

What bug 1043803 doesn't cover at the moment is the "Report broken site link".

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

Fixed by bug 1043803. The "report broken site link" is https://bugzilla.mozilla.org/show_bug.cgi?id=1054379 and https://bugzilla.mozilla.org/show_bug.cgi?id=1055176.

Comment 6

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: large-shield.zip

Here are the large shield icons.
Sorry that took so long.

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Since phlsa uploaded the shield here, I am stealing this bug to make the strike-through consistent in the large and small icons when both types of protection are disabled.

Comment 8

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

I just came across this bug again and I'm not sure if I understand the new summary correctly.

If the user disables *any* protection, the shield should show the red strikethrough (with the shield still being gray) both in the URL bar and in the doorhanger.

Comment 9

[Alex Bardas :alexbardas]

(In reply to Philipp Sackl [:phlsa] from comment #8)
> I just came across this bug again and I'm not sure if I understand the new
> summary correctly.
> 
> If the user disables *any* protection, the shield should show the red
> strikethrough (with the shield still being gray) both in the URL bar and in
> the doorhanger.

Thank you Philip for clarifying this with the word *any*, because the title is misleading.

Comment 10

[[:mmc] Monica Chew (no longer reading bugmail)]

I think that depends on if you want the large icon synced to the small one, or to the doorhanger title.

Blocking anything, no type of protection is disabled
Title: Nightly is blocking content on this page
Icon: Regular shield
Large: Regular shield

Blocking something, one type of protection is disabled but the other is active
Title: Nightly is blocking content on this page
Icon: Strike-through
Large: ?

Both types of protection are disabled (or one is disabled and the other is irrelevant)
Title: Nightly is not blocking any content on this page
Icon: Strike-through
Large: Strike-through

Comment 11

[Alex Bardas :alexbardas]

Attachment: bug1043805_blocked_content_doorhanger_should_show_a_large_red_shield_when_protection_is_disabled.diff

This patch should be applied on top of the patch from bug 1063115, otherwise there may be conflicts. I've tested it on Mac / Linux / Windows.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8488111 [details] [diff] [review]
bug1043805_blocked_content_doorhanger_should_show_a_large_red_shield_when_protection_is_disabled.diff

>--- a/browser/base/content/urlbarBindings.xml
>+++ b/browser/base/content/urlbarBindings.xml
>@@ -1673,16 +1673,19 @@
>                              tooltiptext="&closeNotification.tooltip;"/>
>         </xul:vbox>
>       </xul:hbox>
>     </content>
>     <resources>
>       <stylesheet src="chrome://global/skin/notification.css"/>
>     </resources>
>     <implementation>
>+      <field name="_doorhanger">
>+        document.getElementById("bad-content-notification");
>+      </field>

How is '_doorhanger' going to be different from 'this'?

Comment 13

[Alex Bardas :alexbardas]

Attachment: Blocked content doorhanger should show a large red shield when protection is disabled

As before, it should be applied on top of the other patch.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8488271 [details] [diff] [review]
Blocked content doorhanger should show a large red shield when protection is disabled

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

r+ on that assumption that Philipp confirms this is indeed what we want.

Comment 15

[Alex Bardas :alexbardas]

Here is the try run for this:

https://tbpl.mozilla.org/?tree=Try&rev=4eb1da13354d

Philipp, you can test it on a mac using this version: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4eb1da13354d/try-macosx64/firefox-35.0a1.en-US.mac.dmg

Comment 16

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Yes, let's go with this!
Sorry for the back and forth here, it's not a clear decision.

Comment 17

[Alex Bardas :alexbardas]

Attachment: Add tests for checking mixedblockdisabled and trackingblockdisabled attributes

This is an addition to this and other patches from tracking protection part, checking for some element attributes.

Comment 18

[Drew Willcoxon :adw]

Comment on attachment 8490877 [details] [diff] [review]
Add tests for checking mixedblockdisabled and trackingblockdisabled attributes

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

Please run this by tryserver before adding checkin-needed.

::: browser/base/content/test/general/browser_mixedcontent_securityflags.js
@@ +66,5 @@
> +  notification.reshow();
> +
> +  // Make sure the notification has the mixedblockdisabled attribute set to true
> +  is(PopupNotifications.panel.firstChild.getAttribute("mixedblockdisabled"), "true",
> +    "Doorhanger must have [mixedblockdisabled='true'] attribute");

I think you need to call notification.remove at this point?  Like how the previous function calls it after calling notification.reshow.

Comment 19

[Alex Bardas :alexbardas]

(In reply to Drew Willcoxon :adw from comment #18)
> Comment on attachment 8490877 [details] [diff] [review]
> Add tests for checking mixedblockdisabled and trackingblockdisabled
> attributes
> 
> Review of attachment 8490877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please run this by tryserver before adding checkin-needed.
> 
> I think you need to call notification.remove at this point?  Like how the
> previous function calls it after calling notification.reshow.

Try looks good:

https://tbpl.mozilla.org/?tree=Try&rev=f276b9261ad5

Comment 20

[Alex Bardas :alexbardas]

There are 2 patches attached. Both need to be checked in.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/012c062c9247
https://hg.mozilla.org/integration/fx-team/rev/516ea3a53081

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/012c062c9247
https://hg.mozilla.org/mozilla-central/rev/516ea3a53081

Comment 23

[Catalin Varga [QA][:VarCat]]

Verified as fixed using:

FF 35 Aurora
Build Id:20141022004003
OS: Win 7 x64, Mac Os X 10.9.5, Ubuntu 14.04 x64