756926 - Implement new Australis error page look and feel

Status: RESOLVED DUPLICATE of bug 676795

(Toolkit :: Themes, defect)

Description

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

This bug is to implement the design that is linked to in the URL field and get the in-content error pages to match some of the styling that is present in about:home and about:newtab.

Comment 1

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

Attachment: WIP patch for bug (Windows only)

This is a current WIP patch of the bug. It only implements it for Windows and currently breaks about:blocked.

Comment 2

[u428464]

I think there are more recent mockups about this by Shorlander. There was also a project by Boriss to add some features to this page, but it has been apparently abandoned.

Comment 3

[u428464]

More info in bug 510623

Comment 4

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

These are screenshots of the patch that I'm about to upload.

Network error: http://screencast.com/t/GincXyxP5
Certificate error: http://screencast.com/t/Xi4A8Oh2iFOq
Phishing attack: http://screencast.com/t/4WzmjcH3
Malware attack: http://screencast.com/t/dB3grMJbw

Comment 5

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

Attachment: Patch for bug (Windows only)

Frank, can you take a look at this patch and see if you find anything that should be changed. You can reference the above screenshots to see what it looks like.

These are links to Stephen's mockups:
http://people.mozilla.com/~shorlander/incontent-error-page/incontentErrorPage.html
http://people.mozilla.com/~shorlander/incontent-error-page/incontentBlockedPage.html

If you want to see what the pages currently look like, you can go to:
http://www.jaredwein.com (network error)
about:cert (certificate error)
http://www.mozilla.org/firefox/its-a-trap.html (phishing attack)
http://www.mozilla.org/firefox/its-an-attack.html (malware attack)

Comment 6

[Frank Yan (:fryn)]

Comment on attachment 626631 [details] [diff] [review]
Patch for bug (Windows only)

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

(In reply to Jared Wein [:jaws] from comment #4)

The following feedback is just from looking at the mockups, screenshots, and code.
No testing done yet, since the patch doesn't build, because messageBox-Blocked-Background.png is missing.

> Certificate error: http://screencast.com/t/Xi4A8Oh2iFOq

The two collapsed sections ("Technical Details" and "I Understand the Risks") have odd indentations: neither the twistys nor the text are horizontally aligned with the other sections. I realize that this is already the case with the existing styling, but this is a good opportunity to try to align the header text with the other header text ("What Should I Do?"). The margin between the twisty and the text could be reduced if there isn't enough space.

> about:cert (certificate error)

ITYM about:certerror. ;)

Thanks for the links.

These look fresher than mint! We're gonna have the best error pages. :P
Great work, Stephen and Jared! :)

::: browser/components/safebrowsing/content/blockedSite.xhtml
@@ +146,5 @@
>          text-align: right;
>        }
> +
> +      body[dir="rtl"] #ignoreWarning {
> +        text-align: left;

I think you can use `text-align: end;` here:
https://developer.mozilla.org/en/CSS/text-align

Likewise in netError.css.

::: browser/themes/winstripe/aboutCertError.css
@@ +16,1 @@
>    font: message-box;

To what does `message-box` resolve in the default OS settings on Windows 7?
It seems not to be 'Segoe UI' unfortunately.

::: toolkit/themes/winstripe/global/netError.css
@@ +60,4 @@
>  }
>  
>  #errorPageContainer.certerror {
>    background-image: url("chrome://global/skin/icons/sslWarning.png");

Doesn't this need to be updated too, given the style changes above with the addition of the gradient?

@@ +93,4 @@
>  }
>  
>  #errorTryAgain {
> +  float: right;

Is this RTL-dependent?

Comment 7

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

Attachment: Patch for bug v2 (Windows only)

Frank, thanks for the feedback. I've gone and made the changes requested as well as replaced float:right with display:block;margin-start:auto, etc.

I also removed the graphics that weren't being used anymore. blocked_large.png is still left because gnomestripe is using it.

Comment 8

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

Attachment: WIP Patch for bug v3 (Windows only)

This patch uses percentage font-sizes instead of hard-coded pixel measurements. Let me know if you think this is good to go and then I'll implement it for Linux and Mac.

