Closed Bug 1043797 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Version:
30 Branch
Type:
defect
Priority:
Not set
Severity:
normal
Points:
3

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 34
Iteration:
34.3

People

(Reporter: Unfocused, Assigned: geko1702+bugzilla)

References

Details

(Keywords: user-doc-needed, Whiteboard: [user doc: see comment 19 and 21])

Attachments

(2 files, 20 obsolete files)

Screen Shot 2014-08-14 at 6.58.34 PM.png
98.13 KB, image/png
Details
extended popup notifications to create a generic doorhanger for all security notifications incl. mixed content
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+
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?
Attachment #8463563 - Attachment is obsolete: true
(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
Attachment #8464132 - Attachment is obsolete: true
Blocks: 1043803
Attachment #8464174 - Attachment is obsolete: true
Attachment #8465555 - Attachment is obsolete: true
Attachment #8465696 - Flags: feedback?(bmcbride)

    
  

        Attachment #8465869 -
        Attachment is obsolete: true

    
    

    
  
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+

    
    

    
  
(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.

    
  

        Attachment #8465696 -
        Attachment is obsolete: true

    
  

        Attachment #8465870 -
        Attachment is obsolete: true

    
  

        Attachment #8467462 -
        Attachment is obsolete: true

    
    

    
  
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)

    
    

    
  
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

    
  

        Attachment #8469345 -
        Attachment is obsolete: true

    
  

        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)

    
  
Blocks: 1045809

    
  

        Attachment #8469353 -
        Attachment is obsolete: true

    
  

        Attachment #8473260 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached image
          
        Screen Shot 2014-08-14 at 5.45.13 PM.png (obsolete)
        — 
      

    


  

    
  

        Attachment #8465697 -
        Attachment is obsolete: true

    
    

    
  
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)

    
  

        Attachment #8473395 -
        Flags: review?(bmcbride)

    
  

        Attachment #8473395 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #8473396 -
        Attachment is obsolete: true

    
    

    
  
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)

    
  

        Attachment #8473418 -
        Flags: review?(bmcbride)

    
  

        Attachment #8473418 -
        Attachment is obsolete: true

    
  

        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 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.

    
    

    
  
(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.

    
    

    
  
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)

    
  

        Attachment #8476393 -
        Flags: review?(adw)

    
    

    
  
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)

    
  

        Attachment #8475327 -
        Attachment is obsolete: true

        Attachment #8475327 -
        Flags: review?(gijskruitbosch+bugs)

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

    
    

    
  
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+

    
    

    
  
(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.

    
  

        Attachment #8476393 -
        Attachment is obsolete: true

    
    

    
  
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."

    
  

        Attachment #8476774 -
        Attachment is obsolete: true

    
    

    
  
(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

    
    

    
  
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

    
    

    
  
(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
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
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.

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: