1363051 - Update about:blocked page visuals and copy

Status: VERIFIED FIXED

(Firefox :: General, enhancement, P1)

Description

[Johann Hofmann [:johannh]]

See bug 1358536 for specs.

Comment 1

[Justin Dolske [:Dolske]]

Putting on reserve backlog for now.

Comment 2

[:prathiksha]

What links are we going to provide for "reported as a deceptive site" and "report a detection problem" here(https://cl.ly/133s093C2A3x) ? :)

Comment 3

[François Marier [:francois]]

(In reply to :prathiksha from comment #2)
> What links are we going to provide for "reported as a deceptive site" and
> "report a detection problem" here(https://cl.ly/133s093C2A3x) ? :)

The URLs are in https://bugzilla.mozilla.org/show_bug.cgi?id=1358536#c13:

- https://www.google.com/transparencyreport/safebrowsing/diagnostic/index.html?#url=http://testsafebrowsing.appspot.com/s/malware.html (phishing, malware and unwanted)
- https://safebrowsing.google.com/safebrowsing/report_error/?tpl=mozilla (phishing)
- https://www.stopbadware.org/firefox (malware)

Note:

- reporting page is different for malware and phishing
- there is no reporting page for unwanted

Also, the "reported as XXXX" link includes the URL of the bad page as a query string param.

Comment 4

[:prathiksha]

What link should we provide for "Unwanted Software Policy" here (https://cl.ly/3v0F2K2u3c2u) ?

Comment 5

[:prathiksha]

Also, what link should we provide for "reported as containing a potentially harmful application" here (https://cl.ly/0X0T0D1U270w) ?

Thanks :)

Comment 6

[François Marier [:francois]]

(In reply to :prathiksha from comment #4)
> What link should we provide for "Unwanted Software Policy" here
> (https://cl.ly/3v0F2K2u3c2u) ?

https://www.google.com/about/unwanted-software-policy.html

(In reply to :prathiksha from comment #5)
> Also, what link should we provide for "reported as containing a potentially
> harmful application" here (https://cl.ly/0X0T0D1U270w) ?

Same as for malware, phishing and unwanted:

https://www.google.com/transparencyreport/safebrowsing/diagnostic/index.html?#url=<URL of the site>

Comment 7

[:prathiksha]

On updating the about:blocked page, we will no longer have the "reportButton" (Why was this page blocked?) and the "ignoreWarningButton". The update will also introduce a "See Details" button that will open a white box with the error description when we click on it.

What are we going to do with the telemetry code in browser.js (https://searchfox.org/mozilla-central/source/browser/base/content/browser.js#3165) that was written for the "reportButton" and the "ignoreWarningButton"? Are we going to remove that code? Are we going to have the links "support.mozilla.org" and "ignore the risk" do the telemetry stuff instead, since they are replacing the two buttons from the blocked pages ? Do we need telemetry stuff for all the other links and the new "See Details" button in the error description ?

Comment 8

[François Marier [:francois]]

(In reply to :prathiksha from comment #7)
> What are we going to do with the telemetry code in browser.js
> (https://searchfox.org/mozilla-central/source/browser/base/content/browser.
> js#3165) that was written for the "reportButton" and the
> "ignoreWarningButton"? Are we going to remove that code? Are we going to
> have the links "support.mozilla.org" and "ignore the risk" do the telemetry
> stuff instead, since they are replacing the two buttons from the blocked
> pages ? Do we need telemetry stuff for all the other links and the new "See
> Details" button in the error description ?

For "ignoreWarningButton", we need to keep the probe and tie it to "ignore the risk" link. This is how we're going to measure the impact of this new UI.

"reportButton" is not really that relevant anymore. I think we could remove that probe and replace it with a new one on the "See details" button instead. That will help us figure out whether we are exposing enough information in the main section.

Another thing that would be nice is to add a pref in about:config similar to "browser.xul.error_pages.expert_bad_cert" that will open the details pane by default just like that one does for TLS error pages. Could be "browser.xul.error_pages.safe_browsing_details" (defaulting to false).

Comment 9

[:prathiksha]

Attachment: Bug 1363051 - Update about:blocked page visuals and copy.

Review commit: https://reviewboard.mozilla.org/r/178046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/178046/

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review183022

It looks like you're on the right path, but there are still a couple of things to do:

- This patch excludes Android right now, that would be ok for me if we open up a follow-up bug for making the Android changes (the other Photon content MVP bugs were desktop-only as well)
- As you mentioned on IRC, telemetry for the "See Details" button is missing right now, but I would be okay with putting that into a different bug in the interest of getting the string changes out of the door ASAP
- There are still a couple of style changes to be done, please have another look at the mockup (https://cl.ly/133s093C2A3x). The background color needs to be changed and the thingy at the top needs to be removed. The details box also has a lot more padding in the mockup.
- The details box should not push the main content to the top. Try clicking on "see details" on https://wrong.host.badssl.com/ to see what I mean. I think we can pretty much just steal the code from that page. Let me know if you need help with that. Note that the "see details" button also closes the dialog when clicked a second time.
- The "go back" button is missing hover/active state.

I also left some first review comments below.

Thank you so much for all the hard work you're putting into this!

::: browser/base/content/blockedSite.xhtml:37
(Diff revision 1)
>        // the URI that the user attempted to load.
>  
>        function getErrorCode() {
> -        var url = document.documentURI;
> -        var error = url.search(/e\=/);
> -        var duffUrl = url.search(/\&u\=/);
> +        let url = document.documentURI;
> +        let error = url.search(/e\=/);
> +        let duffUrl = url.search(/\&u\=/);

While these changes are generally according to our code style, please avoid doing them on code that is entirely unrelated to your patch, for the following reasons:

- It makes it harder to review your patch and to understand changes looking back at your patch.
- You create a lot of possible merge conflicts.
- It adds "noise" in the hg blame for these lines, removing the original blame information without a clear reason (a commit named "clean up var -> let in blockedSite.xhtml" would be much easier to understand when browsing blame).
- There's a tiny risk of introducing bugs that distract us from our work.

::: browser/base/content/blockedSite.xhtml:134
(Diff revision 1)
> -          el = document.getElementById("errorShortDescText_harmful");
> -          el.remove();
> +          document.getElementById("errorDescription_harmful").remove();
> +          document.getElementById("errorLearnMore_harmful").remove();
> +        }
> +
> +        // Set sitename in error details.
> +        let sitename = document.getElementById(error + "Sitename");

I'd just note that I preferred the errorType_sitename style and it seems more consistent to me.

::: browser/base/content/browser.js:3159
(Diff revision 1)
>          break;
>  
> -      case "reportButton":
> -        // This is the "Why is this site blocked" button. We redirect
> -        // to the generic page describing phishing/malware protection.
> -
> +      case "malwareIgnoreWarningLink":
> +      case "unwantedIgnoreWarningLink":
> +      case "phishingIgnoreWarningLink":
> +      case "harmfulIgnoreWarningLink":

Why does ignoreWarningLink need to be specific for each warning type?

::: browser/base/content/content.js:132
(Diff revision 1)
>  
>  var AboutBlockedSiteListener = {
>    init(chromeGlobal) {
>      addMessageListener("DeceptiveBlockedDetails", this);
>      chromeGlobal.addEventListener("AboutBlockedLoaded", this, false, true);
> +    chromeGlobal.addMessageListener("AboutBlockedSiteListener", this);

What does this do? :)

::: browser/base/content/content.js:172
(Diff revision 1)
>      }
>  
>      let advisoryUrl = Services.prefs.getCharPref(
>        "browser.safebrowsing.provider." + provider + ".advisoryURL", "");
>      if (!advisoryUrl) {
> -      let el = content.document.getElementById("advisoryDesc");
> +      let el = content.document.getElementById("advisoryDescText");

Why did you change this? :)

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:32
(Diff revision 1)
> +<!ENTITY safeb.blocked.unwantedPage.shortDesc2 "Firefox blocked this page because it might try to trick you into installing programs that harm your browsing experience (for example, by changing your homepage or showing extra ads on sites you visit).">
> +<!ENTITY safeb.blocked.unwantedPage.errorDesc "<span id='unwantedSitename'>This webpage</span> has been <a id='unwantedErrorDescLink'>reported as containing harmful software</a>.<span id='unwantedOverridingText'> You can <a id='unwantedIgnoreWarningLink'>ignore the risk</a> and go to this unsafe site.</span>">
> +<!ENTITY safeb.blocked.unwantedPage.learnMore "Learn more about harmful and unwanted software at <a href='https://www.google.com/about/unwanted-software-policy.html'>Unwanted Software Policy</a>. Learn more about Firefox's Phishing and Malware Protection at <a href='https://support.mozilla.org/'>support.mozilla.org</a>.">
>  
> -<!ENTITY safeb.blocked.malwarePage.title "Reported Attack Page!">
> -<!-- Localization note (safeb.blocked.malwarePage.shortDesc) - Please don't translate the contents of the <span id="malware_sitename"/> tag.  It will be replaced at runtime with a domain name (e.g. www.badsite.com) -->
> +<!ENTITY safeb.blocked.phishingPage.title3 "Deceptive site ahead">
> +<!ENTITY safeb.blocked.phishingPage.shortDesc3 "Firefox blocked this page because it may trick you into doing something dangerous like installing software or revealing personal information like passwords or credit cards.">

You will need to add a localization note to you new strings where appropriate (see the ones you removed).

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:37
(Diff revision 1)
> -<!-- Localization note (safeb.blocked.phishingPage.shortDesc2) - Please don't translate the contents of the <span id="phishing_sitename"/> tag. It will be replaced at runtime with a domain name (e.g. www.badsite.com) -->
> -<!ENTITY safeb.blocked.phishingPage.shortDesc2 "This web page at <span id='phishing_sitename'/> has been reported as a deceptive site and has been blocked based on your security preferences.">
> -<!ENTITY safeb.blocked.phishingPage.longDesc2 "<p>Deceptive sites are designed to trick you into doing something dangerous, like installing software, or revealing your personal information, like passwords, phone numbers or credit cards.</p><p>Entering any information on this web page may result in identity theft or other fraud.</p>">
>  
>  <!ENTITY safeb.blocked.harmfulPage.title "The site ahead may contain malware">
> -<!ENTITY safeb.blocked.harmfulPage.shortDesc2 "&brandShortName; blocked this page because it might try to install dangerous apps that steal or delete your information (for example, photos, passwords, messages and credit cards).">
> +<!ENTITY safeb.blocked.harmfulPage.shortDesc3 "Firefox blocked this page because it might try to install dangerous apps that steal or delete your information (for example, photos, passwords, messages and credit cards).">

You need to use &brandShortName; instead of "Firefox"

Comment 12

[:prathiksha]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review183022

> What does this do? :)

We don't need this. Sorry.

> Why did you change this? :)

I changed it by mistake :(

Comment 13

[Francesco Lodolo [:flod]]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review183298

A see a few issues:
* Hard coded Firefox.
* ' instead of ’.
* A huge amount of URLs hard coded into strings, making them hard to update in the future.

One particularly concerning is about removing pieces of text at run-time (unless I misunderstood the intent)

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:19
(Diff revision 1)
>    report-phishing.dtd are never shown at the same time, the same accesskey can
>    be used for them. -->
>  <!ENTITY safeb.palm.notdeceptive.accesskey "d">
> -<!ENTITY safeb.palm.reportPage.label "Why was this page blocked?">
> +
>  <!-- Localization note (safeb.palm.advisory.desc) - Please don't translate <a id="advisory_provider"/> tag.  It will be replaced at runtime with advisory link-->
> -<!ENTITY safeb.palm.advisory.desc "Advisory provided by <a id='advisory_provider'/>">
> +<!ENTITY safeb.palm.advisory.desc "Advisory provided by <a id='advisory_provider'/>.">

Please update the string ID.

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:22
(Diff revision 1)
> -<!ENTITY safeb.palm.reportPage.label "Why was this page blocked?">
> +
>  <!-- Localization note (safeb.palm.advisory.desc) - Please don't translate <a id="advisory_provider"/> tag.  It will be replaced at runtime with advisory link-->
> -<!ENTITY safeb.palm.advisory.desc "Advisory provided by <a id='advisory_provider'/>">
> +<!ENTITY safeb.palm.advisory.desc "Advisory provided by <a id='advisory_provider'/>.">
> +
> +<!ENTITY safeb.blocked.malwarePage.title2 "Visiting this website may harm your computer">
> +<!ENTITY safeb.blocked.malwarePage.shortDesc2 "Firefox blocked this page because it might attempt to install malicious software that may steal or delete personal information on your computer.">

Don't hard code Firefox in strings.

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:23
(Diff revision 1)
>  <!-- Localization note (safeb.palm.advisory.desc) - Please don't translate <a id="advisory_provider"/> tag.  It will be replaced at runtime with advisory link-->
> -<!ENTITY safeb.palm.advisory.desc "Advisory provided by <a id='advisory_provider'/>">
> +<!ENTITY safeb.palm.advisory.desc "Advisory provided by <a id='advisory_provider'/>.">
> +
> +<!ENTITY safeb.blocked.malwarePage.title2 "Visiting this website may harm your computer">
> +<!ENTITY safeb.blocked.malwarePage.shortDesc2 "Firefox blocked this page because it might attempt to install malicious software that may steal or delete personal information on your computer.">
> +<!ENTITY safeb.blocked.malwarePage.errorDesc "<span id='malwareSitename'>This webpage</span> has been <a id='malwareErrorDescLink'>reported as containing malicious software</a>. You can <a href='https://www.stopbadware.org/firefox/report-badware'>report a detection problem</a><span id='malwareOverridingText'> or <a id='malwareIgnoreWarningLink'>ignore the risk</a> and go to this unsafe site</span>.">

Do we really want an hard-coded URL in the string? I'd suggest to avoid it.

I have the feeling you're doing risky assumptions on other languages.

"This is the message A<span>or B</span>."

Do you plan to drop the <span> at run-time? While that works for English, is far from safe in other languages.

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:24
(Diff revision 1)
> -<!ENTITY safeb.palm.advisory.desc "Advisory provided by <a id='advisory_provider'/>">
> +<!ENTITY safeb.palm.advisory.desc "Advisory provided by <a id='advisory_provider'/>.">
> +
> +<!ENTITY safeb.blocked.malwarePage.title2 "Visiting this website may harm your computer">
> +<!ENTITY safeb.blocked.malwarePage.shortDesc2 "Firefox blocked this page because it might attempt to install malicious software that may steal or delete personal information on your computer.">
> +<!ENTITY safeb.blocked.malwarePage.errorDesc "<span id='malwareSitename'>This webpage</span> has been <a id='malwareErrorDescLink'>reported as containing malicious software</a>. You can <a href='https://www.stopbadware.org/firefox/report-badware'>report a detection problem</a><span id='malwareOverridingText'> or <a id='malwareIgnoreWarningLink'>ignore the risk</a> and go to this unsafe site</span>.">
> +<!ENTITY safeb.blocked.malwarePage.learnMore "Learn more about harmful web content including viruses and other malware and how to protect your computer at <a href='https://www.stopbadware.org/firefox'>StopBadware.org</a>. Learn more about Firefox's Phishing and Malware Protection at <a href='https://support.mozilla.org/'>support.mozilla.org</a>.">

Don't hard code Firefox, use proper apostrophes instead of straight quotes, same question about hard coding URLs in strings.

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:27
(Diff revision 1)
> +<!ENTITY safeb.blocked.malwarePage.shortDesc2 "Firefox blocked this page because it might attempt to install malicious software that may steal or delete personal information on your computer.">
> +<!ENTITY safeb.blocked.malwarePage.errorDesc "<span id='malwareSitename'>This webpage</span> has been <a id='malwareErrorDescLink'>reported as containing malicious software</a>. You can <a href='https://www.stopbadware.org/firefox/report-badware'>report a detection problem</a><span id='malwareOverridingText'> or <a id='malwareIgnoreWarningLink'>ignore the risk</a> and go to this unsafe site</span>.">
> +<!ENTITY safeb.blocked.malwarePage.learnMore "Learn more about harmful web content including viruses and other malware and how to protect your computer at <a href='https://www.stopbadware.org/firefox'>StopBadware.org</a>. Learn more about Firefox's Phishing and Malware Protection at <a href='https://support.mozilla.org/'>support.mozilla.org</a>.">
> +
> +<!ENTITY safeb.blocked.unwantedPage.title2 "The site ahead may contain harmful programs">
> +<!ENTITY safeb.blocked.unwantedPage.shortDesc2 "Firefox blocked this page because it might try to trick you into installing programs that harm your browsing experience (for example, by changing your homepage or showing extra ads on sites you visit).">

Hard coded Firefox

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:28
(Diff revision 1)
> +<!ENTITY safeb.blocked.malwarePage.errorDesc "<span id='malwareSitename'>This webpage</span> has been <a id='malwareErrorDescLink'>reported as containing malicious software</a>. You can <a href='https://www.stopbadware.org/firefox/report-badware'>report a detection problem</a><span id='malwareOverridingText'> or <a id='malwareIgnoreWarningLink'>ignore the risk</a> and go to this unsafe site</span>.">
> +<!ENTITY safeb.blocked.malwarePage.learnMore "Learn more about harmful web content including viruses and other malware and how to protect your computer at <a href='https://www.stopbadware.org/firefox'>StopBadware.org</a>. Learn more about Firefox's Phishing and Malware Protection at <a href='https://support.mozilla.org/'>support.mozilla.org</a>.">
> +
> +<!ENTITY safeb.blocked.unwantedPage.title2 "The site ahead may contain harmful programs">
> +<!ENTITY safeb.blocked.unwantedPage.shortDesc2 "Firefox blocked this page because it might try to trick you into installing programs that harm your browsing experience (for example, by changing your homepage or showing extra ads on sites you visit).">
> +<!ENTITY safeb.blocked.unwantedPage.errorDesc "<span id='unwantedSitename'>This webpage</span> has been <a id='unwantedErrorDescLink'>reported as containing harmful software</a>.<span id='unwantedOverridingText'> You can <a id='unwantedIgnoreWarningLink'>ignore the risk</a> and go to this unsafe site.</span>">

This should be relatively safer, since you're removing the entire sentence (if you're removing the <span> at run-time).

Comment 14

[:prathiksha]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review183298

Yes, I'm currently hiding pieces of text at run time when we don't want to give the user an option to "ignore the risk" and visit the website anyway. We may want to hide that link if the website is not overridable(https://bugzilla.mozilla.org/show_bug.cgi?id=1226490). I agree that its risky. Is there any other way I can hide the "ignore the risk" link which will take the user to the website? :)

Here's the mockup: https://cl.ly/133s093C2A3x

Comment 15

[Francesco Lodolo [:flod]]

Any chance to split the sentence in two stand-alone bits? e.g.

<span id='malwareSitename'>This webpage</span> has been <a id='malwareErrorDescLink'>reported as containing malicious software</a>. You can <a href='https://www.stopbadware.org/firefox/report-badware'>report a detection problem</a>. <span id='malwareOverridingText'>It’s also possible to <a id='malwareIgnoreWarningLink'>ignore the risk</a> and go to this unsafe site.</span>

Doing patchwork on languages with a completely different structure is searching for troubles.

One further alternative is to duplicate the sentence.

<span id='malwareSitename'>This webpage</span> has been <a id='malwareErrorDescLink'>reported as containing malicious software</a>. You can <a href='https://www.stopbadware.org/firefox/report-badware'>report a detection problem</a> or <a id='malwareIgnoreWarningLink'>ignore the risk</a> and go to this unsafe site.

<span id='malwareSitename'>This webpage</span> has been <a id='malwareErrorDescLink'>reported as containing malicious software</a>. You can <a href='https://www.stopbadware.org/firefox/report-badware'>report a detection problem</a>.

Comment 16

[:prathiksha]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178046/diff/1-2/

Comment 17

[:prathiksha]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

Things left to do:

-Stop the page from shifting upwards when we click on the "See details" button.
-Use less risky ways to hide the "ignore the risk" link instead of hiding pieces of code during run time.
-Update code here > https://searchfox.org/mozilla-central/source/toolkit/components/viewsource/content/viewSource-content.js#391

Comment 18

[Francesco Lodolo [:flod]]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review183424

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:24
(Diff revision 2)
> -<!ENTITY safeb.palm.advisory.desc "Advisory provided by <a id='advisory_provider'/>">
> +<!ENTITY safeb.palm.advisory.desc2 "Advisory provided by <a id='advisory_provider'/>.">
> +
> +<!ENTITY safeb.blocked.malwarePage.title2 "Visiting this website may harm your computer">
> +<!ENTITY safeb.blocked.malwarePage.shortDesc2 "&brandShortName; blocked this page because it might attempt to install malicious software that may steal or delete personal information on your computer.">
> +
> +<!ENTITY safeb.blocked.malwarePage.reportDetection   "https://www.stopbadware.org/firefox/report-badware">

These URLs should be directly in the code, not in localizable files, they're identical for all languages.

That way, any update would not require string changes.

(I should have been more explicit in the previous comment)

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:29
(Diff revision 2)
> +<!ENTITY safeb.blocked.malwarePage.reportDetection   "https://www.stopbadware.org/firefox/report-badware">
> +<!ENTITY safeb.blocked.malwarePage.learnMoreLink     "https://www.stopbadware.org/firefox">
> +<!ENTITY safeb.blocked.malwarePage.firefoxSupport    "https://support.mozilla.org/">
> +
> +<!-- Localization note (safeb.blocked.malwarePage.errorDesc) - <span id='malware_sitename'>This webpage</span> will be replaced at runtime with the error page host name. If there exists no host name, it will fall back on "This webpage". <a id='malware_error_desc_link'>reported as containing malicious software</a> and <a id='malware_ignore_warning_link'>ignore the risk</a> are replaced with links at runtime. -->
> +<!ENTITY safeb.blocked.malwarePage.errorDesc "<span id='malware_sitename'>This webpage</span> has been <a id='malware_error_desc_link'>reported as containing malicious software</a>. You can <a href='&safeb.blocked.malwarePage.reportDetection;'>report a detection problem</a><span id='malware_overriding_text'> or <a id='malware_ignore_warning_link'>ignore the risk</a> and go to this unsafe site</span>.">

This is another problem for localization.

"blocked" is an adjective and needs to be declined accordingly to the associated noun. For example, webpage is female in Italian, a domain is male, so the sentence will fall apart in one of the two cases.

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:36
(Diff revision 2)
> +<!ENTITY safeb.blocked.malwarePage.learnMore "Learn more about harmful web content including viruses and other malware and how to protect your computer at <a href='&safeb.blocked.malwarePage.learnMoreLink;'>StopBadware.org</a>. Learn more about &brandShortName;'s Phishing and Malware Protection at <a href='&safeb.blocked.malwarePage.firefoxSupport;'>support.mozilla.org</a>.">
> +
> +<!ENTITY safeb.blocked.unwantedPage.title2 "The site ahead may contain harmful programs">
> +<!ENTITY safeb.blocked.unwantedPage.shortDesc2 "&brandShortName; blocked this page because it might try to trick you into installing programs that harm your browsing experience (for example, by changing your homepage or showing extra ads on sites you visit).">
> +
> +<!ENTITY safeb.blocked.unwantedPage.learnMoreLink     "https://www.google.com/about/unwanted-software-policy.html">

Same note about URLs

Comment 19

[Johann Hofmann [:johannh]]

Thank you for the help, flod.

(In reply to Francesco Lodolo [:flod] from comment #18)
> :::
> browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-
> message.dtd:29
> (Diff revision 2)
> > +<!ENTITY safeb.blocked.malwarePage.reportDetection   "https://www.stopbadware.org/firefox/report-badware">
> > +<!ENTITY safeb.blocked.malwarePage.learnMoreLink     "https://www.stopbadware.org/firefox">
> > +<!ENTITY safeb.blocked.malwarePage.firefoxSupport    "https://support.mozilla.org/">
> > +
> > +<!-- Localization note (safeb.blocked.malwarePage.errorDesc) - <span id='malware_sitename'>This webpage</span> will be replaced at runtime with the error page host name. If there exists no host name, it will fall back on "This webpage". <a id='malware_error_desc_link'>reported as containing malicious software</a> and <a id='malware_ignore_warning_link'>ignore the risk</a> are replaced with links at runtime. -->
> > +<!ENTITY safeb.blocked.malwarePage.errorDesc "<span id='malware_sitename'>This webpage</span> has been <a id='malware_error_desc_link'>reported as containing malicious software</a>. You can <a href='&safeb.blocked.malwarePage.reportDetection;'>report a detection problem</a><span id='malware_overriding_text'> or <a id='malware_ignore_warning_link'>ignore the risk</a> and go to this unsafe site</span>.">
> 
> This is another problem for localization.
> 
> "blocked" is an adjective and needs to be declined accordingly to the
> associated noun. For example, webpage is female in Italian, a domain is
> male, so the sentence will fall apart in one of the two cases.

Ok, so the best way to go here is to make two versions of that sentence, one with "This webpage" and one for URLs?

Comment 20

[Francesco Lodolo [:flod]]

(In reply to Johann Hofmann [:johannh] from comment #19)
> Ok, so the best way to go here is to make two versions of that sentence, one
> with "This webpage" and one for URLs?

Sadly, yes.

Comment 21

[:prathiksha]

Should we keep the "report a detection problem" link visible for websites that cannot be overridden?

See https://bugzilla.mozilla.org/show_bug.cgi?id=1226490

Comment 22

[:prathiksha]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178046/diff/2-3/

Comment 23

[François Marier [:francois]]

(In reply to :prathiksha from comment #21)
> Should we keep the "report a detection problem" link visible for websites
> that cannot be overridden?
> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=1226490

Good catch. I think we should. False positives are even more of a problem if users can't click through the warning.

Comment 24

[Francesco Lodolo [:flod]]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review184226

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:33
(Diff revision 3)
> +
> +<!ENTITY safeb.blocked.malwarePage.errorDesc.noOverride "<span id='malware_sitename'/> has been <a id='malware_error_desc_link'>reported as containing malicious software</a>.">
> +
> +<!ENTITY safeb.blocked.malwarePage.errorDesc.noHost "This webpage has been reported as containing malicious software.">
> +
> +<!ENTITY safeb.blocked.malwarePage.learnMore "Learn more about harmful web content including viruses and other malware and how to protect your computer at <a id='malware_learn_more_link'>StopBadware.org</a>. Learn more about &brandShortName;'s Phishing and Malware Protection at <a id='malware_firefox_support'>support.mozilla.org</a>.">

' -> ’

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:47
(Diff revision 3)
> +
> +<!ENTITY safeb.blocked.unwantedPage.errorDesc.noOverride "<span id='unwanted_sitename'/> has been <a id='unwanted_error_desc_link'>reported as containing harmful software</a>.">
> +
> +<!ENTITY safeb.blocked.unwantedPage.errorDesc.noHost "This webpage has been reported as containing harmful software.">
> +
> +<!ENTITY safeb.blocked.unwantedPage.learnMore "Learn more about harmful and unwanted software at <a id='unwanted_learn_more_link'>Unwanted Software Policy</a>. Learn more about &brandShortName;'s Phishing and Malware Protection at <a id='unwanted_firefox_support'>support.mozilla.org</a>.">

' -> ’

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:60
(Diff revision 3)
> +
> +<!ENTITY safeb.blocked.phishingPage.errorDesc.noOverride "<span id='phishing_sitename'/> has been <a id='phishing_error_desc_link'>reported as a deceptive site</a>.">
> +
> +<!ENTITY safeb.blocked.phishingPage.errorDesc.noHost "This webpage has been reported as a deceptive site.">
> +
> +<!ENTITY safeb.blocked.phishingPage.learnMore "Learn more about deceptive sites and phishing at <a id='phishing_learn_more_link'>www.antiphishing.org</a>. Learn more about &brandShortName;'s Phishing and Malware Protection at <a id='phishing_firefox_support'>support.mozilla.org</a>.">

' -> ’

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:64
(Diff revision 3)
> -<!-- Localization note (safeb.blocked.phishingPage.shortDesc2) - Please don't translate the contents of the <span id="phishing_sitename"/> tag. It will be replaced at runtime with a domain name (e.g. www.badsite.com) -->
> -<!ENTITY safeb.blocked.phishingPage.shortDesc2 "This web page at <span id='phishing_sitename'/> has been reported as a deceptive site and has been blocked based on your security preferences.">
> -<!ENTITY safeb.blocked.phishingPage.longDesc2 "<p>Deceptive sites are designed to trick you into doing something dangerous, like installing software, or revealing your personal information, like passwords, phone numbers or credit cards.</p><p>Entering any information on this web page may result in identity theft or other fraud.</p>">
>  
>  <!ENTITY safeb.blocked.harmfulPage.title "The site ahead may contain malware">
> -<!ENTITY safeb.blocked.harmfulPage.shortDesc2 "&brandShortName; blocked this page because it might try to install dangerous apps that steal or delete your information (for example, photos, passwords, messages and credit cards).">
> +<!ENTITY safeb.blocked.harmfulPage.shortDesc3 "&brandShortName; blocked this page because it might try to install dangerous apps that steal or delete your information (for example, photos, passwords, messages and credit cards).">

This string didn't change, you don't need to change the ID.

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:73
(Diff revision 3)
> +
> +<!ENTITY safeb.blocked.harmfulPage.errorDesc.noOverride "<span id='harmful_sitename'/> has been <a id='harmful_error_desc_link'>reported as containing a potentially harmful application</a>.">
> +
> +<!ENTITY safeb.blocked.harmfulPage.errorDesc.noHost "This webpage has been reported as containing a potentially harmful application.">
> +
> +<!ENTITY safeb.blocked.harmfulPage.learnMore "Learn more about &brandShortName;'s Phishing and Malware Protection at <a id='harmful_firefox_support'>support.mozilla.org</a>.">

' -> ’

Comment 25

[Francesco Lodolo [:flod]]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

There are still a few ' (btw, you can search in page for ', toggle the case sensitive search and highlight them), and one string ID changed needlessly, but it looks better.

I still thing "stealing credit cards" is factually wrong (you steal credit card numbers or data, not piece of plastics), but at this point I'm not sure whose call it is.

Comment 26

[Johann Hofmann [:johannh]]

(In reply to Francesco Lodolo [:flod] from comment #25)
> Comment on attachment 8906315 [details]
> Bug 1363051 - Update about:blocked page visuals and copy.
> 
> I still thing "stealing credit cards" is factually wrong (you steal credit
> card numbers or data, not piece of plastics), but at this point I'm not sure
> whose call it is.

I would agree. François, do you have an opinion on this?

Comment 27

[Johann Hofmann [:johannh]]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review184256

The page looks much better now, thank you. To make the panel not push the main content upwards, you will need to apply these styles to #errorLongDesc and .error-description: https://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/browser/themes/shared/aboutNetError.css#78-92

(Ideally we could share this style between those pages, e.g. in error-pages.css, but I'd be ok with doing that in a follow-up).

::: browser/base/content/blockedSite.xhtml
(Diff revision 3)
>              break;
>            case "harmfulBlocked" :
>              error = "harmful";
>              break;
> -          default:
> -            return;

What makes you think that having no error code (simply about:blocked) is something we will need to handle gracefully in the future? It's broken in my current Nightly, and I'm quite certain we can leave it unsupported.

::: browser/base/content/blockedSite.xhtml:112
(Diff revision 3)
> +        /**
> +        /* If there exists no error code i.e error = "" (for example, about:blocked),
> +        /* we show the string with no hostname in it
> +        /* (for example, "This webpage has been reported as a deceptive site.").
> +        */
> +        if (error == "") {

I don't think we need to handle this.

::: browser/base/content/blockedSite.xhtml:125
(Diff revision 3)
> +          document.getElementById("harmful_error_desc_no_override").remove();
> +        }
> +
>          var el;
>  
> -        if (error !== "malware") {
> +        if (error && error !== "malware") {

Why did you change this? :)

::: browser/base/content/blockedSite.xhtml:162
(Diff revision 3)
> +          el = document.getElementById("errorLongDesc_harmful");
> +          el.remove();
> +        }
> +
> +        if (error !== "") {
> +          el = document.getElementById(error + "_error_desc_no_host");

See above, I don't think we need to handle the error == "" case at all.

::: browser/base/content/blockedSite.xhtml:174
(Diff revision 3)
> +          }
>          }
>  
> -        // Set sitename if necessary.
> +        // Set sitename in error details.
>          let sitenameElem = document.getElementById(error + "_sitename");
>          if (sitenameElem) {

I know this isn't your code, but is there ever any possibility that sitenameElem does not exist?

::: browser/base/content/blockedSite.xhtml:176
(Diff revision 3)
>  
> -        // Set sitename if necessary.
> +        // Set sitename in error details.
>          let sitenameElem = document.getElementById(error + "_sitename");
>          if (sitenameElem) {
> +          sitenameElem.setAttribute("class", "sitename");
>            sitenameElem.textContent = getHostString();

This is the place where we would want to show the no_host strings (if there's no sensible host name). However, looking at the getHostString() code now, it seems quite unlikely to me that we will ever not have a host to display. I would recommend removing all no_host and noHost parts to get rid of some of the complexity here.

::: browser/base/content/blockedSite.xhtml:184
(Diff revision 3)
> -        document.title = document.getElementById("errorTitleText_" + error)
> -                                 .innerHTML;
> -
> -        if (!getOverride()) {
> -          var btn = document.getElementById("ignoreWarningButton");
> -          if (btn) {
> +        /** Set error description link in error details.
> +        * For example, the "reported as a deceptive site" link for
> +        * blocked phishing pages.
> +        */
> +        let desc = document.getElementById(error + "_error_desc_link");
> +        if (desc) {

Again, can this element ever not exist?

::: browser/base/content/blockedSite.xhtml:186
(Diff revision 3)
> -
> -        if (!getOverride()) {
> -          var btn = document.getElementById("ignoreWarningButton");
> -          if (btn) {
> -            btn.remove();
> -          }
> +        * blocked phishing pages.
> +        */
> +        let desc = document.getElementById(error + "_error_desc_link");
> +        if (desc) {
> +          desc.setAttribute("href",
> +            "https://www.google.com/transparencyreport/safebrowsing/diagnostic/index.html?#url=" + getURL());

As mentioned, this should probably be set in content.js from a pref.

::: browser/base/content/blockedSite.xhtml:190
(Diff revision 3)
> -            btn.remove();
> -          }
> +          desc.setAttribute("href",
> +            "https://www.google.com/transparencyreport/safebrowsing/diagnostic/index.html?#url=" + getURL());
> +        }
> +
> +        if (error) {
> +          document.title = document.getElementById("errorTitleText_" + error).innerHTML;

Don't use innerHTML here, you can use .textContent instead.

::: browser/base/content/blockedSite.xhtml:222
(Diff revision 3)
> +              "https://www.antiphishing.org//");
> +            document.getElementById("phishing_firefox_support").setAttribute("href",
> +              "https://support.mozilla.org/");
> +          case "harmful" :
> +            document.getElementById("harmful_firefox_support").setAttribute("href",
> +              "https://support.mozilla.org/");

I would advise to set links in content.js, like we're doing for advisory_provider (https://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/browser/base/content/content.js#184). That way we can read these URLs from prefs.

Note that we also shouldn't hardcode support URLs, you need to construct them from a pref as well: https://searchfox.org/mozilla-central/rev/d29c832536839b60a9ee5b512eeb934274b879b0/browser/components/preferences/in-content/privacy.js#76

::: browser/themes/shared/blockedSite.css:9
(Diff revision 3)
>  
>  @import url("chrome://browser/skin/error-pages.css");
>  
>  body {
> -  background-image: linear-gradient(-45deg, #9b2e2e,     #9b2e2e 33%,
> -                                            #a83232 33%, #a83232 66%,
> +  background-color: #A4000F;
> +  color: #FFFFFF;

Nit: Please use white instead of #FFFFFF (this applies to all instances below).

Comment 28

[François Marier [:francois]]

(In reply to Johann Hofmann [:johannh] from comment #26)
> (In reply to Francesco Lodolo [:flod] from comment #25)
> > Comment on attachment 8906315 [details]
> > Bug 1363051 - Update about:blocked page visuals and copy.
> > 
> > I still thing "stealing credit cards" is factually wrong (you steal credit
> > card numbers or data, not piece of plastics), but at this point I'm not sure
> > whose call it is.
> 
> I would agree. François, do you have an opinion on this?

While I agree that there is a distinction between CC numbers and CC and that CC is technically wrong, I'm not convinced that it matters for users.

Also, the recommended user warnings from Google use "credit cards", not "credit card numbers": https://developers.google.com/safe-browsing/v4/usage-limits#UserWarnings

Comment 29

[:prathiksha]

The about:blocked page is currently broken for view-source pages on Nightly. We could fix it as a part of this bug but I really think we should just file a new bug to fix it and not let it get in the way of this bug.

Comment 30

[:prathiksha]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178046/diff/3-4/

Comment 31

[Johann Hofmann [:johannh]]

(In reply to :prathiksha from comment #29)
> The about:blocked page is currently broken for view-source pages on Nightly.
> We could fix it as a part of this bug but I really think we should just file
> a new bug to fix it and not let it get in the way of this bug.

I think that's a good idea.

Comment 32

[Johann Hofmann [:johannh]]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review184946

The site is looking great already, just a bunch of nits to fix. Please flag francois (and me) for the next round of review, especially to double-check the strings and SafeBrowsing pecularities.

As mentioned in IRC, this is failing a lot of tests that still need to get fixed up, but I'm confident that we can get this in with 57.

::: browser/base/content/blockedSite.xhtml:105
(Diff revision 4)
>              error = "unwanted";
>              break;
>            case "harmfulBlocked" :
>              error = "harmful";
>              break;
> -          default:
> +          default :

Nit: I know it's inconsistent, but let's avoid touching blame here.

::: browser/base/content/blockedSite.xhtml:162
(Diff revision 4)
> -            btn.remove();
> -          }
> -        }
>  
>          // Inform the test harness that we're done loading the page
> -        var event = new CustomEvent("AboutBlockedLoaded", {bubbles: true});
> +        let u = getURL();

Nit: why don't you just inline that variable into the object below?

::: browser/base/content/content.js:177
(Diff revision 4)
> +    * blocked phishing pages.
> +    */
> +    let desc = Services.prefs.getCharPref(
> +      "browser.safebrowsing.provider." + provider + ".reportURL", "");
> +    if (!desc) {
> +      doc.getElementById("error_desc_link").setAttribute("class", "hide-link");

See below, you don't need this class.

::: browser/base/content/content.js:186
(Diff revision 4)
> +
> +    // Set other links in error details.
> +    switch (aEvent.detail.err) {
> +      case "malware" :
> +        doc.getElementById("report_detection").setAttribute("href",
> +          "https://www.stopbadware.org/firefox");

I would assume that we have to put all these URLs in prefs, but I'll let francois make that call.

::: browser/base/content/content.js:226
(Diff revision 4)
>      }
>  
>      let anchorEl = content.document.getElementById("advisory_provider");
>      anchorEl.setAttribute("href", advisoryUrl);
>      anchorEl.textContent = advisoryLinkText;
> +

Nit: I don't think we need the new blank line.

::: browser/themes/shared/blockedSite.css:25
(Diff revision 4)
> -.button-container button:not(.primary) {
> +.button-container button {
>    background-color: transparent;
>    color: white;
> -  border: 1px solid #9b2e2e;
> +  border: 1px solid white;
>    margin-inline-end: 0;
> +  margin-top: 10px;

This is getting overriden by the rule in line 39, you can just remove it.

::: browser/themes/shared/blockedSite.css:29
(Diff revision 4)
>    margin-inline-end: 0;
> +  margin-top: 10px;
>  }
>  
> -.button-container button:not(.primary):hover {
> +.button-container button:hover {
>    background-color: #a83232;

So lacking input from bram (he's on PTO), I asked our local UX people in the Berlin office. We had a hard time finding good hover colors for these buttons, but my suggestion is the following:

For this color, use Red 80 from http://design.firefox.com/photon/visual/color.html#red

::: browser/themes/shared/blockedSite.css:33
(Diff revision 4)
> -.button-container button:not(.primary):hover {
> +.button-container button:hover {
>    background-color: #a83232;
>  }
>  
> -.button-container button:not(.primary):active {
> -  background-color: #9b2e2e;
> +.button-container button:active {
> +  background-color: #A4000F;

Use Red 90 from http://design.firefox.com/photon/visual/color.html#red

::: browser/themes/shared/blockedSite.css:37
(Diff revision 4)
> -.button-container button:not(.primary):active {
> -  background-color: #9b2e2e;
> +.button-container button:active {
> +  background-color: #A4000F;
>  }
>  
>  .button-container button {
> -  margin-top: 1.2em;
> +  margin-top: 1.5em;

Please merge this selector with the one in line 23.

::: browser/themes/shared/blockedSite.css:40
(Diff revision 4)
>  
>  .button-container button {
> -  margin-top: 1.2em;
> +  margin-top: 1.5em;
> +}
> +
> +#goBackButton {

For hover and active states for this button, please use white with 0.8 and 0.7 opacity.

::: browser/themes/shared/blockedSite.css:50
(Diff revision 4)
> -  padding: 0;
> -  font-size: smaller;
> -  min-width: 0;
>  }
>  
> -#ignoreWarningButton:hover {
> +#errorLongDesc {

Nit: this should probably be called #errorDescriptionContainer or something like that.

::: browser/themes/shared/blockedSite.css:70
(Diff revision 4)
> +
> +.error-description > p:last-child {
> +  padding: 0 3.5em 3.5em 3.5em;
> +}
> +
> +a, a:hover, a:active, a:visited {

You should add the :link pseudo-selector to your a's.

https://developer.mozilla.org/en-US/docs/Web/CSS/:link

Also, this is limited to links inside .error-description, right? Should probably add that to the selector.

::: browser/themes/shared/blockedSite.css:76
(Diff revision 4)
> +  cursor: pointer;
>    text-decoration: underline;
> +  color: black;
> +}
> +
> +.hide-link, .hide-link:hover {

The correct solution to this IMO would be to fix up these rules: https://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/toolkit/themes/shared/in-content/common.inc.css#524 to heed to the :link pseudo-selector. Let's not do that in this patch, though. (Though we should really file a bug!)

To avoid setting the .hide-link class, you can use "a:not(:link)" as a selector instead.

::: browser/themes/shared/blockedSite.css:77
(Diff revision 4)
>    text-decoration: underline;
> +  color: black;
> +}
> +
> +.hide-link, .hide-link:hover {
> +  text-decoration : none;

Nit: Remove the space between "text-decoration" and the colon

::: browser/themes/shared/blockedSite.css:78
(Diff revision 4)
> +  color: black;
> +}
> +
> +.hide-link, .hide-link:hover {
> +  text-decoration : none;
> +  cursor : auto;

Nit: see above.

::: browser/themes/shared/blockedSite.css:79
(Diff revision 4)
> +}
> +
> +.hide-link, .hide-link:hover {
> +  text-decoration : none;
> +  cursor : auto;
> +}

For this to completely work you will also need to override the text color with !important, I think.

Comment 33

[François Marier [:francois]]

(In reply to :prathiksha from comment #29)
> The about:blocked page is currently broken for view-source pages on Nightly.
> We could fix it as a part of this bug but I really think we should just file
> a new bug to fix it and not let it get in the way of this bug.

There is bug 1388928 for issues with view-source: URLs.

Comment 34

[:prathiksha]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178046/diff/4-5/

Comment 35

[:prathiksha]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

test_safe_browsing_notification.py is failing in this revision and I can't figure out how to fix this python test.

Comment 36

[Johann Hofmann [:johannh]]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review185186

Just a few comments, then this should be good to go in my view.

::: browser/base/content/blockedSite.xhtml
(Diff revision 5)
>              break;
>            case "harmfulBlocked" :
>              error = "harmful";
>              break;
> -          default:
> -            return;

Don't remove this.

::: browser/base/content/content.js:182
(Diff revision 5)
> +      doc.getElementById("error_desc_link").setAttribute("href", desc + aEvent.detail.url);
> +    }
> +
> +    // Set other links in error details.
> +    switch (aEvent.detail.err) {
> +      case "malware" :

Nit: remove the space between string and colon.

::: browser/base/content/content.js:188
(Diff revision 5)
> +        doc.getElementById("report_detection").setAttribute("href",
> +          "https://www.stopbadware.org/firefox");
> +        doc.getElementById("learn_more_link").setAttribute("href",
> +          "https://www.stopbadware.org/firefox");
> +        break;
> +      case "unwanted" :

Nit: space in front of colon

::: browser/base/content/content.js:192
(Diff revision 5)
> +        break;
> +      case "unwanted" :
> +        doc.getElementById("learn_more_link").setAttribute("href",
> +          "https://www.google.com/about/unwanted-software-policy.html");
> +        break;
> +      case "phishing" :

Nit: space in front of colon

::: browser/themes/shared/blockedSite.css:8
(Diff revision 5)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  @import url("chrome://browser/skin/error-pages.css");
>  
>  body {
> -  background-image: linear-gradient(-45deg, #9b2e2e,     #9b2e2e 33%,
> +  background-color: #A4000F;

This background color needs to be set on html instead of body, because the dialog can overflow on small window sizes, showing white background color.

::: browser/themes/shared/blockedSite.css:32
(Diff revision 5)
>  
> -.button-container button:not(.primary):hover {
> -  background-color: #a83232;
> +.button-container button:hover {
> +  background-color: #5a0002;
>  }
>  
> -.button-container button:not(.primary):active {
> +.button-container button:active {

This needs to be :hover:active to override https://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/toolkit/themes/shared/in-content/common.inc.css#219

::: browser/themes/shared/blockedSite.css:76
(Diff revision 5)
> +
> +.error-description > p:last-child {
> +  padding: 0 3.5em 3.5em 3.5em;
> +}
> +
> +.error-description > #ignore_warning_link, a, a:hover, a:active, a:visited, a:link {

This selector looks strange, I suspect you're expecting it to evaluate to

(.error-description > #ignore_warning_link, a, a:hover, a:active, a:visited, a:link)

while it really evaluates to

(.error-description > #ignore_warning_link), a, a:hover, a:active, a:visited, a:link

The first part of that selector doesn't work either, because #ignore_warning_link is not a direct descendant of .error-description.

I'd suggest something like

.error-description #ignore_warning_link,
.error-description a:-moz-any(:link, :visited)

instead :)

::: browser/themes/shared/blockedSite.css:84
(Diff revision 5)
> +  color: black;
> +}
> +
> +#error_desc_link:not(:link) {
> +  text-decoration: none;
> +  cursor: auto;

When the previous selector is evaluating correctly this also needs to override text color.

Comment 37

[:prathiksha]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178046/diff/5-6/

Comment 38

[Johann Hofmann [:johannh]]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review185408

This looks good, thank you.

::: browser/base/content/content.js:170
(Diff revision 6)
>        }
>      }
>  
> +    let doc = content.document;
> +
> +    /** Set error description link in error details.

Nit: start the first line in this comment one line below, so

/**
 * Set error description link in error details.
 * ...
 */

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:25
(Diff revision 6)
> +
> +
> +<!ENTITY safeb.blocked.malwarePage.title2 "Visiting this website may harm your computer">
> +<!ENTITY safeb.blocked.malwarePage.shortDesc2 "&brandShortName; blocked this page because it might attempt to install malicious software that may steal or delete personal information on your computer.">
> +
> +<!-- Localization note (safeb.blocked.malwarePage.errorDesc.override and safeb.blocked.malwarePage.errorDesc.noOverride and safeb.blocked.malwarePage.learnMore) - All <span> and <a> tags are replaced by the appropriate links and text during runtime. -->

Please separate the different string IDs with commas instead of "and".

I'm not sure if that's required or not (flod likely would have flagged it), but it's definitely the dominant pattern.

A different pattern I've seen used is e.g.

"Localization note (safeb.blocked.*)"

which could help reduce the amount of notes.

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:37
(Diff revision 6)
> +
> +
> +<!ENTITY safeb.blocked.unwantedPage.title2 "The site ahead may contain harmful programs">
> +<!ENTITY safeb.blocked.unwantedPage.shortDesc2 "&brandShortName; blocked this page because it might try to trick you into installing programs that harm your browsing experience (for example, by changing your homepage or showing extra ads on sites you visit).">
> +
> +<!-- Localization note (safeb.blocked.unwantedPage.errorDesc.override and safeb.blocked.unwantedPage.errorDesc.noOverride and safeb.blocked.unwantedPage.learnMore) - All <span> and <a> tags are replaced by the appropriate links and text during runtime. -->

"and" -> ","

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:49
(Diff revision 6)
> +
> +
> +<!ENTITY safeb.blocked.phishingPage.title3 "Deceptive site ahead">
> +<!ENTITY safeb.blocked.phishingPage.shortDesc3 "&brandShortName; blocked this page because it may trick you into doing something dangerous like installing software or revealing personal information like passwords or credit cards.">
> +
> +<!-- Localization note (safeb.blocked.phishingPage.errorDesc.override and safeb.blocked.phishingPage.errorDesc.noOverride and safeb.blocked.phishingPage.learnMore) - All <span> and <a> tags are replaced by the appropriate links and text during runtime. -->

"and" -> ","

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:60
(Diff revision 6)
> -<!ENTITY safeb.blocked.phishingPage.longDesc2 "<p>Deceptive sites are designed to trick you into doing something dangerous, like installing software, or revealing your personal information, like passwords, phone numbers or credit cards.</p><p>Entering any information on this web page may result in identity theft or other fraud.</p>">
>  
>  <!ENTITY safeb.blocked.harmfulPage.title "The site ahead may contain malware">
>  <!ENTITY safeb.blocked.harmfulPage.shortDesc2 "&brandShortName; blocked this page because it might try to install dangerous apps that steal or delete your information (for example, photos, passwords, messages and credit cards).">
> +
> +<!-- Localization note (safeb.blocked.harmfulPage.errorDesc.override and safeb.blocked.harmfulPage.errorDesc.noOverride and safeb.blocked.harmfulPage.learnMore) - All <span> and <a> tags are replaced by the appropriate links and text during runtime. -->

"and" -> ","

::: toolkit/components/viewsource/content/viewSource-content.js
(Diff revision 6)
> -      } else if (target == errorDoc.getElementById("reportButton")) {
> -        // This is the "Why is this site blocked" button. We redirect
> -        // to the generic page describing phishing/malware protection.
> -        let URL = Services.urlFormatter.formatURLPref("app.support.baseURL");
> -        sendAsyncMessage("ViewSource:OpenURL", { URL })
> -      } else if (target == errorDoc.getElementById("ignoreWarningButton")) {

Please file a bug about this not working correctly.

Comment 39

[:prathiksha]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review185408

> Please file a bug about this not working correctly.

Francois mentioned earlier that there's bug 1388928 already.

Comment 40

[François Marier [:francois]]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review184946

> I would assume that we have to put all these URLs in prefs, but I'll let francois make that call.

I'm not too worried about it, since the URL has been in there for a really long time.

Comment 41

[François Marier [:francois]]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review185606

Looks good to me. The reason for the r- is the incorrect SUMO link.

::: commit-message-50857:1
(Diff revision 6)
> +Bug 1363051 - Update about:blocked page visuals and copy. r?johannh

I guess this is not updating Fennec, only desktop. Is there a bug open for Fennec?

We should also check that this change doesn't break the existing UI on Fennec.

::: browser/base/content/blockedSite.xhtml:208
(Diff revision 6)
>            <!-- Commands handled in browser.js -->
> -          <button id="getMeOutButton" class="primary">&safeb.palm.accept.label;</button>
> -          <div class="button-spacer"></div>
> -          <button id="reportButton">&safeb.palm.reportPage.label;</button>
> +          <button id="goBackButton">&safeb.palm.accept.label2;</button>
> +          <button id="seeDetailsButton" onclick="onClickSeeDetails();">&safeb.palm.seedetails.label;</button>
> +        </div>
> -        </div>
> +      </div>
> +      <div id="errorDescriptionContainer" hidden="true">

If it's not too hard, it would be nice to introduce a `browser.xul.error_pages.safe_browsing_details` pref and have it determine the initial state of the "Show details" panel (see comment 8).

::: browser/base/content/content.js:201
(Diff revision 6)
> +          "https://www.antiphishing.org//");
> +        break;
> +    }
> +
> +    // Set the firefox support url.
> +    let u = Services.urlFormatter.formatURLPref("app.support.baseURL") + "tracking-protection-pbm";

That's the wrong support URL. This is Safe Browsing, not Tracking Protection.

The correct SUMO link is <https://support.mozilla.org/kb/how-does-phishing-and-malware-protection-work>.

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:29
(Diff revision 6)
> +
> +<!-- Localization note (safeb.blocked.malwarePage.errorDesc.override and safeb.blocked.malwarePage.errorDesc.noOverride and safeb.blocked.malwarePage.learnMore) - All <span> and <a> tags are replaced by the appropriate links and text during runtime. -->
> +
> +<!ENTITY safeb.blocked.malwarePage.errorDesc.override "<span id='malware_sitename'/> has been <a id='error_desc_link'>reported as containing malicious software</a>. You can <a id='report_detection'>report a detection problem</a> or <a id='ignore_warning_link'>ignore the risk</a> and go to this unsafe site.">
> +
> +<!ENTITY safeb.blocked.malwarePage.errorDesc.noOverride "<span id='malware_sitename'/> has been <a id='error_desc_link'>reported as containing malicious software</a>. You can <a id='report_detection'>report a detection problem</a>">

Shouldn't the last sentence have a period at the end?

Comment 42

[Francesco Lodolo [:flod]]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review185698

::: browser/locales/en-US/chrome/browser/safebrowsing/phishing-afterload-warning-message.dtd:52
(Diff revision 6)
> +<!ENTITY safeb.blocked.phishingPage.shortDesc3 "&brandShortName; blocked this page because it may trick you into doing something dangerous like installing software or revealing personal information like passwords or credit cards.">
> +
> +<!-- Localization note (safeb.blocked.phishingPage.errorDesc.override and safeb.blocked.phishingPage.errorDesc.noOverride and safeb.blocked.phishingPage.learnMore) - All <span> and <a> tags are replaced by the appropriate links and text during runtime. -->
> +<!ENTITY safeb.blocked.phishingPage.errorDesc.override "<span id='phishing_sitename'/> has been <a id='error_desc_link'>reported as a deceptive site</a>. You can <a id='report_detection'>report a detection problem</a> or <a id='ignore_warning_link'>ignore the risk</a> and go to this unsafe site.">
> +
> +<!ENTITY safeb.blocked.phishingPage.errorDesc.noOverride "<span id='phishing_sitename'/> has been <a id='error_desc_link'>reported as a deceptive site</a>. You can <a id='report_detection'>report a detection problem</a>">

In case, you need to add a closing period here too.

Comment 43

[Johann Hofmann [:johannh]]

(In reply to François Marier [:francois] from comment #41)
> Comment on attachment 8906315 [details]
> Bug 1363051 - Update about:blocked page visuals and copy.
> 
> https://reviewboard.mozilla.org/r/178046/#review185606
> 
> Looks good to me. The reason for the r- is the incorrect SUMO link.
> 
> ::: commit-message-50857:1
> (Diff revision 6)
> > +Bug 1363051 - Update about:blocked page visuals and copy. r?johannh
> 
> I guess this is not updating Fennec, only desktop. Is there a bug open for
> Fennec?

I asked for opening one for updating Fennec (Android) in comment 11, but I think that wasn't done yet.

> 
> We should also check that this change doesn't break the existing UI on
> Fennec.

Yes, we should do that ASAP. Good catch.

> 
> ::: browser/base/content/blockedSite.xhtml:208
> (Diff revision 6)
> >            <!-- Commands handled in browser.js -->
> > -          <button id="getMeOutButton" class="primary">&safeb.palm.accept.label;</button>
> > -          <div class="button-spacer"></div>
> > -          <button id="reportButton">&safeb.palm.reportPage.label;</button>
> > +          <button id="goBackButton">&safeb.palm.accept.label2;</button>
> > +          <button id="seeDetailsButton" onclick="onClickSeeDetails();">&safeb.palm.seedetails.label;</button>
> > +        </div>
> > -        </div>
> > +      </div>
> > +      <div id="errorDescriptionContainer" hidden="true">
> 
> If it's not too hard, it would be nice to introduce a
> `browser.xul.error_pages.safe_browsing_details` pref and have it determine
> the initial state of the "Show details" panel (see comment 8).

Let's do that in a follow-up bug, if we want to retain our chances of getting this into 57.

> ::: browser/base/content/content.js:201
> (Diff revision 6)
> > +          "https://www.antiphishing.org//");
> > +        break;
> > +    }
> > +
> > +    // Set the firefox support url.
> > +    let u = Services.urlFormatter.formatURLPref("app.support.baseURL") + "tracking-protection-pbm";
> 
> That's the wrong support URL. This is Safe Browsing, not Tracking Protection.
> 
> The correct SUMO link is
> <https://support.mozilla.org/kb/how-does-phishing-and-malware-protection-
> work>.

Ooh, right. That is quite unfortunate. We can't link to that URL using app.support.baseURL, because we need the SUMO content manager to set us up with a special redirect URL. That's already stalling bug 1359289. At this point I'd recommend for us and bug 1359289 to just hardcode the URL you set above and make a new bug for adding the app.support.baseURL version later, whenever Joni gets back to us.

Prathiksha, would you mind changing the URL back to https://support.mozilla.org/kb/how-does-phishing-and-malware-protection-work? Sorry for the back and forth.

Comment 44

[Johann Hofmann [:johannh]]

I tested the deceptive warning site with this patch on Android and everything's still working for me, so I think we're good.

Comment 45

[:prathiksha]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178046/diff/6-7/

Comment 46

[François Marier [:francois]]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

https://reviewboard.mozilla.org/r/178046/#review186114

::: commit-message-50857:1
(Diff revisions 6 - 7)
> -Bug 1363051 - Update about:blocked page visuals and copy. r?johannh
> + Bug 1363051 - Update about:blocked page visuals and copy. r?johannh

Superfluous `>` character at the front of this line.

Comment 47

[:prathiksha]

Comment on attachment 8906315 [details]
Bug 1363051 - Update about:blocked page visuals and copy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178046/diff/7-8/

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/716be9d0b6e0

Comment 49

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This patch caused bustage of the Firefox UI safe browsing tests:
https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=firefox%20ui%20remote&bugfiler

Francois, can you please coordinate a fix? Thanks.

Comment 50

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I filed bug 1400992 to track this regression.

Comment 51

[Johann Hofmann [:johannh]]

Filed bug 1401137 to track the SUMO link.

Comment 52

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/716be9d0b6e0

Comment 53

[Valentina Claudia Ona]

Attachment: a (2).bmp

Hi Prathiksha
Can you tell me which of the results supposed to be the correct one?
I attached screenshot with every links I opened and the context it's a bit different.

Comment 54

[Valentina Claudia Ona]

Attachment: b.bmp

I want to mention that I used the latest version of Nightly 57.0a1
Build ID 20170920220431
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 55

[Valentina Claudia Ona]

Attachment: c.bmp

Comment 56

[Valentina Claudia Ona]

Attachment: d.bmp

Comment 57

[Valentina Claudia Ona]

Attachment: new.bmp

Comment 58

[:prathiksha]

Please don't use about:blocked for anything. It is left broken intentionally. :)

You could use the links provided in http://testsafebrowsing.appspot.com/ to verify all four types of blocked pages.

You can find the specs here > https://bugzilla.mozilla.org/show_bug.cgi?id=1358536#c25

Comment 59

[:prathiksha]

See also: https://github.com/mozilla/itisatrap/tree/master/firefox

Comment 60

[Valentina Claudia Ona]

I verified the links and here in specs I have an extra checkbox with "Report errors like this to help Google identify and block malicious sites", but if I open the links  

http://testsafebrowsing.appspot.com/s/phishing.html
http://testsafebrowsing.appspot.com/s/unwanted.html
http://testsafebrowsing.appspot.com/s/malware.html

I don't have than checkbox with "Report errors like this to help Google identify and block malicious sites". So I want to tell me if it's correct or not? or the page will automatically report the error without necessity to check the checkbox?

Comment 61

[:prathiksha]

No, we still have to add that checkbox and we'll be adding it in a separate bug. So please ignore the checkbox when you verify this bug. :)

Comment 62

[:prathiksha]

Francois, do we have the backend code for the report error checkbox in place? Should I file a bug to add that checkbox to the about:blocked UI?

Comment 63

[François Marier [:francois]]

(In reply to :prathiksha from comment #62)
> Francois, do we have the backend code for the report error checkbox in
> place?

Not yet, but it's being worked on in bug 1351147.

> Should I file a bug to add that checkbox to the about:blocked UI?

We have bug 1385156 for that.

Comment 64

[Valentina Claudia Ona]

I was wondering if will be ok, from your point of view, to mark this issue as verified fixed based on the fact that the check box issue is handled in bug 1385156 and also based on comment 60. Please tell me your toughs about this. Thanks

Comment 65

[François Marier [:francois]]

(In reply to Valentina Claudia Ona from comment #64)
> I was wondering if will be ok, from your point of view, to mark this issue
> as verified fixed based on the fact that the check box issue is handled in
> bug 1385156 and also based on comment 60. Please tell me your toughs about
> this. Thanks

Absolutely. You've tested all of the things we've chosen to implement at this time.

Comment 66

[Hector Zhao [:hectorz]]

(In reply to François Marier [:francois] from comment #3)
> (In reply to :prathiksha from comment #2)
> > What links are we going to provide for "reported as a deceptive site" and
> > "report a detection problem" here(https://cl.ly/133s093C2A3x) ? :)
> 
> The URLs are in https://bugzilla.mozilla.org/show_bug.cgi?id=1358536#c13:
> 
> -
> https://www.google.com/transparencyreport/safebrowsing/diagnostic/index.
> html?#url=http://testsafebrowsing.appspot.com/s/malware.html (phishing,
> malware and unwanted)
> - https://safebrowsing.google.com/safebrowsing/report_error/?tpl=mozilla
> (phishing)
> - https://www.stopbadware.org/firefox (malware)
> 
> Note:
> 
> - reporting page is different for malware and phishing
> - there is no reporting page for unwanted
> 
> Also, the "reported as XXXX" link includes the URL of the bad page as a
> query string param.

Sorry I didn't notice this earlier, but shouldn't we use the per-provider reportMalwareMistakeURL/reportPhishMistakeURL here?