Comment 9

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

Attachment: WIP Patch for bug v3 (Windows only)

Added missing image.

Comment 10

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

Comment on attachment 627061 [details] [diff] [review]
WIP Patch for bug v3 (Windows only)

Talked it over in person, I'm working on Linux and Mac now.

Comment 11

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

Attachment: Patch for bug (Windows, Linux, Mac)

Frank, can you review the style and markup changes here?
Boris, I've flagged you for review since this touches docshell, but the changes are constrained to netError.xhtml.

The patch might look large but it is mostly CSS and the addition of some new images.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 627108 [details] [diff] [review]
Patch for bug (Windows, Linux, Mac)

Why are you removing the autocomplete=off attribute?  Doesn't that just regress the bug that added it?

Comment 13

[Frank Yan (:fryn)]

(In reply to Boris Zbarsky (:bz) from comment #12)
> Why are you removing the autocomplete=off attribute?  Doesn't that just
> regress the bug that added it?

We didn't know why that was there. Having the autocomplete attribute on an element that doesn't accept text input seemed like a typo, but now I see bug 671466 and how the autocomplete attribute actually behaves. Thanks for the heads up.

I think we should include a comment that explains this usage and/or document using autocomplete=off on buttons, not just forms and inputs. I looked it up, and it wasn't documented at all on MDN.

Comment 14

[Frank Yan (:fryn)]

Comment on attachment 627108 [details] [diff] [review]
Patch for bug (Windows, Linux, Mac)

Could you upload a new version with the autocomplete=off fix? :)

Comment 15

[Boris Zbarsky [:bzbarsky]]

> We didn't know why that was there.

That's why we have a version control system with blame annotations.  ;) This time we're in luck and I _did_ know why the attribute was there, but in general if something looks like a typo it's worth checking when it was added and why.  It might still be a typo, but then you'd know for sure...

Comment 16

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

Attachment: Patch for bug v2 (Windows, Linux, Mac)

Added back the autocomplete="off" attribute.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 627326 [details] [diff] [review]
Patch for bug v2 (Windows, Linux, Mac)

r=me on the docshell bits.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 627326 [details] [diff] [review]
Patch for bug v2 (Windows, Linux, Mac)

I'll need to review this.

Comment 19

[Dão Gottwald [:dao]]

I wasn't clear. I'll at least give this a final once-over. Feel free to continue reviewing this.

Comment 20

[Frank Yan (:fryn)]

Comment on attachment 627326 [details] [diff] [review]
Patch for bug v2 (Windows, Linux, Mac)

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

Reduce the stripe PNGs to exactly one stripe in width.

What modification did you make to background-texture.png?

::: browser/components/safebrowsing/content/blockedSite.xhtml
@@ +132,1 @@
>        #ignoreWarningButton {

There's a comment in the stylesheet about why we're using a <button/> here. Could you do a s/oncommand/onclick/ s/BrowserOnCommand/BrowserOnClick/ to that?

