Make the mixed content doorhanger more generic for all blocked content types

VERIFIED FIXED in Firefox 34

Status

()

Firefox
General
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Unfocused, Assigned: Georgios Kontaxis)

Tracking

({user-doc-needed})

30 Branch
Firefox 34
user-doc-needed
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [user doc: see comment 19 and 21])

Attachments

(2 attachments, 20 obsolete attachments)

98.13 KB, image/png
Details
61.99 KB, patch
mmc
: review+
Details | Diff | Splinter Review
Currently, the blocked content doorhanger is specific to just mixed content blocking. We want to add more types of blocked content, so this doorhanger should be made more generic to allow showing of information and controls for other types of content blocking.

General structure should be a heading section describing that content was blocked, followed by one or more other sections for each type of blocked content.

This should be able to follow how the WebRTC notification doorhangers work.

Mockup: attachment 8459851 [details]
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Blocks: 1043801
(Assignee)

Comment 1

3 years ago
Created attachment 8463563 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Assignee: nobody → gkontaxis
Status: NEW → ASSIGNED
Comment on attachment 8463563 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content

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

This patch just changes the mixed content implementation to use an extended version of PopupNotifications.jsm that we can customize by adding to the xul, right?

::: browser/base/content/browser.js
@@ +6612,5 @@
> +    };
> +
> +    PopupNotifications.show(gBrowser.selectedBrowser, "bad-content",
> +                            "", "mixed-content-blocked-notification-icon",
> +                            null, null, options);

We do not need to worry about the icon being hidden after an action is triggered since that only applies to the primary/secondary actions which are set here to null.

::: browser/base/content/urlbarBindings.xml
@@ +1589,5 @@
> +            <xul:description class="popup-notification-item-title" xbl:inherits="popupid">
> +              Insecure content
> +            </xul:description>
> +            <xul:description style="width: 200px">
> +              Parts of this web page have been transmitted over an unencrypted connection and have been blocked.

All strings must point to labels so they can be localized.

