982347 - Implement new visual style for network error pages

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Valentin Tsatskin [:vt]]

Update existing network error pages over to new proposed visual style defined in bug 966115. These would be all errors currently using the netError.xhtml template. No behaviours or interactions should be change (i.e. the removal of "try again" or adding the search bar should not be in this bug).

This will involve partially using the changes in the patches submitted to bug 966107.

Comment 1

[Marco Castelluccio [:marco]]

Valentin, as you've just worked on the design for the Firefox error pages, could you take a look at bug 769842?
We'd like to improve the error messages for web apps in the webapp runtime, your input would be really useful.

Comment 2

[Valentin Tsatskin [:vt]]

Attachment: newNetError

First attempt at implementing the visual style. Has media queries for small width viewports such as phones or tiny iframes.

Comment 3

[Valentin Tsatskin [:vt]]

Attachment: default screenshot.png

Comment 4

[Valentin Tsatskin [:vt]]

Attachment: condensed screenshot.png

Comment 5

[Valentin Tsatskin [:vt]]

Attachment: rtl screenshot.png

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8390290 [details] [diff] [review]
newNetError

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

Overall it seems fine to me, but you should note that I'm not a Firefox peer, so my r+ means essentially nothing.  ;)

::: browser/base/content/aboutneterror/netError.js
@@ +84,5 @@
> +  }
> +
> +  var title = document.getElementById("errorTitleText");
> +  if (title) {
> +    title.parentNode.replaceChild(errTitle, title);

Instead of replacing the title (and longDesc), what if we just had all the titles as "display: none" in the errorTitleText and changed the class of the appropriate one to "shown" or "selected"?

Comment 7

[Valentin Tsatskin [:vt]]

(In reply to Blake Winton (:bwinton) from comment #6)
> Instead of replacing the title (and longDesc), what if we just had all the
> titles as "display: none" in the errorTitleText and changed the class of the
> appropriate one to "shown" or "selected"?

That's actually a really great suggestion, and one that I had when initially working on this code. There were some related bugs as to why things work the way they do currently, and I'll have to dig into those to figure out the rationale for the current behaviour. I'll get back to you when I understand more about why things are the way they currently are.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8390290 [details] [diff] [review]
newNetError

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

This is what it looks like on my Win8.1 machine, http://screencast.com/t/Ysdwv9SF

Were you able to copy most of netError.js or is it all new?

::: browser/base/content/aboutneterror/netError.css
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +html {
> +  background: #F2F2F2;

Styling like this needs to go in a /theme/ CSS file, not in /base/. There is a /browser/themes/shared/ directory that you can place these files in.

Structural CSS can remain in /browser/base/, but stylistic CSS should move to /browser/themes/.

@@ +9,5 @@
> +body {
> +  margin: 0;
> +  padding: 48px;
> +  box-sizing: padding-box;
> +  font-family: Clear Sans;

This font isn't available on my Windows8 setup. Am I missing a patch here?

@@ +21,5 @@
> +}
> +
> +h1 {
> +  margin: 0;
> +  font-family: Fira Sans;

This font is also missing for me.

@@ +49,5 @@
> +}
> +
> +#errorTitle:-moz-dir(rtl) {
> +  background-position: right 0;
> +}

I think you meant, :-moz-locale-dir(rtl), and this should be placed *after* the #errorTitle rules or else it is going to get overwritten.

@@ +80,5 @@
> +  color: #737980;
> +}
> +
> +button:hover {
> +  background-image: -moz-linear-gradient(-90deg, #FFFFFF 0%, rgba(255,255,255,0.60) 100%);

Please use the unprefixed versions linear-gradient, note that the syntax may be a little different.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8390290 [details] [diff] [review]
newNetError

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

Awaiting new patch.

Comment 10

[Valentin Tsatskin [:vt]]

Attachment: newNetError v2

I've updated the patch addressing Jaw's feedback.

I've moved common theme elements to browser/themes/in-content/common.js. I've copied over the styles declared in browser/themes/in-content/preferences.css as a starting base for a unified stylesheet for all our in-content pages.

Fonts have been added to browser/content/fonts/ and should serve as a central place for all our fonts. A new bug should be created to consolidate all fonts to live in this location.

There's currently a security issue with including the fonts on the neterror pages that doesn't occur in other in-content pages.

> downloadable font: download not allowed (font-family: "Fira Sans" style:normal weight:300 stretch:normal src index:1): bad URI or cross-site access not allowed
> source: chrome://browser/content/fonts/FiraSans-Light.woff

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8400129 [details] [diff] [review]
newNetError v2

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

Here's a small early review. You should remove the blank lines where I mentioned. Also, I'm not sure, but I think we mostly use 2 spaces indenting across our files. But that's not important.

::: browser/base/content/aboutneterror/netError.css
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +body {
> +    display: flex;
> +

Please scrap out the blank line here

@@ +7,5 @@
> +
> +    box-sizing: padding-box;
> +    min-height: 100vh;
> +    padding: 48px;
> +

Same here

@@ +15,5 @@
> +
> +ul, ol {
> +    margin: 0;
> +    padding: 0;
> +

And here

@@ +35,5 @@
> +
> +#errorTitle {
> +    background: url('info.svg') left 0 no-repeat;
> +    background-size: 3em 3em;
> +

And here.

@@ +55,5 @@
> +
> +@media (max-width: 675px) {
> +    body {
> +        padding: 50px;
> +

Here too

@@ +61,5 @@
> +    }
> +
> +    #errorTitle {
> +        padding-top: 0;
> +

Here too

@@ +63,5 @@
> +    #errorTitle {
> +        padding-top: 0;
> +
> +        background-image: none;
> +

Same thing here

@@ +74,5 @@
> +/* common.css overrides */
> +
> +button {
> +  font-size: 1em;
> +

Same thing here too

::: browser/themes/shared/in-content/common.css
@@ +30,5 @@
> +body {
> +    font-family: Clear Sans;
> +    font-size: 15px;
> +    font-weight: normal;
> +

Extra blank line here

@@ +32,5 @@
> +    font-size: 15px;
> +    font-weight: normal;
> +
> +    margin: 0;
> +

Same here

@@ +43,5 @@
> +    font-family: Fira Sans;
> +    font-size: 2.5em;
> +    font-weight: lighter;
> +    line-height: 1.2;
> +

Same here

@@ +51,5 @@
> +
> +button,
> +menulist {
> +    line-height: 20px;
> +

Here too

@@ +54,5 @@
> +    line-height: 20px;
> +
> +    height: 30px;
> +    max-height: 30px;
> +

Here too

@@ +62,5 @@
> +    background-color: #f1f1f1;
> +    background-image: linear-gradient(#fff, rgba(255,255,255,.1));
> +    box-shadow: 0 1px 1px 0 #fff, inset 0 2px 2px 0 #fff;
> +    text-shadow: 0 1px 1px #fefffe;
> +

Here too

Comment 12

[Tim Nguyen :ntim]

Oh, it's already 2 spaces indented. Sorry. Just an illusion Bugzilla gave me while displaying the patch.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Tim Nguyen [:ntim] from comment #12)
> Oh, it's already 2 spaces indented. Sorry. Just an illusion Bugzilla gave me
> while displaying the patch.

That's not an illusion, there are places within the patch that are using 4-space indentation.

Comment 14

[Valentin Tsatskin [:vt]]

I forgot to include in my comments concerning the spacing and other style matters. I was following the webdev style guide we have here at Mozilla as a basis this patch. The rationale here was that since we don't have a style guide set in place (that I'm aware of), I might as well follow one set out in different parts of the organization.

Webdev CSS style guide: http://mozweb.readthedocs.org/en/latest/css-style.html

Comment 15

[Tim Nguyen :ntim]

(In reply to Valentin Tsatskin [:vt] from comment #14)
> I forgot to include in my comments concerning the spacing and other style
> matters. I was following the webdev style guide we have here at Mozilla as a
> basis this patch. The rationale here was that since we don't have a style
> guide set in place (that I'm aware of), I might as well follow one set out
> in different parts of the organization.
> 
> Webdev CSS style guide:
> http://mozweb.readthedocs.org/en/latest/css-style.html

I don't think the style guide actually applies for Firefox developing (maybe for other web mozilla projects). I haven't seen 4 space indenting or line breaks between rules.

Comment 16

[Tim Nguyen :ntim]

(In reply to Valentin Tsatskin [:vt] from comment #14)
> I forgot to include in my comments concerning the spacing and other style
> matters. I was following the webdev style guide we have here at Mozilla as a
> basis this patch. The rationale here was that since we don't have a style
> guide set in place (that I'm aware of), I might as well follow one set out
> in different parts of the organization.
> 
> Webdev CSS style guide:
> http://mozweb.readthedocs.org/en/latest/css-style.html

Yep, I confirm, there's only one mention of "Mercurial" or hg in the wiki. And mainly mentions about github. So it must be for Firefox OS/Together JS/or any other Web project.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8400129 [details] [diff] [review]
newNetError v2

drive-by comments:
- netError.css belongs in themes/, not in content/.
- indentation should be 2 spaces, not 4.
- how is browser/themes/shared/in-content/common.css different from toolkit/themes/*/global/inContentUI.css? Can't really tell by the name.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8400129 [details] [diff] [review]
newNetError v2

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

This won't be able to load the fonts because this page runs under content privileges (most other in-content pages run with chrome privileges). The problem with that is it means you're subject to cross-origin policies (CORS). And the problem with that is that I have no idea how (or even if) this is fixable :( Especially considering chrome:/about: URIs don't technically have a domain, and therefore don't really have an origin to compare to. 

I've tried various things in some small hope something would work (trying to trick it into thinking it's the same origin), with no luck. So, unless someone else can come up with a fix for that, I can think of three alternate solutions:
* Don't use these fonts, just use the system standard fonts for now.
* Use data: URLs.  I suspect this would have a noticeably negative impact on memory usage, however.
* Investigate making them available as though they were installed at the system level (while still only bundled with the installation of Firefox). You'd need to ask someone like John Daggett (nattokirai) if/how this would be possible.

Other general notes:
* In a small sized window, the content is forced to the top of the page. It should still be centered vertically.
* The entire mozilla-central tree uses 2-space indenting. You're never going to change that. Give up now. REPENT!

::: browser/base/content/aboutneterror/netError.css
@@ +54,5 @@
> +}
> +
> +@media (max-width: 675px) {
> +    body {
> +        padding: 50px;

Er, why a change of 2px?

::: browser/base/content/aboutneterror/netError.xhtml
@@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8"?>

Aside from the stylesheet locations and moving the JS out to it's own file, did you change anything else when you copied this file/the JS?

@@ +22,5 @@
> +    <title>&loadError.label;</title>
> +
> +    <!-- If the location of the favicon is changed here, the FAVICON_ERRORPAGE_URL symbol in
> +         toolkit/components/places/src/nsFaviconService.h should be updated. -->
> +    <link rel="icon" type="image/svg" id="favicon" href="chrome://browser/content/aboutneterror/info.svg"/>

Hm, we need a solution for this. The reason for the warning is that this can result in this icon being kept as the favicon for the site, unless the favicon service explicitly special-cases it. Sadly, no one counted on wanting to change the icon for some pages but not others...

Something like the following should work:
* Update FAVICON_ERRORPAGE_URL in nsFaviconService.h to point to a generic URI used *only* for error pages, something like chrome://global/skin/places/aboutNetErrorFavicon.png
* Update every usage that mentions FAVICON_ERRORPAGE_URL to use that URI
* Copy the existing icon used by about:neterror to where that URI resolves
* Add an override directive to a jar.mn file for our new neterror page here, to override the old icon with our new shiny one

@@ +23,5 @@
> +
> +    <!-- If the location of the favicon is changed here, the FAVICON_ERRORPAGE_URL symbol in
> +         toolkit/components/places/src/nsFaviconService.h should be updated. -->
> +    <link rel="icon" type="image/svg" id="favicon" href="chrome://browser/content/aboutneterror/info.svg"/>
> +    <link rel="stylesheet" href="chrome://browser/skin/in-content/common.css" type="text/css" media="all" />

@import this, as is done with inContentUI.css

::: browser/themes/shared/jar.mn
@@ +2,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +browser.jar:
> +  skin/classic/browser/in-content/common.css  (in-content/common.css)

Alas, this won't work - it breaks on Windows when the aero variant of the theme is used. You need to add this to each of the theme's jar.mn files, like aboutWelcomeback.css is done:
http://hg.mozilla.org/mozilla-central/file/4941a2ac0786/browser/themes/windows/jar.mn#l14

Remember to add it to the aero section for windows too, and avoid a mishap like bug 990979 (found that bug when looking at this).

@@ +6,5 @@
> +  skin/classic/browser/in-content/common.css  (in-content/common.css)
> +
> +  skin/classic/browser/fonts/ClearSans-Regular.woff      (fonts/ClearSans-Regular.woff)
> +  skin/classic/browser/fonts/FiraSans-Regular.woff       (fonts/FiraSans-Regular.woff)
> +  skin/classic/browser/fonts/FiraSans-Light.woff         (fonts/FiraSans-Light.woff)

Content is the right place for these. Don't need them twice.

Comment 19

[Valentin Tsatskin [:vt]]

Attachment: javascript.diff

This is a diff comparing the original netError.xhtml inline js with the new external js. Here are a summary of the changes I made to the script:

- Formatting fixes
  - Function curly braces on same line
  - expanded on some variable names for readability
- Removed code paths that do not get executed anymore
  - SSL error code has been removed since these are handled by about:certerror
- Added a DOMContentLoaded event listener to init page
- Attempted to fix favicon loading issue by injecting a favicon into the DOM
  - This was accidentally left in patch since it doesn't actually work. Will remove on next iteration.
- Moved code which auto-focuses retry button into its own function

Comment 20

[Valentin Tsatskin [:vt]]

(In reply to Dão Gottwald [:dao] from comment #17)
> Comment on attachment 8400129 [details] [diff] [review]
> newNetError v2
> 
> drive-by comments:
> - netError.css belongs in themes/, not in content/.

What's your rationale for that?

I was intending to follow the pattern established by the other about pages located in the content directory. These about pages contain mostly everything they need in one folder, such as about:accounts, about:healthreport, about:home, about:newtab. It seems my approach is more consistent with the existing way things are done with about pages.

Putting netError.css in themes will break that logical organization. In addition, multiple manifest entries will need to be created and maintained if we stick that file in themes.

> - indentation should be 2 spaces, not 4.

That will be fixed in the coming patch, thanks for pointing this out.

> - how is browser/themes/shared/in-content/common.css different from
> toolkit/themes/*/global/inContentUI.css? Can't really tell by the name.

From my inspection of where and how inContentUI.css is used, at first it may seem like they are very similar. In fact they serve the same purpose, to unify the in-content styles.

However, inContentUI is using a visual style that we are starting to move away from, in favour of what can be seen in about:preferences (see project Chameleon or talk to shorlander). So it made sense to start a new file with the new visual style. Once we start building up this stylesheet with more components and having more about pages use it, will phase out the older inContentUI.css. I don't have the entire strategy fleshed out just yet, but for the time being this file will be the starting ground for this work.

Comment 21

[Valentin Tsatskin [:vt]]

Attachment: newNetError v3

Summary of changes:

- Changed css whitespace to 2 spaces
- Vertically cenetered error page on small viewports
- Import fonts with data uri in new css file embeddedFonts.css
- Removed fonts from theme folders
- Removed non-working favicon fix code
- Imported additional stylesheets
- Moved common.css to system theme manifests
  - Removed build files in shared directory

The favicon issue Unfocused mentioned has not been resolved in this patch.

Comment 22

[John Daggett (:jtd)]

(In reply to Blair McBride [:Unfocused] from comment #18)
> Comment on attachment 8400129 [details] [diff] [review]
> newNetError v2
> 
> Review of attachment 8400129 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This won't be able to load the fonts because this page runs under content
> privileges (most other in-content pages run with chrome privileges). The
> problem with that is it means you're subject to cross-origin policies
> (CORS). And the problem with that is that I have no idea how (or even if)
> this is fixable :( Especially considering chrome:/about: URIs don't
> technically have a domain, and therefore don't really have an origin to
> compare to. 
> 
> I've tried various things in some small hope something would work (trying to
> trick it into thinking it's the same origin), with no luck. So, unless
> someone else can come up with a fix for that, I can think of three alternate
> solutions:
> * Don't use these fonts, just use the system standard fonts for now.
> * Use data: URLs.  I suspect this would have a noticeably negative impact on
> memory usage, however.
> * Investigate making them available as though they were installed at the
> system level (while still only bundled with the installation of Firefox).
> You'd need to ask someone like John Daggett (nattokirai) if/how this would
> be possible.
> 
> Other general notes:
> * In a small sized window, the content is forced to the top of the page. It
> should still be centered vertically.
> * The entire mozilla-central tree uses 2-space indenting. You're never going
> to change that. Give up now. REPENT!
> 
> ::: browser/base/content/aboutneterror/netError.css
> @@ +54,5 @@
> > +}
> > +
> > +@media (max-width: 675px) {
> > +    body {
> > +        padding: 50px;
> 
> Er, why a change of 2px?
> 
> ::: browser/base/content/aboutneterror/netError.xhtml
> @@ +1,1 @@
> > +<?xml version="1.0" encoding="UTF-8"?>
> 
> Aside from the stylesheet locations and moving the JS out to it's own file,
> did you change anything else when you copied this file/the JS?
> 
> @@ +22,5 @@
> > +    <title>&loadError.label;</title>
> > +
> > +    <!-- If the location of the favicon is changed here, the FAVICON_ERRORPAGE_URL symbol in
> > +         toolkit/components/places/src/nsFaviconService.h should be updated. -->
> > +    <link rel="icon" type="image/svg" id="favicon" href="chrome://browser/content/aboutneterror/info.svg"/>
> 
> Hm, we need a solution for this. The reason for the warning is that this can
> result in this icon being kept as the favicon for the site, unless the
> favicon service explicitly special-cases it. Sadly, no one counted on
> wanting to change the icon for some pages but not others...
> 
> Something like the following should work:
> * Update FAVICON_ERRORPAGE_URL in nsFaviconService.h to point to a generic
> URI used *only* for error pages, something like
> chrome://global/skin/places/aboutNetErrorFavicon.png
> * Update every usage that mentions FAVICON_ERRORPAGE_URL to use that URI
> * Copy the existing icon used by about:neterror to where that URI resolves
> * Add an override directive to a jar.mn file for our new neterror page here,
> to override the old icon with our new shiny one
> 
> @@ +23,5 @@
> > +
> > +    <!-- If the location of the favicon is changed here, the FAVICON_ERRORPAGE_URL symbol in
> > +         toolkit/components/places/src/nsFaviconService.h should be updated. -->
> > +    <link rel="icon" type="image/svg" id="favicon" href="chrome://browser/content/aboutneterror/info.svg"/>
> > +    <link rel="stylesheet" href="chrome://browser/skin/in-content/common.css" type="text/css" media="all" />
> 
> @import this, as is done with inContentUI.css
> 
> ::: browser/themes/shared/jar.mn
> @@ +2,5 @@
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +browser.jar:
> > +  skin/classic/browser/in-content/common.css  (in-content/common.css)
> 
> Alas, this won't work - it breaks on Windows when the aero variant of the
> theme is used. You need to add this to each of the theme's jar.mn files,
> like aboutWelcomeback.css is done:
> http://hg.mozilla.org/mozilla-central/file/4941a2ac0786/browser/themes/
> windows/jar.mn#l14
> 
> Remember to add it to the aero section for windows too, and avoid a mishap
> like bug 990979 (found that bug when looking at this).
> 
> @@ +6,5 @@
> > +  skin/classic/browser/in-content/common.css  (in-content/common.css)
> > +
> > +  skin/classic/browser/fonts/ClearSans-Regular.woff      (fonts/ClearSans-Regular.woff)
> > +  skin/classic/browser/fonts/FiraSans-Regular.woff       (fonts/FiraSans-Regular.woff)
> > +  skin/classic/browser/fonts/FiraSans-Light.woff         (fonts/FiraSans-Light.woff)
> 
> Content is the right place for these. Don't need them twice.

Comment 23

[John Daggett (:jtd)]

(In reply to Blair McBride [:Unfocused] from comment #18)

*argh* sorry for the accidental comment...

> * Don't use these fonts, just use the system standard fonts for now.
> * Use data: URLs.  I suspect this would have a noticeably negative impact on
> memory usage, however.

One consideration to keep in mind is that these fonts will only work
for the scripts supported by the included fonts. That's definitely a
subset of the localizations we currently support.

I think you only want to bundle fonts if you intend on using them
widely through the UI and in a way that really is common enough that
it justifies the added bulk of the install package.  Using data urls
for fonts like in the patch will definitely bloat the install package.

Using downloadable fonts here is not ideal I think. We cache
downloaded font activations but there are still scenarios under which
you'd end up activating and deactivating the same font multiple times
across the life of a browser run. As Blair mentions, there's also the
additional memory used for the activated font.

> * Investigate making them available as though they were installed at
> the system level (while still only bundled with the installation of
> Firefox). You'd need to ask someone like John Daggett (nattokirai)
> if/how this would be possible.

Yes, this is definitely doable. I seem to remember Asa pooh-poohing
this years back but it's much more efficient from a packaging and
usage perspective. We've discussed this in the past with regards to
load-on-demand font downloading for minority scripts.

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Valentin Tsatskin [:vt] from comment #20)
> (In reply to Dão Gottwald [:dao] from comment #17)
> > Comment on attachment 8400129 [details] [diff] [review]
> > newNetError v2
> > 
> > drive-by comments:
> > - netError.css belongs in themes/, not in content/.
> 
> What's your rationale for that?
> 
> I was intending to follow the pattern established by the other about pages
> located in the content directory. These about pages contain mostly
> everything they need in one folder, such as about:accounts,
> about:healthreport, about:home, about:newtab. It seems my approach is more
> consistent with the existing way things are done with about pages.
> 
> Putting netError.css in themes will break that logical organization. In
> addition, multiple manifest entries will need to be created and maintained
> if we stick that file in themes.

Yea, I think these specific pages (*not* all in-content about: pages!) are the one exception to the otherwise tree-wide (and strictly enforced) rule of never putting styling-related rules in /content/ CSS files. So Dao was right to question it. If the goal here is for this page to be completely platform agnostic (just like a web page), then /content/ would be the right place.

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Valentin Tsatskin [:vt] from comment #21)
> The favicon issue Unfocused mentioned has not been resolved in this patch.

You mentioned to me on IRC that eventually we'd have different favicons for this page, for different error messages. Which rather kills my solution proposed in comment 18 of overriding a generic URL that nsFaviconService knows about.

So here's an alternate proposal:

* Register a faviconservice-ignore entry with the category manager, in a chrome manifest file. Kinda like this: http://dxr.mozilla.org/mozilla-central/source/browser/experiments/Experiments.manifest#6
* The first time FAVICON_ERRORPAGE_URL would be used in the current code, get all faviconservice-ignore category entries, cache them, and use them like FAVICON_ERRORPAGE_URL was used (*really* don't want to be going through the category manager for every favicon query)
* Will need to respond to some observer notification to force a refresh of the cache of the category entries - needed for tests (nice bonus: useful for restartless add-ons)

You'll want to run that idea past Marco Bonardo (mak) first.

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8401594 [details] [diff] [review]
newNetError v3

(Assuming you meant to flag me for this. Will get to it when I can.)

Comment 27

[Jonathan Kew [:jfkthame]]

Comment on attachment 8401594 [details] [diff] [review]
newNetError v3

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

Drive-by comment: ISTM that adding "embedded" fonts to the product for this purpose is questionable. Why don't network error pages use the system's default UI or content font, like everything else? I think this adds unnecessary bulk for no real gain.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jonathan Kew (:jfkthame) from comment #27)
> Comment on attachment 8401594 [details] [diff] [review]
> newNetError v3
> 
> Review of attachment 8401594 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive-by comment: ISTM that adding "embedded" fonts to the product for this
> purpose is questionable. Why don't network error pages use the system's
> default UI or content font, like everything else? I think this adds
> unnecessary bulk for no real gain.

This is not just for network error pages, but will be used for all in-content pages. This is part of a larger goal of unifying the visual identity of the browser across platforms.

Using a cross platform web-safe font is less than ideal because they are not considered to be pretty by today's design standards. We can't use downloadable fonts due to network, privacy, and performance constraints. The platform specific fonts such as Segoe UI and Helvetica are not available on their competitors platforms.

Comment 29

[Jonathan Kew [:jfkthame]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #28)
> (In reply to Jonathan Kew (:jfkthame) from comment #27)
> > Comment on attachment 8401594 [details] [diff] [review]
> > newNetError v3
> > 
> > Review of attachment 8401594 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Drive-by comment: ISTM that adding "embedded" fonts to the product for this
> > purpose is questionable. Why don't network error pages use the system's
> > default UI or content font, like everything else? I think this adds
> > unnecessary bulk for no real gain.
> 
> This is not just for network error pages, but will be used for all
> in-content pages. This is part of a larger goal of unifying the visual
> identity of the browser across platforms.
> 
> Using a cross platform web-safe font is less than ideal because they are not
> considered to be pretty by today's design standards. We can't use
> downloadable fonts due to network, privacy, and performance constraints. The
> platform specific fonts such as Segoe UI and Helvetica are not available on
> their competitors platforms.

Surely either the page should be treated similarly to web content, in which case using font-family:sans-serif will get you a platform-appropriate default; or else it should be treated similarly to browser UI, in which case it would use Segoe UI on Windows, Lucida Grande on OS X, etc.

(I don't think "unifying the visual identity of the browser across platforms" is entirely compatible with another goal that I think should be a pretty high priority: conforming to the normal look and feel of the platform.)

Comment 30

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Blair McBride [:Unfocused] from comment #25)
> (In reply to Valentin Tsatskin [:vt] from comment #21)
> * Register a faviconservice-ignore entry with the category manager, in a
> chrome manifest file. 

Great idea Blair, and likely what we should do in the long run.  For this small step, though, using the generic nsFaviconService URLs should be sufficient - even though it doesn't mean a different favicon on each page.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #28)
> (In reply to Jonathan Kew (:jfkthame) from comment #27)
> > Comment on attachment 8401594 [details] [diff] [review]
> Using a cross platform web-safe font is less than ideal because they are not
> considered to be pretty by today's design standards. We can't use
> downloadable fonts due to network, privacy, and performance constraints. The
> platform specific fonts such as Segoe UI and Helvetica are not available on
> their competitors platforms.

True.  And we do want these pages to be both beautiful and cross-platform as a larger design goal.  That's why these pages are similar to the look of about:preferences, both of which are part of the larger Project Chameleon design effort.  These pages should be styled exactly like the font of about:preferences, (font-family: "Clear Sans"; src: url("chrome://browser/skin/fonts/ClearSans-Regular.ttf");

Comment 31

[Matthew N. [:MattN]]

Comment on attachment 8401594 [details] [diff] [review]
newNetError v3

I still think it's sad that we're forking so much of this when a lot of the improvements apply to the docshell version.

Any files which are based on another (e.g docshell) version should be done as a |hg copy| in this patch so it's clear what is actually changing and to preserve the history.

Comment 32

[Valentin Tsatskin [:vt]]

Attachment: newNetError.wip.patch

Changes from v2:

- Changed css whitespace to 2 spaces
- Vertically cenetered error page on small viewports
- Import fonts with data uri in new css file embeddedFonts.css
- Removed fonts from theme folders
- Removed non-working favicon fix code
- Imported additional stylesheets
- Moved common.css to system theme manifests
  - Removed build files in shared directory

Changes from v3:

I've chosen to use the old warning icon for the favicon. Although this is contradicting the stated design, I've opted to hold off on the work until a more generalized solution can be created for all about pages. This work should be part of bug 988959 (or a dependant bug).

Comment 33

[Jonathan Kew [:jfkthame]]

Comment on attachment 8403653 [details] [diff] [review]
newNetError.wip.patch

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

I still think the use of product-embedded fonts here is questionable; we should be using standard platform fonts to conform to platform look-and-feel.

However, if that's a firm decision, I have a couple of further concerns:
- This patch includes the fonts as separate .woff files *and* as base64-encoded data urls in a CSS file. That's even more bloat.
- Why does this call for both Fira Sans *and* Clear Sans? Pick ONE sans family and stick with it. If a visible contrast is actually wanted, you need something much more distinctive than Fira vs Clear.

Comment 34

[John Daggett (:jtd)]

The more I think about this the more I agree with Jonathan. I think it's much better to use the existing platform fonts. Adding Fira Sans to the install package is possible but for the purpose of having nice error pages it's not worth the effort.  Especially on WinXP you really want to use one of the core web fonts as they have been specially hinted for Cleartype GDI.  Under both OSX and Windows there are a number of fine choices for fonts that would be appropriate for use in things like error pages.

Comment 35

[Tim Nguyen :ntim]

(In reply to John Daggett (:jtd) from comment #34)
> The more I think about this the more I agree with Jonathan. I think it's
> much better to use the existing platform fonts. Adding Fira Sans to the
> install package is possible but for the purpose of having nice error pages
> it's not worth the effort.  Especially on WinXP you really want to use one
> of the core web fonts as they have been specially hinted for Cleartype GDI. 
> Under both OSX and Windows there are a number of fine choices for fonts that
> would be appropriate for use in things like error pages.

I had a discussion with UX, and the goal is to unify looks across Firefox's platform. Which also includes typography.

Comment 36

[John Daggett (:jtd)]

(In reply to Tim Nguyen [:ntim] (Limited internet 14-24 April) from comment #35)
> I had a discussion with UX, and the goal is to unify looks across Firefox's
> platform. Which also includes typography.

All features, including UX ones, need to be balanced with the costs needed to support them.  In this case, packaging fonts with desktop downloads (1) increases the size of the install package and (2) increases the memory footprint of the Firefox application.  Additionally, "unify looks" is not what will happen here since the "look" on OSX/Linux/DirectWrite will look one way while the look under Windows GDI will *not* look the same.  In fact, it will probably look significantly poorer than default platform fonts, unless the fonts have been designed with GDI in mind.  I suspect that's not the case.

In short, whether to do this or not is not simply a UX decision I think, it's whether the resource costs justify the gains in platform consistency.  Seems like you should be thinking about how to achieve *approximate* consistency in your design while minimizing resource consumption.

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to John Daggett (:jtd) from comment #36)

Let's fork this conversation about fonts to a firefox-dev or a dev-platform thread and make sure that the right people (yourself, jfkthame, UX, and Firefox peers/module owners) are included. This isn't the right forum for the back and forth, and it shouldn't hold up the rest of the work in this bug.

Comment 38

[Jennifer Morrow [:Boriss] (UX)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #37)
> (In reply to John Daggett (:jtd) from comment #36)
> 
> Let's fork this conversation about fonts to a firefox-dev or a dev-platform
> thread and make sure that the right people (yourself, jfkthame, UX, and
> Firefox peers/module owners) are included. This isn't the right forum for
> the back and forth, and it shouldn't hold up the rest of the work in this
> bug.

Absolutely agreed.  For this bug, we should treat text and layout identically to about:preferences which, as Jonathan points out in  Comment 29, is treating the page more similarly to Firefox UI than to web content.

Comment 39

[Madhava Enros [:madhava]]

It sounds to me like the proposed next step is to
(a) defer the fonts (what to use, how to do it) to a followup bug, and 
(b) get the bulk of the benefit of this landed by using system fonts

Can we create that follow-up and land the new pages otherwise? Valentin - can we see some screenshots of the pages with system fonts?

Comment 40

[Valentin Tsatskin [:vt]]

Attachment: newNetError.wip.patch

Replaced the fonts with system fonts. Screenshots to follow.

Comment 41

[Valentin Tsatskin [:vt]]

Attachment: net.error.system.fonts.mac.png

Comment 42

[Valentin Tsatskin [:vt]]

Attachment: net.error.system.fonts.linux.png

Comment 43

[Valentin Tsatskin [:vt]]

Attachment: net.error.system.fonts.win.png

Comment 44

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8414011 [details] [diff] [review]
newNetError.wip.patch

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

So, I've tried reviewing the JS file... and I agree with Matt in comment 31. There aren't that many changes needed to support this specific bug any more, now the scope has been reduced. But it's still extremely hard to figure out exactly what of significance has changed. While I do agree that the copy of netError.xhtml in docshell is feeling rather old and crusty (I've tried cleaning it up myself), I think the better path here is to do that in discrete steps, so we're clear about what's going on.

Eventually we need to figure out a way to get back to only having one copy of the neterror page in the tree, with different applications only overriding parts they need to. I think we can achieve that by cleaning it up, and modularising it into files that apps can override.

But while we have multiple copies, if we land a change in the docshell version, this patch makes it needlessly difficult to see if/how to apply that to the Firefox Desktop copy.

So:
* Keep your JS changes around in a patch somewhere, as we can use that as a basis for cleaning up/splitting up the docshell file
* |hg copy| the docshell file into /browser/
* Should only need minimal changes to neterror.xhtml
* That reduces this bug back down to what it's original intention was - just implementing the new style
* File a new bug to sort out the multiple copies of neterror.xhtml problem? (No need for it to be on your list of priorities, but we should have a bug for it)

::: browser/base/content/aboutneterror/netError.css
@@ +8,5 @@
> +  display: flex;
> +
> +  box-sizing: padding-box;
> +  min-height: 100vh;
> +  padding: 48px;

If you resize the window to be vertically quite short, you notice that it feels like it starts scrolling prematurely (ie, it starts scrolling but there's nothing but blank space in the hidden area at the bottom of the page) because of the bottom padding.

I also wonder about removing the top padding too, so that the content moves up the page more when the page has very little height.

@@ +35,5 @@
> +  max-width: 512px;
> +}
> +
> +#errorTitle {
> +  background: url('info.svg') left 0 no-repeat;

Hm, you've added a <div> with an id=errorIcon, but you're not using it here.

@@ +72,5 @@
> +button {
> +  font-size: 1em;
> +
> +  min-width: 150px;
> +  padding: 3px 0;

Think you need to tweak the left/right edges of the dotted focus indicator on the buttons. There's padding between it and the top/bottom borders, but it's hard up against the left/right borders. Think this padding is likely what's causing that difference - but it should probably be fixed also in common.css so buttons without this padding adjustment don't have focus indicator hard up against the borders.

::: browser/themes/shared/in-content/common.css
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +body {
> +  font: -moz-dialog;

Pro-tip: -moz-dialog is actually a color :) And, alas, it's not as simple as a single font anyway to get what looks best for the OS (with reasonable fallbacks).

On Windows you want: "Segoe UI", sans-serif
OSX: "Lucida Grande", sans-serif
Linux: sans-serif

@@ +52,5 @@
> +  background-image: linear-gradient(rgba(255,255,255,.1), rgba(255,255,255,.6));
> +}
> +
> +button:disabled {
> +    cursor: not-allowed;

This has the downside that when you click on (for instance) the "Try Again" button, the cursor will flash from the normal cursor, to the not-allowed cursor, and then back to the normal cursor. IMO, that's unwanted noise.

Comment 45

[John Daggett (:jtd)]

(In reply to Blair McBride [:Unfocused] from comment #44)
> ::: browser/themes/shared/in-content/common.css
> @@ +2,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +body {
> > +  font: -moz-dialog;
> 
> Pro-tip: -moz-dialog is actually a color :) And, alas, it's not as simple as
> a single font anyway to get what looks best for the OS (with reasonable
> fallbacks).
> 
> On Windows you want: "Segoe UI", sans-serif
> OSX: "Lucida Grande", sans-serif
> Linux: sans-serif

Hmmm, '-moz-dialog' is a perfectly valid system font name, not sure what the "it's a color" comment means.  It will use the font used for dialogs (see the nsLookAndFeel::GetFontImpl implementations). Don't know if this is appropriate for those pages or not (many other CSS rules in browser/themes use 'message-box').  I don't think you want to use explicit fonts like this if you can avoid it.  Either use a system font or just sans-serif, that will work better across localizations.

Comment 46

[Tim Nguyen :ntim]

(In reply to John Daggett (:jtd) from comment #45)
> (In reply to Blair McBride [:Unfocused] from comment #44)
> > ::: browser/themes/shared/in-content/common.css
> > @@ +2,5 @@
> > > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > > +
> > > +body {
> > > +  font: -moz-dialog;
> > 
> > Pro-tip: -moz-dialog is actually a color :) And, alas, it's not as simple as
> > a single font anyway to get what looks best for the OS (with reasonable
> > fallbacks).
> > 
> > On Windows you want: "Segoe UI", sans-serif
> > OSX: "Lucida Grande", sans-serif
> > Linux: sans-serif
> 
> Hmmm, '-moz-dialog' is a perfectly valid system font name, not sure what the
> "it's a color" comment means.  It will use the font used for dialogs (see
> the nsLookAndFeel::GetFontImpl implementations). Don't know if this is
> appropriate for those pages or not (many other CSS rules in browser/themes
> use 'message-box').  I don't think you want to use explicit fonts like this
> if you can avoid it.  Either use a system font or just sans-serif, that will
> work better across localizations.

It's not for me... I think font:menu might be more appropriate here.

Comment 47

[Tim Nguyen :ntim]

(In reply to Valentin Tsatskin [:vt] from comment #43)
> Created attachment 8414028 [details]
> net.error.system.fonts.win.png

Seems like the wrong one here. Please use font:menu instead.

Comment 48

[Tim Nguyen :ntim]

Comment on attachment 8414011 [details] [diff] [review]
newNetError.wip.patch

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

Just some small feedback about that SVG file (also 2 space indenting might be better).
Also, I don't think we need much blank lines between CSS properties (never seen that in Mozilla's source code before).

::: browser/base/content/aboutneterror/info.svg
@@ +1,3 @@
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" x="0px" y="0px" viewBox="0 0 100 100" enable-background="new 0 0 100 100" xml:space="preserve">
> +<g>
> +</g>

Can you remove this please ? It's useless.

@@ +1,5 @@
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" x="0px" y="0px" viewBox="0 0 100 100" enable-background="new 0 0 100 100" xml:space="preserve">
> +<g>
> +</g>
> +<g>
> +	<g>

Do we really need to wrap with 2 <g> elements ?
Plus, you can apply fill="#85898C" directly on the <g> element (without repeating it). Correct me if I'm wrong :)

@@ +8,5 @@
> +		<rect x="45" y="39.9" fill="#85898C" width="10.1" height="41.8"/>
> +	</g>
> +</g>
> +<g>
> +</g>

Same for this

Comment 49

[Tim Nguyen :ntim]

Are there any reasons we're not putting the theme files into the browser/themes/shared directory ? browser/base/content seems like a weird place for me.

Comment 50

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to John Daggett (:jtd) from comment #45)
> Hmmm, '-moz-dialog' is a perfectly valid system font name, not sure what the
> "it's a color" comment means.  It will use the font used for dialogs (see
> the nsLookAndFeel::GetFontImpl implementations). Don't know if this is
> appropriate for those pages or not (many other CSS rules in browser/themes
> use 'message-box').  I don't think you want to use explicit fonts like this
> if you can avoid it.  Either use a system font or just sans-serif, that will
> work better across localizations.

Hm, well, in this case it seemed to be interpreted as a color.

And those fonts I mentioned is what we explicitly use elsewhere when we want to do this. Segoe UI is the modern font to use on Windows, but you won't get that by default by just specifying sans-serif.

Comment 51

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Tim Nguyen [:ntim] from comment #49)
> Are there any reasons we're not putting the theme files into the
> browser/themes/shared directory ? browser/base/content seems like a weird
> place for me.

See comment 24. Though, (temporarily) using OS specific fonts somewhat goes against that argument.

I don't feel overly strongly about where the CSS goes for this. But it seems a lot of other people feel it should be in /themes/.

Comment 52

[Tim Nguyen :ntim]

(In reply to Blair McBride [:Unfocused] from comment #50)
> (In reply to John Daggett (:jtd) from comment #45)
> > Hmmm, '-moz-dialog' is a perfectly valid system font name, not sure what the
> > "it's a color" comment means.  It will use the font used for dialogs (see
> > the nsLookAndFeel::GetFontImpl implementations). Don't know if this is
> > appropriate for those pages or not (many other CSS rules in browser/themes
> > use 'message-box').  I don't think you want to use explicit fonts like this
> > if you can avoid it.  Either use a system font or just sans-serif, that will
> > work better across localizations.
> 
> Hm, well, in this case it seemed to be interpreted as a color.
> 
> And those fonts I mentioned is what we explicitly use elsewhere when we want
> to do this. Segoe UI is the modern font to use on Windows, but you won't get
> that by default by just specifying sans-serif.

XP uses Tahoma, so it might just be good to use font:menu as mentioned before.

Comment 53

[Jonathan Kew [:jfkthame]]

For the content of the page, ISTM that font:message-box would be more appropriate than font:menu.

On Win8 they both appear to resolve to Segoe UI, on WinXP they both give Tahoma, and on OS X they both give Lucida Grande, although in this case font:message-box results in a smaller size; but if you're subsequently overriding the size anyhow, that shouldn't matter.

So AFAICT either menu or message-box would give the desired result at this point, but message-box seems a better semantic match; and so if there's ever a theme that results in the two resolving to different faces, it would be better to use message-box here.

Comment 54

[Valentin Tsatskin [:vt]]

Attachment: newNetError.wip.patch

I'm opting to keep netError.css inside of content. The rationale here is that since we are going for platform agnostic styling, we should put it in content (see comment 24).

One reason I can think of moving netError.css to the themes directory is if we intend for a full theme to be able to theme the netError page. So do we want to allow people to theme netError pages?

Once a decision can be made I can update the patch, but I don't want this to block getting the changes in.

# Summary of changes

* netError.xhtml
  * Used |hg copy| on netError.xhtml to preserve history
  * Extra whitespace was removed from netError.xhtml	

* info.svg
  * Addressed feedback from comment 48

* netError.css
  * Removed blank lines (comment 11, comment 48)
  * Don't show a disabled cursor for retry button (comment 44)
  * Removed top and bottom padding from body

* common.css
  * Removed blank lines
  * Using message-box for font (comment 53)
  * Adding 3px padding on buttons for focus indicators (comment 44)

* Removed netError.js to simplify patch

Comment 55

[Valentin Tsatskin [:vt]]

I've created bugs for the discussions which have stemmed from this bug:
* Bug 1007412 - Sort out what we should do with multiple copies of netError.xhtml
* Bug 1007400 - Remove unused SSL code from netError.xhtml
* Bug 1007404 - Determine how to handle embedding non-system fonts

Please excuse me if I have missed any other discussion points.

Comment 56

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8419033 [details] [diff] [review]
newNetError.wip.patch

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

So close! Think we need to do something about the SVG though.

::: browser/base/content/aboutneterror/info.svg
@@ +1,1 @@
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" x="0px" y="0px" viewBox="0 0 100 100" enable-background="new 0 0 100 100" xml:space="preserve">

(Gotta admit, I was kinda avoiding reviewing the SVG...*sigh*)

Should be able to remove the xlink namespace. Ditto with the xml:space attribute.

@@ +1,4 @@
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" x="0px" y="0px" viewBox="0 0 100 100" enable-background="new 0 0 100 100" xml:space="preserve">
> +<g fill="#85898C">
> +  <path d="M50-0.1c-27.6 0-50 22.4-50 50c0 27.6 22.4 50 50 50c27.6 0 50-22.4 50-50C100 22.3 77.6-0.1 50-0.1z M50 89.2c-21.7 0-39.2-17.6-39.2-39.2S28.3 10.8 50 10.8S89.2 28.3 89.2 50S71.7 89.2 50 89.2z"/>
> +  <path d="M50 18.2c-2 0-3.6 0.6-4.8 1.9c-1.2 1.2-1.8 2.7-1.8 4.5c0 1.8 0.6 3.4 1.8 4.6C46.4 30.3 48 31 50 31 c1.9 0 3.5-0.6 4.7-1.8c1.2-1.2 1.8-2.7 1.8-4.6c0-1.8-0.6-3.3-1.8-4.5C53.6 18.8 52 18.2 50 18.2z"/>

And these paths are overkill. And since they're describing circles, they're inherently both inaccurate and inefficient. You should just use a <circle> element. I found the following mimics what you had:

	<circle cx="50" cy="50" r="44" style="stroke: #85898C; stroke-width: 11; fill: transparent;"/>
	<circle cx="50" cy="24.6" r="6.4"/>

::: browser/base/content/aboutneterror/netError.css
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +@import url(chrome://browser/skin/in-content/common.css);

Nit: Quote url (double quotes)

@@ +32,5 @@
> +  max-width: 512px;
> +}
> +
> +#errorTitle {
> +  background: url('info.svg') left 0 no-repeat;

Nit: Double quotes

Comment 57

[Tim Nguyen :ntim]

Comment on attachment 8419033 [details] [diff] [review]
newNetError.wip.patch

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

Just some extra nits

::: browser/base/content/aboutneterror/netError.css
@@ +59,5 @@
> +    -moz-margin-start: 0;
> +  }
> +}
> +
> +

Extra blank line here :)

::: browser/base/jar.mn
@@ +48,5 @@
> +
> +        content/browser/aboutneterror/netError.xhtml     (content/aboutneterror/netError.xhtml)
> +        content/browser/aboutneterror/netError.css       (content/aboutneterror/netError.css)
> +        content/browser/aboutneterror/info.svg           (content/aboutneterror/info.svg)
> +

Can you align these properly to the upper ones too ? Thanks :)

Comment 58

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #57)
> Comment on attachment 8419033 [details] [diff] [review]
> newNetError.wip.patch
> 
> Review of attachment 8419033 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

> ::: browser/base/jar.mn
> @@ +48,5 @@
> > +
> > +        content/browser/aboutneterror/netError.xhtml     (content/aboutneterror/netError.xhtml)
> > +        content/browser/aboutneterror/netError.css       (content/aboutneterror/netError.css)
> > +        content/browser/aboutneterror/info.svg           (content/aboutneterror/info.svg)
> > +
> 
> Can you align these properly to the upper ones too ? Thanks :)
Well, probably not needed. Since these are longer than the upper ones.

Comment 59

[Valentin Tsatskin [:vt]]

(In reply to Tim Nguyen [:ntim] from comment #57)
> Comment on attachment 8419033 [details] [diff] [review]
> newNetError.wip.patch
> 
> Review of attachment 8419033 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just some extra nits
> 
> ::: browser/base/content/aboutneterror/netError.css
> @@ +59,5 @@
> > +    -moz-margin-start: 0;
> > +  }
> > +}
> > +
> > +
> 
> Extra blank line here :)

I figured putting an extra line to separate sections the css file was a good idea. Makes things more scanable IMO since properties are separated by one empty line and sections are separated by two and a comment.

Is there an agreed upon convention how we separate sections in CSS files? I know webdev has something formalized: http://mozweb.readthedocs.org/en/latest/css-style.html#style-sheet-organization. An example: https://github.com/mozilla/lumbergh/blob/27c7fd38cf4a7aa638068f6d8388dd339c16ac75/careers/base/static/css/base.css#L183

Comment 60

[Valentin Tsatskin [:vt]]

Attachment: newNetError.wip.patch

Addressed SVG issues and nits.

Comment 61

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8419910 [details] [diff] [review]
newNetError.wip.patch

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

Ship it!


(In reply to Valentin Tsatskin [:vt] from comment #59)
> > Extra blank line here :)
> 
> I figured putting an extra line to separate sections the css file was a good
> idea.

This is what I've been advocating for, for awhile. Whitespace can be very useful.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/d773600f10bd

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d773600f10bd

Comment 64

[Marco Mucci [:MarcoM]]

Hi Juan, can you review this bug if it will require further QA verification.

Comment 65

[juan becerra [:juanb]]

Qa+'ing to make sure network error pages get looked at across platforms.

Comment 66

[Bogdan Maris, Desktop Test Engineering]

We verified the implementation of the new visual style for network errors pages using latest Nightly 32 2014-05-13: en-US, ar, he (rtl builds). 

We found two new issues and logged bugs on them, our testing can be found in etherpad [1]. 
Platforms covered: Windows 7 64-bit, Windows 8.1 64-bit, Mac OS X 10.9.2 and Ubuntu 13.10 32bit.

We noticed that using High contrast themes on Windows 8.1 makes the 'i' icon disappear. Could this be related to bug 963950?

[1] https://etherpad.mozilla.org/Visual-Style-Network-error

Comment 67

[Madhava Enros [:madhava]]

Hey Bogdan and other QA folks -- is there a set of links you use to see all the different error states? If so, can you share your testing secrets? :) I'd love to see these in action.

Comment 68

[Bogdan Maris, Desktop Test Engineering]

(In reply to Madhava Enros [:madhava] from comment #67)
> Hey Bogdan and other QA folks -- is there a set of links you use to see all
> the different error states? If so, can you share your testing secrets? :)
> I'd love to see these in action.

We basically used some bad/slow/invalid proxys, no internet connection and entered invalid URLs in the URL bar. We added some examples in the etherpad from comment 66 under Connection errors section.

Comment 69

[Jim Jeffery not reading bug-mail 1/2/11]

Entering bad URL's into the address bar is not presenting the New Error page but falling back to my IPS's default 'search' page.  

I'm pretty sure I used to see the old error page however without any fail-back.  

I'm also unclear if this is just my ISP being stupid or what.

ISP: Centurylink

Comment 70

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #69)
> Entering bad URL's into the address bar is not presenting the New Error page
> but falling back to my IPS's default 'search' page.  
> 
> I'm pretty sure I used to see the old error page however without any
> fail-back.  
> 
> I'm also unclear if this is just my ISP being stupid or what.
> 
> ISP: Centurylink

It's your ISP. On many of these pages, you may find a link to disable the ISP's fallback.

Comment 71

[Valentin Tsatskin [:vt]]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #66)
> We verified the implementation of the new visual style for network errors
> pages using latest Nightly 32 2014-05-13: en-US, ar, he (rtl builds). 
> 
> We found two new issues and logged bugs on them, our testing can be found in
> etherpad [1]. 

I've addressed the issues in their respective bugs.

> 
> We noticed that using High contrast themes on Windows 8.1 makes the 'i' icon
> disappear. Could this be related to bug 963950?
> 
> [1] https://etherpad.mozilla.org/Visual-Style-Network-error

Please see bug 1009812, as I've made a patch that changes the colour of the text and the icon to be darker. Discussion of the accessibility of the colours should be differed to there.

Thanks for the great work finding these bugs!

Comment 72

[Bogdan Maris, Desktop Test Engineering]

(In reply to Valentin Tsatskin [:vt] from comment #71)
> Thanks for the great work finding these bugs!

Thanks. If there is anything we can help with here please let me know.