::: browser/themes/gnomestripe/aboutCertError.css
@@ +10,5 @@
>  body {
>    margin: 0;
>    padding: 0 1em;
> +  background-image: url(chrome://global/skin/inContentUI/background-texture.png),
> +                    -moz-linear-gradient(rgb(218,223,229), rgb(230,236,242));

The background images only cover one viewport height.
Likewise for the ones in netError.*

@@ +22,5 @@
>  }
>  
>  h1 {
> +  text-align: center;
> +  font-size: 100%;

It might be useful to developers who revisit this to include a few comments for what the computed font sizes are with the default OS setting on today's machines, e.g.:
font-size: 100%; /* default: 16px */
...
font-size: 81.25%; /* default: 13px */

@@ +33,4 @@
>  }
>  
>  #errorPageContainer {
>    position: relative;

This is no longer needed, as far as I can tell.
Please remove this in *stripe/aboutCertError.css and in *stripe/netError.css.

::: docshell/resources/content/netError.xhtml
@@ +353,5 @@
>            <div id="securityOverrideContent" style="display: none;">&securityOverride.warningContent;</div>
>          </div>
> +
> +        <!-- Retry Button -->
> +        <button id="errorTryAgain" class="button" autocomplete="off" onclick="retryThis(this);">&retry.label;</button>

I've used `hg blame` many times, but I think a comment here would be helpful, e.g. appending " - autocomplete=off to prevent persistence of disabled state" to the existing "Retry Button" comment.

Comment 21

[Frank Yan (:fryn)]

This fixes the background-image problem:

html {
  display: table;
  width: 100%;
  height: 100%;
}

Comment 22

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

Attachment: Patch for bug v3 (Windows, Linux, Mac)

Addressed Frank's feedback. Thanks for providing a way to stretch the background past the viewport.

Carrying forward bz's r+ on the docshell changes.

Comment 23

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

Try results: https://tbpl.mozilla.org/?tree=Try&rev=df3085dceaf9

Comment 24

[Frank Yan (:fryn)]

Comment on attachment 627458 [details] [diff] [review]
Patch for bug v3 (Windows, Linux, Mac)

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

r+, assuming you optimized the PNGs, e.g. using pngcrush.
shorlander usually does this when he delivers us the images, but since you modified the stripe images, you might want to check.

Thanks for working on this! :)

Comment 25

[Frank Yan (:fryn)]

Comment on attachment 627458 [details] [diff] [review]
Patch for bug v3 (Windows, Linux, Mac)

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