@@ +1607,5 @@
> +    </resources>
> +    <implementation>
> +      <constructor><![CDATA[
> +        // nsIWebProgressListener
> +        const STATE_BLOCKED_MIXED_ACTIVE_CONTENT  = 0x00000010;

Can this reference nsIWebProgressListener.STATE_BLOCKED_foo?
(Assignee)

Comment 3

3 years ago
Created attachment 8464132 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8463563 - Attachment is obsolete: true
(Assignee)

Comment 4

3 years ago
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #2)
> Comment on attachment 8463563 [details] [diff] [review]
> extended popup notifications to create a generic doorhanger for all security
> notifications incl. mixed content
> 
> Review of attachment 8463563 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch just changes the mixed content implementation to use an extended
> version of PopupNotifications.jsm that we can customize by adding to the
> xul, right?
> 
> ::: browser/base/content/browser.js
> @@ +6612,5 @@
> > +    };
> > +
> > +    PopupNotifications.show(gBrowser.selectedBrowser, "bad-content",
> > +                            "", "mixed-content-blocked-notification-icon",
> > +                            null, null, options);
> 
> We do not need to worry about the icon being hidden after an action is
> triggered since that only applies to the primary/secondary actions which are
> set here to null.
> 
> ::: browser/base/content/urlbarBindings.xml
> @@ +1589,5 @@
> > +            <xul:description class="popup-notification-item-title" xbl:inherits="popupid">
> > +              Insecure content
> > +            </xul:description>
> > +            <xul:description style="width: 200px">
> > +              Parts of this web page have been transmitted over an unencrypted connection and have been blocked.
> 
> All strings must point to labels so they can be localized.
> 
Fixed

> @@ +1607,5 @@
> > +    </resources>
> > +    <implementation>
> > +      <constructor><![CDATA[
> > +        // nsIWebProgressListener
> > +        const STATE_BLOCKED_MIXED_ACTIVE_CONTENT  = 0x00000010;
> 
> Can this reference nsIWebProgressListener.STATE_BLOCKED_foo?

Fixed
(Assignee)

Comment 5

3 years ago
Created attachment 8464174 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8464132 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 1043803
(Assignee)

Comment 6

3 years ago
Created attachment 8465555 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8464174 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=e552178f4c34
(Assignee)

Comment 8

3 years ago
Created attachment 8465696 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8465555 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8465696 - Flags: feedback?(bmcbride)
(Assignee)

Comment 9

3 years ago
Created attachment 8465697 [details]
Screen Shot 2014-07-31 at 12.38.38 PM.png
(Assignee)

Comment 10

3 years ago
Created attachment 8465869 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Comment 11

3 years ago
Created attachment 8465870 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8465869 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=4607979dd266
Comment on attachment 8465696 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content

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

Looking good :)

* Should remove the old <popupnotification> from popupnotifications.inc
* CSS for Windows/Linux (good to get this sorted quickly, as I use Windows as my main OS)
* For making it pretty, refer to Chameleon for colors: http://people.mozilla.org/~jgruen/chameleon/

::: browser/base/content/test/general/browser_bug822367.js
@@ +164,5 @@
>    gTestBrowser.contentWindow.location = url;
>  }
>  function MixedTest6A() {
>    gTestBrowser.removeEventListener("load", MixedTest6A, true);
> +  waitForCondition(function() PopupNotifications.getNotification("bad-content", gTestBrowser), MixedTest6B, "waited to long for doorhanger");

While you're changing this line, want to fix the typo?

::: browser/base/content/urlbarBindings.xml
@@ +1594,5 @@
> +            </xul:description>
> +          </xul:vbox>
> +          <xul:toolbarbutton type="menu" label="Options" class="popup-notification-item-button" xbl:inherits="popupid">
> +            <xul:menupopup>
> +              <xul:menuitem anonid="mixedContentAction.unblock" hidden="true" label="&mixedContentBlocked.unblock.label;" oncommand="disableMixedContentProtection();"/>

Use:
  document.getBindingParent(this).disableMixedContentProtection();

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +717,5 @@
>  
>  <!ENTITY loopCallButton.tooltip "Invite someone to talk">
>  
> +<!-- Bad Content Blocker Doorhanger Notification -->
> +<!ENTITY badContentBlocked.message "Firefox is blocking content on this page.">

Hmm, this wording will look a bit weird/misleading if the person has disabled all protection options in this notification. Should ask Philipp about this.

Also, we can't include "Firefox" in strings, as it's a brand name that changes. eg, "Firefox" on release versions, "Nightly" on nightly builds, etc. Here, you'd use &brandShortName;
See the following for other brand strings:
  http://dxr.mozilla.org/mozilla-central/source/browser/branding/official/locales/en-US/brand.dtd
  http://dxr.mozilla.org/mozilla-central/source/browser/branding/official/locales/en-US/brand.properties

@@ +721,5 @@
> +<!ENTITY badContentBlocked.message "Firefox is blocking content on this page.">
> +<!ENTITY badContentBlocked.moreinfo "Most websites will work properly even if content is blocked.">
> +
> +<!ENTITY mixedContentBlocked.message "Insecure content">
> +<!ENTITY mixedContentBlocked.moreinfo "Parts of this web page have been transmitted over an unencrypted connection and have been blocked.">

It's not safe to re-use a string ID like this (ie, changing the contents, but not the ID), as it's too easy for localizers to miss and not re-translate it.

General rule is: If you change the contents of a string or the context in which the string is used, you need to use a new ID.

@@ +722,5 @@
> +<!ENTITY badContentBlocked.moreinfo "Most websites will work properly even if content is blocked.">
> +
> +<!ENTITY mixedContentBlocked.message "Insecure content">
> +<!ENTITY mixedContentBlocked.moreinfo "Parts of this web page have been transmitted over an unencrypted connection and have been blocked.">
> +<!ENTITY mixedContentBlocked.unblock.label "Disable protection for now">

This should have an accesskey.
Attachment #8465696 - Flags: feedback?(bmcbride) → feedback+
(Assignee)

Comment 14

3 years ago
(In reply to Blair McBride [:Unfocused] from comment #13)
> Comment on attachment 8465696 [details] [diff] [review]
> extended popup notifications to create a generic doorhanger for all security
> notifications incl. mixed content
> 
> Review of attachment 8465696 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good :)
> 
> * Should remove the old <popupnotification> from popupnotifications.inc
DONE

> * CSS for Windows/Linux (good to get this sorted quickly, as I use Windows
> as my main OS)
DONE

> * For making it pretty, refer to Chameleon for colors:
> http://people.mozilla.org/~jgruen/chameleon/
> 
> ::: browser/base/content/test/general/browser_bug822367.js
> @@ +164,5 @@
> >    gTestBrowser.contentWindow.location = url;
> >  }
> >  function MixedTest6A() {
> >    gTestBrowser.removeEventListener("load", MixedTest6A, true);
> > +  waitForCondition(function() PopupNotifications.getNotification("bad-content", gTestBrowser), MixedTest6B, "waited to long for doorhanger");
> 
> While you're changing this line, want to fix the typo?
> 
OK

> ::: browser/base/content/urlbarBindings.xml
> @@ +1594,5 @@
> > +            </xul:description>
> > +          </xul:vbox>
> > +          <xul:toolbarbutton type="menu" label="Options" class="popup-notification-item-button" xbl:inherits="popupid">
> > +            <xul:menupopup>
> > +              <xul:menuitem anonid="mixedContentAction.unblock" hidden="true" label="&mixedContentBlocked.unblock.label;" oncommand="disableMixedContentProtection();"/>
> 
> Use:
>   document.getBindingParent(this).disableMixedContentProtection();
> 
OK

> ::: browser/locales/en-US/chrome/browser/browser.dtd
> @@ +717,5 @@
> >  
> >  <!ENTITY loopCallButton.tooltip "Invite someone to talk">
> >  
> > +<!-- Bad Content Blocker Doorhanger Notification -->
> > +<!ENTITY badContentBlocked.message "Firefox is blocking content on this page.">
> 
> Hmm, this wording will look a bit weird/misleading if the person has
> disabled all protection options in this notification. Should ask Philipp
> about this.
> 
Yes you are right.

> Also, we can't include "Firefox" in strings, as it's a brand name that
> changes. eg, "Firefox" on release versions, "Nightly" on nightly builds,
> etc. Here, you'd use &brandShortName;
> See the following for other brand strings:
>  
> http://dxr.mozilla.org/mozilla-central/source/browser/branding/official/
> locales/en-US/brand.dtd
>  
> http://dxr.mozilla.org/mozilla-central/source/browser/branding/official/
> locales/en-US/brand.properties
> 
My issue was that including &brandShortName; failed to yield a well-defined entity. The fix was including brand.dtd in urlbarBindings.xml - Is that alright?

> @@ +721,5 @@
> > +<!ENTITY badContentBlocked.message "Firefox is blocking content on this page.">
> > +<!ENTITY badContentBlocked.moreinfo "Most websites will work properly even if content is blocked.">
> > +
> > +<!ENTITY mixedContentBlocked.message "Insecure content">
> > +<!ENTITY mixedContentBlocked.moreinfo "Parts of this web page have been transmitted over an unencrypted connection and have been blocked.">
> 
> It's not safe to re-use a string ID like this (ie, changing the contents,
> but not the ID), as it's too easy for localizers to miss and not
> re-translate it.
> 
> General rule is: If you change the contents of a string or the context in
> which the string is used, you need to use a new ID.
> 
> @@ +722,5 @@
> > +<!ENTITY badContentBlocked.moreinfo "Most websites will work properly even if content is blocked.">
> > +
> > +<!ENTITY mixedContentBlocked.message "Insecure content">
> > +<!ENTITY mixedContentBlocked.moreinfo "Parts of this web page have been transmitted over an unencrypted connection and have been blocked.">
> > +<!ENTITY mixedContentBlocked.unblock.label "Disable protection for now">
> 
> This should have an accesskey.
(Assignee)

Comment 15

3 years ago
Created attachment 8467462 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8465696 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8465870 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=7c5ae91f8d94
(Assignee)

Comment 17

3 years ago
Created attachment 8468105 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8467462 - Attachment is obsolete: true
(Assignee)

Comment 18

3 years ago
Comment on attachment 8468105 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content

Addressed review comments. Visual style remained the same.

https://tbpl.mozilla.org/?tree=Try&rev=c4baca23f717
Attachment #8468105 - Flags: review?(bmcbride)
Please use this as the Learn More link (https://bugzilla.mozilla.org/show_bug.cgi?id=1047707)

https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/tracking-protection-firefox
(Assignee)

Comment 20

3 years ago
Created attachment 8469345 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Comment 21

3 years ago
Done. For "mixed-content" since this is the mixed content bug :) However we do need to update the help page, right now instructions and screenshots do not apply. https://support.mozilla.org/en-US/kb/how-does-content-isnt-secure-affect-my-safety
(Assignee)

Comment 22

3 years ago
Created attachment 8469347 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8469345 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
Created attachment 8469353 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8469347 - Attachment is obsolete: true
Attachment #8468105 - Attachment is obsolete: true
Attachment #8468105 - Flags: review?(bmcbride)
Attachment #8469353 - Flags: review?(bmcbride)
Comment on attachment 8469353 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content

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

Few things to do to get this in line with the mockup (attachment 8459851 [details]):
* Missing shield icon - use mixed-content-blocked-64.png and mixed-content-blocked-64@2x.png (only OSX has the later for HiDPI, so just move them all into browser/themes/shared/)
* Missing section separators
* Mixed content section shouldn't be indented relative to the top header section (but both should be indented to accommodate the icon)
* "Options" dropdown should be a <xul:menulist>, which will give it the right style.
* Also, it should be aligned to the top  (see comment below)

::: browser/base/content/urlbarBindings.xml
@@ +1585,5 @@
> +            &badContentBlocked.moreinfo;
> +          </xul:description>
> +        </xul:vbox>
> +        <!-- mixed content -->
> +        <xul:hbox anonid="mixedContent" hidden="true" class="popup-notification-item" xbl:inherits="popupid">

If you add align="top" to this, the "Options" dropdown will no longer stretch to take up all vertical space.

@@ +1595,5 @@
> +              &mixedContentBlocked2.moreinfo;
> +            </xul:description>
> +            <xul:label anonid="mixedContent.helplink" class="text-link" href="" value="&mixedContentBlocked2.learnMore;"/>
> +          </xul:vbox>
> +          <xul:toolbarbutton type="menu" label="Options" class="popup-notification-item-button" xbl:inherits="popupid">

Forgot to localize this label.

@@ +1597,5 @@
> +            <xul:label anonid="mixedContent.helplink" class="text-link" href="" value="&mixedContentBlocked2.learnMore;"/>
> +          </xul:vbox>
> +          <xul:toolbarbutton type="menu" label="Options" class="popup-notification-item-button" xbl:inherits="popupid">
> +            <xul:menupopup>
> +              <xul:menuitem anonid="mixedContentAction.unblock" hidden="true" label="&mixedContentBlocked2.unblock.label;" accesskey="&mixedContentBlocked2.unblock.accesskey;" oncommand="document.getBindingParent(this).disableMixedContentProtection();"/>

Wrap this line to ~80 characters per line?

@@ +1601,5 @@
> +              <xul:menuitem anonid="mixedContentAction.unblock" hidden="true" label="&mixedContentBlocked2.unblock.label;" accesskey="&mixedContentBlocked2.unblock.accesskey;" oncommand="document.getBindingParent(this).disableMixedContentProtection();"/>
> +            </xul:menupopup>
> +          </xul:toolbarbutton>
> +        </xul:hbox>
> +        <!-- -->

Unhelpful comment is unhelpful.

::: browser/themes/osx/browser.css
@@ +4125,5 @@
>  %include ../shared/devtools/responsivedesign.inc.css
>  %include ../shared/devtools/highlighter.inc.css
>  %include ../shared/devtools/commandline.inc.css
>  %include ../shared/plugin-doorhanger.inc.css
> +%include ../shared/badcontent-doorhanger.inc.css

Forgot to add this to the Windows and Linux themes.

::: browser/themes/shared/badcontent-doorhanger.inc.css
@@ +1,1 @@
> +.popup-notification-main-box[popupid="bad-content"] {

Remove any empty rules.
Attachment #8469353 - Flags: review?(bmcbride) → review-
Hey Philipp, from Blair's review in comment 13, should we have a badContentNotBlocked (or badContentLoaded) message that says "Firefox is not blocking content on this page" if both types of protection are disabled?

>  
>  <!ENTITY loopCallButton.tooltip "Invite someone to talk">
>  
> +<!-- Bad Content Blocker Doorhanger Notification -->
> +<!ENTITY badContentBlocked.message "Firefox is blocking content on this page.">


Hmm, this wording will look a bit weird/misleading if the person has disabled all protection options in this notification. Should ask Philipp about this.
Flags: needinfo?(philipp)
Yes changing the title dynamically is a good idea.

So we have two cases for the title:
1. »Firefox is blocking content on this page« (if protection is fully or partially active)
2. »Firefox is not blocking content on this page« (if all available protections are off)
Flags: needinfo?(philipp)
(Assignee)

Updated

3 years ago
Blocks: 1045809
Blocks: 1029886
(Assignee)

Comment 27

3 years ago
Created attachment 8473260 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8469353 - Attachment is obsolete: true
(Assignee)

Comment 28

3 years ago
Created attachment 8473395 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8473260 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
Created attachment 8473396 [details]
Screen Shot 2014-08-14 at 5.45.13 PM.png
(Assignee)

Updated

3 years ago
Attachment #8465697 - Attachment is obsolete: true
(Assignee)

Comment 30

3 years ago
Comment on attachment 8473395 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content

Addressed review comments. Did not move png files.
Attachment #8473395 - Flags: review?(bmcbride)
(Assignee)

Updated

3 years ago
Attachment #8473395 - Flags: review?(bmcbride)
(Assignee)

Comment 31

3 years ago
Created attachment 8473418 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8473395 - Attachment is obsolete: true
(Assignee)

Comment 32

3 years ago
Created attachment 8473419 [details]
Screen Shot 2014-08-14 at 6.58.34 PM.png
Attachment #8473396 - Attachment is obsolete: true
(Assignee)

Comment 33

3 years ago
Comment on attachment 8473418 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content

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

Addressed review comments. Did not move png files. Used a button (followed by a menupopup) instead of a menulist. It looks closer to the mockup and seems more suitable to what I'm trying to do with selectively hiding/showing menuitems based on the doorhanger state.
Attachment #8473418 - Flags: review?(bmcbride)
(Assignee)

Comment 34

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=baac82ccbcc9
(Assignee)

Updated

3 years ago
Attachment #8473418 - Flags: review?(bmcbride)
(Assignee)

Comment 35

3 years ago
Created attachment 8475327 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8473418 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8475327 - Flags: review?(adw)
Comment on attachment 8475327 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content

Moving over to Gijs to help rebalance reviews.
Attachment #8475327 - Flags: review?(adw) → review?(gijskruitbosch+bugs)

Comment 37

3 years ago
Comment on attachment 8475327 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content

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

For future reference: patches of this size become easier to review if you split them up into logical parts. Here, for example, the renaming of the notification led to lots of mind-numbing changes of all the references to the ID.

Both git and hg have tools for this; in hg the easiest that I know of when using mercurial queues is:

hg qref -X . # which empties the patch
hg qref --interactive # which lets you decide which hunks stay in
hg qnew # to stick the rest of the changes in a new patch (or hg qrecord if you need more than 2 patches in total)

(this needs the `record` hg extension to work)

I need to look at this in more detail tomorrow when I'm more awake, but here's some initial feedback anyway.

Generally, this looks sane. I just need to run it through its paces and poke at it some more when it's properly Thursday here, so I'm leaving the r? for now. If you have time to update this before I finish sleeping, go for it (an updated try patch would also be appreciated :-) ).

Also, can you explain why you need to call .reshow() in all the tests? It's not entirely clear to me. Is this just trying to ensure test failures don't cascade/break other stuff? Or working around XBL binding stuff not working if the notification has no frame? Or something else?

::: browser/base/content/browser.js
@@ +6630,4 @@
>      };
> +
> +    PopupNotifications.show(gBrowser.selectedBrowser, "bad-content",
> +                            "", "mixed-content-blocked-notification-icon",

Can we update the icon id, too, while we're at it? Seems odd to make a more generic popup and anchor it to the same specific anchor, when the icon we're using inside the notification doesn't change for different types of bad content - so presumably, neither will the outside icon.

::: browser/base/content/test/general/browser_bug902156.js
@@ +48,5 @@
>    // Removing EventListener because we have to register a new
>    // one once the page is loaded with mixed content blocker disabled
>    gTestBrowser.removeEventListener("load", test1A, true);
>    gTestBrowser.addEventListener("load", test1B, true);
>    

Nit: nuke this trailing whitespace while we're here.

@@ +105,5 @@
>    // Removing EventListener because we have to register a new
>    // one once the page is loaded with mixed content blocker disabled
>    gTestBrowser.removeEventListener("load", test2A, true);
>    gTestBrowser.addEventListener("load", test2B, true);
>    

And this one.

@@ +162,5 @@
>  function test3A() {
>    // Removing EventListener because we have to register a new
>    // one once the page is loaded with mixed content blocker disabled
>    gTestBrowser.removeEventListener("load", test3A, true);
>    

And this one

::: browser/base/content/urlbarBindings.xml
@@ +1606,5 @@
> +                  class="text-link plain" href=""
> +                  value="&mixedContentBlocked2.learnMore;"/>
> +              </xul:vbox>
> +              <xul:button
> +                type="menu" label="&mixedContentBlocked2.options;"

Hrm. Ideally, this should also have an access key. However, if we're going to add multiple sections, and each has an identical button that's just labelled "Options" (sigh...) that makes less sense. So whether I'd want it to have an access key probably depends on whether we're adding real new sections for 34 still, next cycle, or "sometime Q4" or whatever. Let's leave it for now.
(Assignee)

Comment 38

3 years ago
Created attachment 8476393 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Comment 39

3 years ago
(In reply to :Gijs Kruitbosch from comment #37)
> Comment on attachment 8475327 [details] [diff] [review]
> extended popup notifications to create a generic doorhanger for all security
> notifications incl. mixed content
> 
> Review of attachment 8475327 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For future reference: patches of this size become easier to review if you
> split them up into logical parts. Here, for example, the renaming of the
> notification led to lots of mind-numbing changes of all the references to
> the ID.
> 
> Both git and hg have tools for this; in hg the easiest that I know of when
> using mercurial queues is:
> 
> hg qref -X . # which empties the patch
> hg qref --interactive # which lets you decide which hunks stay in
> hg qnew # to stick the rest of the changes in a new patch (or hg qrecord if
> you need more than 2 patches in total)
> 
> (this needs the `record` hg extension to work)
> 
> I need to look at this in more detail tomorrow when I'm more awake, but
> here's some initial feedback anyway.
> 
> Generally, this looks sane. I just need to run it through its paces and poke
> at it some more when it's properly Thursday here, so I'm leaving the r? for
> now. If you have time to update this before I finish sleeping, go for it (an
> updated try patch would also be appreciated :-) ).
> 
> Also, can you explain why you need to call .reshow() in all the tests? It's
> not entirely clear to me. Is this just trying to ensure test failures don't
> cascade/break other stuff? Or working around XBL binding stuff not working
> if the notification has no frame? Or something else?
> 
I couldn't get the notification panel if the doorhanger wasn't deployed (which is not by default) so I forced it to show.

> ::: browser/base/content/browser.js
> @@ +6630,4 @@
> >      };
> > +
> > +    PopupNotifications.show(gBrowser.selectedBrowser, "bad-content",
> > +                            "", "mixed-content-blocked-notification-icon",
> 
> Can we update the icon id, too, while we're at it? Seems odd to make a more
> generic popup and anchor it to the same specific anchor, when the icon we're
> using inside the notification doesn't change for different types of bad
> content - so presumably, neither will the outside icon.
> 
> ::: browser/base/content/test/general/browser_bug902156.js
> @@ +48,5 @@
> >    // Removing EventListener because we have to register a new
> >    // one once the page is loaded with mixed content blocker disabled
> >    gTestBrowser.removeEventListener("load", test1A, true);
> >    gTestBrowser.addEventListener("load", test1B, true);
> >    
> 
> Nit: nuke this trailing whitespace while we're here.
> 
> @@ +105,5 @@
> >    // Removing EventListener because we have to register a new
> >    // one once the page is loaded with mixed content blocker disabled
> >    gTestBrowser.removeEventListener("load", test2A, true);
> >    gTestBrowser.addEventListener("load", test2B, true);
> >    
> 
> And this one.
> 
> @@ +162,5 @@
> >  function test3A() {
> >    // Removing EventListener because we have to register a new
> >    // one once the page is loaded with mixed content blocker disabled
> >    gTestBrowser.removeEventListener("load", test3A, true);
> >    
> 
> And this one
> 
> ::: browser/base/content/urlbarBindings.xml
> @@ +1606,5 @@
> > +                  class="text-link plain" href=""
> > +                  value="&mixedContentBlocked2.learnMore;"/>
> > +              </xul:vbox>
> > +              <xul:button
> > +                type="menu" label="&mixedContentBlocked2.options;"
> 
> Hrm. Ideally, this should also have an access key. However, if we're going
> to add multiple sections, and each has an identical button that's just
> labelled "Options" (sigh...) that makes less sense. So whether I'd want it
> to have an access key probably depends on whether we're adding real new
> sections for 34 still, next cycle, or "sometime Q4" or whatever. Let's leave
> it for now.

This is where this is going. Immediately after this bug we have bug 1043801 and if you look at the mockup (https://bug1041748.bugzilla.mozilla.org/attachment.cgi?id=8459851) there will be identical buttons (not all of them visible all time). I have strong objections to the existing access key (bug 1043803	adds an equivalent block access key) since we are going to have more and more buttons with similar block/unblock actions. We'll also have a hard time coming up with unique (and meaningful) keys for all of them.
(Assignee)

Comment 40

3 years ago
Comment on attachment 8476393 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content

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

This addresses the comments of the previous review. https://tbpl.mozilla.org/?tree=Try&rev=fdf9514ed526
Attachment #8476393 - Flags: review?(adw)
(Assignee)

Updated

3 years ago
Attachment #8476393 - Flags: review?(adw)
(Assignee)

Comment 41

3 years ago
Comment on attachment 8476393 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content

This addresses the comments of the previous review. https://tbpl.mozilla.org/?tree=Try&rev=fdf9514ed526
Attachment #8476393 - Flags: review?(gijskruitbosch+bugs)

Updated

3 years ago
Attachment #8475327 - Attachment is obsolete: true
Attachment #8475327 - Flags: review?(gijskruitbosch+bugs)

Updated

3 years ago
Keywords: user-doc-needed
Whiteboard: [user doc: see comment 19 and 21]

Comment 42

3 years ago
Comment on attachment 8476393 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content

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

Did you use "hg move" to move the images, so as to preserve history? I don't know if hg adequately tells me whether that was the case... but that should be done :-)

With that addressed, ship it!

Regarding the accesskey in the dropdown menu for the "Disable... for now" should be fine; once the menu is open, pressing the accesskey without alt should activate the menu item, and because only one menu can be open at a time, there should be no ambiguity. It kind of sucks that we don't get to have accesskeys for all the "Options" dropdown buttons, but that's life - there's not much else in the dialog, so tabbing through it shouldn't be too terrible.


Finally, when looking at the spec, I noticed that the doorhanger closes if you disable mixed-content protection, and is replaced with a different (collapsed) one with a warning triangle icon. I presume this is tracked in bug 1045809?

::: browser/themes/linux/jar.mn
@@ +51,5 @@
>    skin/classic/browser/menuPanel-exit.png
>    skin/classic/browser/menuPanel-help.png
>    skin/classic/browser/menuPanel-small.png
> +  skin/classic/browser/bad-content-blocked-16.png
> +  skin/classic/browser/bad-content-blocked-64.png

This has bitrotted, but should be trivial to fix.
Attachment #8476393 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 43

3 years ago
(In reply to :Gijs Kruitbosch from comment #42)
> Comment on attachment 8476393 [details] [diff] [review]
> extended popup notifications to create a generic doorhanger for all security
> notifications incl. mixed content
> 
> Review of attachment 8476393 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did you use "hg move" to move the images, so as to preserve history? I don't
> know if hg adequately tells me whether that was the case... but that should
> be done :-)
> 
> With that addressed, ship it!
> 
> Regarding the accesskey in the dropdown menu for the "Disable... for now"
> should be fine; once the menu is open, pressing the accesskey without alt
> should activate the menu item, and because only one menu can be open at a
> time, there should be no ambiguity. It kind of sucks that we don't get to
> have accesskeys for all the "Options" dropdown buttons, but that's life -
> there's not much else in the dialog, so tabbing through it shouldn't be too
> terrible.
> 
> 
> Finally, when looking at the spec, I noticed that the doorhanger closes if
> you disable mixed-content protection, and is replaced with a different
> (collapsed) one with a warning triangle icon. I presume this is tracked in
> bug 1045809?
> 
Actually bug 1043803

> ::: browser/themes/linux/jar.mn
> @@ +51,5 @@
> >    skin/classic/browser/menuPanel-exit.png
> >    skin/classic/browser/menuPanel-help.png
> >    skin/classic/browser/menuPanel-small.png
> > +  skin/classic/browser/bad-content-blocked-16.png
> > +  skin/classic/browser/bad-content-blocked-64.png
> 
> This has bitrotted, but should be trivial to fix.
(Assignee)

Comment 44

3 years ago
Created attachment 8476774 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8476393 - Attachment is obsolete: true
(Assignee)

Comment 45

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=de3e43963358
Comment on attachment 8476774 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content

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

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +719,5 @@
> +<!-- Bad Content Blocker Doorhanger Notification -->
> +<!ENTITY badContentBlocked.moreinfo "Most websites will work properly even if content is blocked.">
> +
> +<!ENTITY mixedContentBlocked2.message "Insecure content">
> +<!ENTITY mixedContentBlocked2.moreinfo "Parts of this web page have been transmitted over an unencrypted connection and have been blocked.">

Per irc and https://bugzilla.mozilla.org/show_bug.cgi?id=1041748#c6, please change to 

Some unencrypted elements on this website have been blocked for your protection.
Correction per irc, "Some unencrypted elements on this website have been blocked."
(Assignee)

Comment 48

3 years ago
Created attachment 8476993 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
(Assignee)

Updated

3 years ago
Attachment #8476774 - Attachment is obsolete: true
(Assignee)

Comment 49

3 years ago
(In reply to gkontaxis from comment #48)
> Created attachment 8476993 [details] [diff] [review]
> extended popup notifications to create a generic doorhanger for all security
> notifications incl. mixed content

Corrected
(Assignee)

Comment 50

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=67acccd4bb40
Comment on attachment 8476993 [details] [diff] [review]
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content

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

Carrying over gijs's review in comment 42.
Attachment #8476993 - Flags: review+
Blocks: 1057643
(Assignee)

Comment 52

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=e443fc2023ef
https://hg.mozilla.org/integration/fx-team/rev/456567bc1a2b
(In reply to Gijs Kruitbosch from comment #42)
> Did you use "hg move" to move the images, so as to preserve history? I don't
> know if hg adequately tells me whether that was the case... but that should
> be done :-)

They show in hg diff -g but I think there's a bug on it not showing up in Splinter.
https://hg.mozilla.org/mozilla-central/rev/456567bc1a2b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34

Updated

3 years ago
Iteration: --- → 34.3
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: catalin.varga
Verified as fixed using the following environment:

FF 34
Build Id: 	20140831030206
OS: Win 7 x64, Ubuntu 13.04 x64, Mac Os X 10.9.4

I've noticed:
1. That when Disabling protection for insecure content "Some unencrypted elements on this website have been blocked." entry is not grey out.

Should I log a new bug for this? or this part was not implemented yet?
Flags: needinfo?(georgios.kontaxis)
I see that, too, which doesn't match the mockup in comment 0.  I don't see an open bug about it in the dependency tree for bug 1029886, so could you please file a new bug?  I'll leave the needinfo for Georgios in case he's able to respond.
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1063115
As a new bug was logged for the only issue found during the verification with FF 34(BuildId:20140831030206), I'm marking this bug as verified.
Status: RESOLVED → VERIFIED
Flags: needinfo?(georgios.kontaxis)
You need to log in before you can comment on or make changes to this bug.