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

VERIFIED FIXED in Firefox 35

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Unfocused, Assigned: alexbardas)

Tracking

30 Branch
Firefox 35
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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]
Flags: firefox-backlog+
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.
Flags: needinfo?(gkontaxis)
Summary: Blocked content doorhanger should show a red warning a type of protection is disabled → Blocked content doorhanger should show a red warning when a type of protection is disabled
Philipp, could we get image(s) for the shield icon struck through with a red line in this mockup: attachment 8459851 [details]?
Flags: needinfo?(philipp)

Comment 3

5 years ago
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.
Flags: needinfo?(gkontaxis)
(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".
Flags: needinfo?(philipp)
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Assignee: nobody → georgios.kontaxis
Target Milestone: --- → Firefox 34
Posted file large-shield.zip
Here are the large shield icons.
Sorry that took so long.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Blocked content doorhanger should show a red warning when a type of protection is disabled → Blocked content doorhanger should show a large red shield when both types of protection are disabled
Assignee: georgios.kontaxis → nobody
Assignee: nobody → mmc

Updated

5 years ago
Flags: qe-verify+
Assignee

Updated

5 years ago
Assignee: mmc → abardas
Status: REOPENED → ASSIGNED
Iteration: --- → 35.1
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.
Assignee

Comment 9

5 years ago
(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.
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
Flags: needinfo?(philipp)
Assignee

Comment 11

5 years ago
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.
Attachment #8488111 - Flags: review?(bmcbride)
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'?
Assignee

Comment 13

5 years ago
As before, it should be applied on top of the other patch.
Attachment #8488111 - Attachment is obsolete: true
Attachment #8488111 - Flags: review?(bmcbride)
Attachment #8488271 - Flags: review?(bmcbride)

Updated

5 years ago
QA Contact: mwobensmith
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.
Attachment #8488271 - Flags: review?(bmcbride) → review+
Yes, let's go with this!
Sorry for the back and forth here, it's not a clear decision.
Flags: needinfo?(philipp)
Iteration: 35.1 → 35.2
Assignee

Comment 17

5 years ago
This is an addition to this and other patches from tracking protection part, checking for some element attributes.
Attachment #8490877 - Flags: review?(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.
Attachment #8490877 - Flags: review?(adw) → review+
Assignee

Comment 19

5 years ago
(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
Assignee

Comment 20

5 years ago
There are 2 patches attached. Both need to be checked in.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/012c062c9247
https://hg.mozilla.org/mozilla-central/rev/516ea3a53081
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: Firefox 34 → Firefox 35
Verified as fixed using:

FF 35 Aurora
Build Id:20141022004003
OS: Win 7 x64, Mac Os X 10.9.5, Ubuntu 14.04 x64
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.