::: toolkit/themes/gnomestripe/global/netError.css
@@ +51,5 @@
> +  border-radius: 4px;
> +  background-image: url(chrome://global/skin/inContentUI/messageBox-Warning-Background.png),
> +                    -moz-linear-gradient(rgb(234,236,239), rgb(226,229,233));
> +  background-position: right bottom, left top;
> +  background-repeat: no-repeat, repeat-x;

Note: repeat-x has the same behavior as repeat here, because, by default (background-size: auto), linear-gradients cover the whole element, which is why your linear-gradient on <body/> works w/o changing background-position or background-repeat.

Setting this here anyway doesn't really hurt.

Comment 26

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

dao: review ping?

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 627458 [details] [diff] [review]
Patch for bug v3 (Windows, Linux, Mac)

Just tested this on Linux. The error page background has a blue hue, which won't fit with most OS themes and the Firefox theme. The blue hue should at most be limited to aero glass, although http://people.mozilla.com/~shorlander/incontent-error-page/incontentErrorPage.html actually seems to be gray based as well. Generally, it seems like you should import chrome://global/skin/inContentUI.css, which we should update if needed.

Comment 28

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

Attachment: Patch for bug v4 (Windows, Linux, Mac)

Carrying forward r+ from Frank.

I talked with Stephen about the different background texture between gnomestripe/winstripe/pinstripe and he said that they should all be the same and that we should be using the one found in the pinstripe theme across all themes. This version of the patch fixes that inconsistency.

I took a look at inContentUI.css and we can't simply import that here because the necessary rules to make the background grey (instead of a blue hint) would be applied to the root element, and we need them applied to the body. To work around this I've copied the necessary parts of the inContentUI.css background-image to gnomestripe/netError.css.

Comment 29

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

dao: review ping?

Comment 30

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

Attachment: Patch for bug v5 (Windows, Linux, Mac)

This makes the background color consistent among the various platforms and uses two shades of gray for the gradient (removing the blue tint).

Carrying forward r+ from bz on the docshell change.

Comment 31

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #28)
> I took a look at inContentUI.css and we can't simply import that here
> because the necessary rules to make the background grey (instead of a blue
> hint) would be applied to the root element, and we need them applied to the
> body.

Changing "*|*:root" to "xul|*:root, html|html > html|body" should be pretty easy if that's needed to use inContentUI.css here.

Comment 32

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

Attachment: Patch for bug v5 (Windows, Linux, Mac)

Fixed the background gradients for aboutCertError.css.

These designs weren't intended to use inContentUI.css since the navigation toolbar is showing in these pages. The gradient transitions found in inContentUI.css are there to blend with the tab backgrounds which isn't necessary here.

We should be trying to follow the design that Stephen implemented instead of changing it ourselves.

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 629979 [details] [diff] [review]
Patch for bug v5 (Windows, Linux, Mac)

(In reply to Jared Wein [:jaws] from comment #32)
> The gradient transitions found in
> inContentUI.css are there to blend with the tab backgrounds which isn't
> necessary here.

You can ignore this for now. It doesn't really hurt and we'll need to stop hiding toolbars anyway when we move the Firefox menu button there.

Comment 34

[Frank Yan (:fryn)]

Comment on attachment 629979 [details] [diff] [review]
Patch for bug v5 (Windows, Linux, Mac)

We should keep these pages looking the same across platforms, as Stephen specified and as this patch implements.

(In reply to Dão Gottwald [:dao] from comment #33)
> we'll need to stop
> hiding toolbars anyway when we move the Firefox menu button there.

I'm glad we're going to stop hiding toolbars.

If we must import inContentUI.css, we should change inContentUI.css to use the same default background-image value as the lighter gray one here. The Add-ons Manager is too dark and gloomy right now anyway.

Comment 35

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ugh, we really need to coordinate better on this stuff. I've had a working patch that implements this in bug 676795 for months, that was just held up due to other projects, and getting additional graphical details.

Comment 37

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

Attachment: Patch for bug v6 (Windows, Linux, Mac)

This patch now uses inContentUI.css. I tested it on Windows, Mac, and Linux.

Comment 38

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

Jesse mentioned that he didn't like the red buttons on aboutBlocked since those two buttons are safe for users and we want them to click those buttons as opposed to the "Ignore this warning" link.

Stephen, should we switch the colors of those buttons to not be less scary?

Comment 39

[Frank Yan (:fryn)]

Comment on attachment 631186 [details] [diff] [review]
Patch for bug v6 (Windows, Linux, Mac)

I don't really have anything to add here, except that, if we're going to import inContentUI.css for these, we should land dao's patch in bug 752434 asap before we land this patch. I'm happy to code review the CSS in bug 752434.

Comment 40

[Tanvi Vyas[:tanvi]]

(In reply to Jared Wein [:jaws] from comment #38)
> Jesse mentioned that he didn't like the red buttons on aboutBlocked since
> those two buttons are safe for users and we want them to click those buttons
> as opposed to the "Ignore this warning" link.
> 
> Stephen, should we switch the colors of those buttons to not be less scary?

Makes sense.  Can we switch the color of the buttons, so users are more likely to click on them instead of "Ignore this warning".

Also, why do we remove Larry?

Thanks for your work on this Jared!

Comment 41

[Daniel Veditz [:dveditz]]

Flagging for security review, this changes both the phishing/malware warning and the certificate error pages (the latter by removing the distinction entirely).

Comment 42

[Daniel Veditz [:dveditz]]

Missed the screenshots in comment 4 and was looking at the mockups in comment 5 and missed the color band in the patch. Still think we need to consider this.

Comment 43

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

Attachment: Patch for bug v7 (Windows, Linux, Mac)

I've set a consistent look for the buttons and made them non-red on the aboutBlocked pages. This address Jesse's and Tanvi's concerns.

Comment 44

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

Comment on attachment 631186 [details] [diff] [review]
Patch for bug v6 (Windows, Linux, Mac)

I talked with Stephen and we want to keep the red buttons on the phishing/malware page. Users won't judge whether a button should be clicked based solely on the color of the button (see GMail redesign for "Compose" button) and moving to a off-white colored button will make it pop too much.

Comment 45

[Tanvi Vyas[:tanvi]]

Can we add Larry back to the cert error page?  Or something else to make the threat more immediate than our current network error pages.  (For example, chrome's cert error page is quite a bit more alarming.)

What happens when you hit "Ignore this warning" on the malware and phishing pages.  Is this behavior the same as it is now, where we get a red banner on the top of the page?

Comment 46

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

(In reply to Tanvi Vyas from comment #45)
> Can we add Larry back to the cert error page?  Or something else to make the
> threat more immediate than our current network error pages.  (For example,
> chrome's cert error page is quite a bit more alarming.)

I don't think we should add Larry back. We aren't gaining a bunch of understanding due to the Larry icon.

Users who see the new page will now see caution tape at the top, the phrase "The Connection is Untrusted", and a (much larger) warning sign in the bottom right.

> What happens when you hit "Ignore this warning" on the malware and phishing
> pages.  Is this behavior the same as it is now, where we get a red banner on
> the top of the page?

The behavior of all of these pages has remained the same.

Comment 47

[Stephen Horlander [:shorlander]]

(In reply to Tanvi Vyas from comment #45)
> Can we add Larry back to the cert error page?  Or something else to make the
> threat more immediate than our current network error pages.  (For example,
> chrome's cert error page is quite a bit more alarming.)

Larry wasn't particularly alarming or informative. The yellow warning stripe is actually quite a bit more noticeable I think. I don't think a cert error really warrants extreme levels of alarm. We are already putting a road block with scary text between people and the content.

Comment 48

[Dão Gottwald [:dao]]

Comment on attachment 631186 [details] [diff] [review]
Patch for bug v6 (Windows, Linux, Mac)

>--- a/browser/themes/gnomestripe/aboutCertError.css
>+++ b/browser/themes/gnomestripe/aboutCertError.css

> #errorPageContainer {

>+  background-image: url(chrome://global/skin/inContentUI/messageBox-Warning-Background.png),
>+                    -moz-linear-gradient(rgb(234,236,239), rgb(226,229,233));

The content box still has a blue tint here. Just dropping the linear gradient makes it look ok (you'll also need to stop hardcoding the text color then).

It's unclear to me why you set a "button" class on buttons.

Comment 49

[Tanvi Vyas[:tanvi]]

(In reply to Stephen Horlander from comment #47)
> Larry wasn't particularly alarming or informative. The yellow warning stripe
> is actually quite a bit more noticeable I think. I don't think a cert error
> really warrants extreme levels of alarm. We are already putting a road block
> with scary text between people and the content.


Okay.  I don't agree that the cert error doesn't warrant extreme levels of warnings - it really depends on what type of cert error it is.  Given we have one page for all cert errors, I think the current proposal with the yellow bar and no larry icon is fine.  

In the future, we should think about the different types of cert warnings, and perhaps have a more alarming warning for some of them.

Comment 50

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

per comment 49 we are removing the review flag as we don't see further need for sec work

Comment 51

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

Attachment: Patch for bug v7

I've fixed the issues that you brought up.

The button class was present because I was using anchors in a previous iteration of the patch. I've now removed usage of the button class.

Comment 52

[u428464]

Seen it on the UX branch, it's very nice, except for the private browsing message layout that looks weird (but I think a new version should be implemented soon).

Comment 53

[Morpheus3k]

Attachment: blurry text on buttons (OS X)

The text on the buttons for trap / attack looks blurry on OS X

Otherwise: Great work!

Comment 54

[u428464]

Same bug for the restore session that doesn't look good, I think all in-content messages are affected.

Comment 55

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

Comment on attachment 635198 [details] [diff] [review]
Patch for bug v7

Cancelling review based on the feedback from UX branch. Thanks for letting me know about these issues.

Comment 56

[u428464]

There is also a bug with "certificate page error", when you click on "technical details" there is a useless scroll bar on the right even if all the details are displayed.

Comment 57

[Tim Nguyen :ntim]

Why does that also affect the Private browsing page ?
http://ntim007.deviantart.com/art/Private-browsing-persona-for-firefox-310952319

Comment 58

[Tim Nguyen :ntim]

Attachment: Also affects Private Browsing Page

Comment 59

[Tim Nguyen :ntim]

Attachment: Shouldn't affect these pages

Affects some other pages :
about:robots
about:sessionrestore
about:privatebrowsing

Comment 60

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

Attachment: Patch for bug v8

This patch fixes about:sessionrestore, about:privatebrowsing, and about:robots.

I have made some placeholder images for those three pages (they are the numbers 1, 2, and 3 in Comic Sans font). :)

Comment 61

[Tim Nguyen :ntim]

Patch v8 landed on UX ??

Comment 62

[u428464]

I don't know if other pages have been affected as well. I've looked on :
about:
about:buildconfig
about:blank
about:cache
about:config
about:crashes
about:credits
about:logo
about:license
about:mozilla
about:plugins

And there is no problem with them. They also could possibly be updated with the new style, but it isn't very important.

Comment 63

[u428464]

About about:privatebrowsing : there should be some purple to be consistent with most representative private browsing color in Firefox. The text could also be updated as proposed in bug 611168

Comment 64

[Tim Nguyen :ntim]

(In reply to Guillaume C. [:ge3k0s] from comment #62)
> I don't know if other pages have been affected as well. I've looked on :
> about:
> about:buildconfig
> about:blank
> about:cache
> about:config
> about:crashes
> about:credits
> about:logo
> about:license
> about:mozilla
> about:plugins
> 
> And there is no problem with them. They also could possibly be updated with
> the new style, but it isn't very important.

That would be great if shorlander could make mockups of these about pages...

Comment 65

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

Attachment: Patch for bug v9

Fixed a margin-top issue that I noticed on about:sessionrestore.

Comment 66

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

Frank, Dao: review ping?

Comment 67

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

Comment on attachment 638921 [details] [diff] [review]
Patch for bug v9

Blair, do you think you could review this?

Comment 68

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 638921 [details] [diff] [review]
Patch for bug v9

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

Note: I'm testing on Linux at the moment, I'll see about testing on Windows/OSX later.

The final paragraph in about:privatebrowsing looks quite out of place - it uses a larger font than the text above it (also visible in about:robots). The tree column headers in about:sessionrestore also use a font that looks too big.

Also in about:privatebrowsing, there's a left-aligned paragraph, followed by a right-aligned button, followed by a left-aligned paragraph. Should try to fix the harmony there - possibly by moving the final paragraph to above the button, or right-aligning the final paragraph. This hadn't been an issue previously, as the button was left-aligned.

The hover state of the expanders in about:certerror, and the hover state of normal buttons from netError.css gives them a blue background, which looks out of place on Linux. It also has a box-shadow, but with a white-ish border, so it could generally do with some polishing.

The background image shows through quite clearly in the buttons (noticeable when there's an edge showing through) - I think the background of the buttons needs to be slightly more opaque. 

The "Ignore this warning" link at the bottom of about:blocked could do with some subtle visual feedback on hover/click.

There's a decent amount of code duplication between netError.css and aboutCertError.css - these was already a bit of an issue, but these changes are going to make it even harder to maintain. In bug 676795 I solved this by adding an inContentUILite.css, which could be used by all lightweight in-content UI pages, with an attribute on the :root element to select between normal and dark theme (for about:blocked). Back then, shorlander and I figured all in-content UI fell into two categories: heavyweight app-like pages (inContentUI.css), and lightweight information pages (inContentUILite.css). And that will really help with modernizing pages like about:memory, etc.

about:robots and about:privatebrowsing are very difficult to read on a dark Linux theme - they (and about:certerror) would benefit from an opaque background like about:neterror has. There are similar readability issues with all buttons (except those on about:blocked). The color of the header text in about:certerror also looks odd (and may be unreadable in other themes), and should probably be kept black like the headers in other pages.

Disabled buttons look a bit weird, and they incorrectly have hover states (visible in about:sessionrestore).

::: browser/base/content/aboutRobots.xhtml
@@ -42,5 @@
>      ]]></script>
>  
>      <style type="text/css"><![CDATA[
> -      #errorPageContainer:before {
> -        content: url('chrome://browser/content/aboutRobots-icon.png');

If this image isn't being used anymore, can it be removed?

@@ -93,5 @@
>        <button id="errorTryAgain"
>                label2="&robots.dontpress;"
>                onclick="robotButton();">&retry.label;</button>
> -
> -      <img src="chrome://browser/content/aboutRobots-widget-left.png"

Ditto.

(Though, I'd vote to keeping these and/or the icon above in the page, since it looks kinda boring without them.)

::: browser/components/safebrowsing/content/blockedSite.xhtml
@@ +174,5 @@
>          <!-- Action buttons -->
>          <div id="buttons">
>            <!-- Commands handled in browser.js -->
> +          <button class="blockedSite" id="getMeOutButton">&safeb.palm.accept.label;</button>
> +          <button class="blockedSite" id="reportButton">&safeb.palm.reportPage.label;</button>

The blockSite class added here doesn't appear to be used.

::: toolkit/themes/gnomestripe/global/netError.css
@@ +52,3 @@
>    margin: 4em auto;
> +  border-radius: 4px;
> +  background-image: url(chrome://global/skin/inContentUI/messageBox-Warning-Background.png),

Nit: Mixing url(whatever) and url("whatever") throughout the patch.

(Personally I prefer to use the quotes.)

@@ +67,5 @@
>  }
>  
>  #errorTitle {
> +  background-color: rgba(255,255,255,0.4);
> +  background-image: url(chrome://global/skin/inContentUI/messageBox-Header-Stripes.png);

There's a semi-transparent horizontal white line going through this image, about 70% of the way down (I had it in bug 676795 too, so I assume you got given the same image).

Also, this background is visible through the bottom border, since that's semi-transparent. It wouldn't normally be so much of an issue, but the border is 2 pixels, with the bottom of it being a highlight for the start of the content box below. So it makes it look like this background is bleeding into another box.

@@ +137,5 @@
> +  background-color: hsla(210,15%,25%,.2);
> +  box-shadow: 0 1px 1px hsla(210,15%,25%,.2) inset,
> +              0 0 2px hsla(210,15%,25%,.4) inset;
> +  -moz-transition-property: background-color, border-color, box-shadow;
> +  -moz-transition-duration: 10ms;

Is a transition of 10ms visibly different from no transition?

::: toolkit/themes/winstripe/global/inContentUI.css
@@ +27,5 @@
>  }
>  
>  %ifdef WINSTRIPE_AERO
>  @media (-moz-windows-default-theme) {
> +  xul|*:root {

The style a page gets shouldn't depend on whether it's implemented in XUL or HTML. Put another way, its entirely reasonable to implement an in-content page in HTML, and want this style. If we're re-using inContentUI.css like this, we should switch styles like this based on an attribute on :root.

::: toolkit/themes/winstripe/global/jar.mn
@@ +145,5 @@
> +        skin/classic/global/inContentUI/messageBox-Blocked-Background.png  (inContentUI/messageBox-Blocked-Background.png)
> +        skin/classic/global/inContentUI/messageBox-Mask-Background.png     (inContentUI/messageBox-Mask-Background.png)
> +        skin/classic/global/inContentUI/messageBox-Restore-Background.png  (inContentUI/messageBox-Restore-Background.png)
> +        skin/classic/global/inContentUI/messageBox-Robots-Background.png   (inContentUI/messageBox-Robots-Background.png)
> +        skin/classic/global/inContentUI/messageBox-Robots-Background-RTL.png   (inContentUI/messageBox-Robots-Background-RTL.png)

The robot, session restore, and private browsing background images don't belong in toolkit.

Comment 69

[u428464]

(In reply to Guillaume C. [:ge3k0s] from comment #63)
> About about:privatebrowsing : there should be some purple to be consistent
> with most representative private browsing color in Firefox. The text could
> also be updated as proposed in bug 611168

For example a purple header could be nice.

Comment 70

[u428464]

Why this bug has been abandoned so close to landing ?

Comment 71

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

(In reply to Guillaume C. [:ge3k0s] from comment #70)
> Why this bug has been abandoned so close to landing ?

While the patches attached may make it seem close to complete, addressing the review feedback provided in comment #68 will take a while to complete and I haven't been spending any time on it recently. If anyone wants to help out it would be greatly appreciated :)

Comment 72

[u428464]

I've also remembered we still miss the mockups for other pages (private browsing and other about:pages).

Comment 74

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Taking the approach originally taken in bug 676795, so duping to that (also, I fail at bugzilla :\)