808636 - Update visual style of mobile error pages

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: mockups

Now that we have a working implementation of phishing and error pages on Firefox for Android, we should update the visual design to match the newer style of our UI. We should include our standard network error pages as well. 

Mockups for phone are attached (tablet will be a slightly different design, coming soon). Specs and assets to follow.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

The phishing/malware error page needs some way to bypass this warning. Possibly a similar to the SSL error page add a twisty with 'I understand the risks'.

Comment 3

[Ian Barlow (:ibarlow)]

I had that in a previous mockup http://cl.ly/image/3G42273f3b2j
, but in talking to Larissa our resident security UX expert, it sounded like that's actually *not* a thing we want for this case. At least not on desktop, and I can't see a reason to include it here if we explicitly don't want it there. 
cc-ing Larissa, since she can likely articulate the why better than I can.

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Desktop phishing/malware page

Desktop you can click 'ignore this warning' to bypass.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

bug 422410 / bug 400731 added this ui in Firefox 3

Comment 6

[Larissa Co [:lco]]

cc'ed Bsmith who may be able to help me understand the use case.

Yes, I noticed that it's possible to override the page in desktop. But before adding that feature into mobile, I want to understand what the use case for overriding the warning is. Since we only show our most severe warning page (the red page) for known attack sites or phishing sites, why wouldn't we err on the site of protection and prevent the user from accessing the page altogether?

Furthermore, offering the option to visit the site anyway dilutes the security message to our users. If the user were truly about to do a dangerous thing, why is Firefox, who supposedly is there to protect him, allow him to do it?

To be clear, I'm not unilaterally against adding exceptions. I just want to have a clear understanding of why we need one for the malware/phishing cases so that I can recommend a safer UI that the user can understand.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Strings might not make any uplift, but structural changes could

Comment 8

[Brian Nicholson (:bnicholson)]

(In reply to Larissa Co from comment #6)
> cc'ed Bsmith who may be able to help me understand the use case.
> 
> Yes, I noticed that it's possible to override the page in desktop. But
> before adding that feature into mobile, I want to understand what the use
> case for overriding the warning is. Since we only show our most severe
> warning page (the red page) for known attack sites or phishing sites, why
> wouldn't we err on the site of protection and prevent the user from
> accessing the page altogether?
> 
> Furthermore, offering the option to visit the site anyway dilutes the
> security message to our users. If the user were truly about to do a
> dangerous thing, why is Firefox, who supposedly is there to protect him,
> allow him to do it?
> 
> To be clear, I'm not unilaterally against adding exceptions. I just want to
> have a clear understanding of why we need one for the malware/phishing cases
> so that I can recommend a safer UI that the user can understand.

One problem is that there isn't a guarantee that our filters are correct. What if some site is attacked, correctly reported as a phishing site, but then the site owner fixes the problem? The site owner could then request the site be removed from the phishing list, but how would the site owner look at his own site before then?

In addition to the "Ignore this warning" link on desktop Firefox, desktop Firefox also allows users to uncheck "Block reported attack sites" in the settings, and it gives an "Exceptions..." button for user-defined suggestions. AFAIK, Fennec doesn't have any of these.

Comment 9

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #8)
> settings, and it gives an "Exceptions..." button for user-defined
> suggestions

Err...that is, user-defined *exceptions*.

Comment 10

[Kevin Brosnan [Ex-Mozilla]]

The other problem is that users who go to a page that is blocked and still want to see it will just go to a different browser, which is likely to be less secure. The risk of a malware page on Mobile is somewhat small considering the main vector is plugins and the only one we support is click to play. Phishing I don't have a good reason to allow through other than consistency.

The situation that Brian suggests is somewhat common. For example recently Curse's minecraftwiki was hit by a malware advertisement http://www.minecraftforum.net/news/649-dont-panic-regarding-warning-visiting-this-web-site-may-harm-your-computer/

Comment 11

[Eric Pang [:epang] UX]

Attachment: 7" Tablet Error Page Mockups

Comment 12

[Eric Pang [:epang] UX]

Attachment: 10" Tablet Error Page Mockups

Added mock ups for 7" and 10" tablets

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Since we have a single XHTML page, we'll need to use a responsive design to achieve the mockups.

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Larissa Co from comment #6)
> cc'ed Bsmith who may be able to help me understand the use case.
> 
> Yes, I noticed that it's possible to override the page in desktop. But
> before adding that feature into mobile, I want to understand what the use
> case for overriding the warning is. Since we only show our most severe
> warning page (the red page) for known attack sites or phishing sites, why
> wouldn't we err on the site of protection and prevent the user from
> accessing the page altogether?

As Brian Nicholson wrote, sometimes we are wrong or our information it out-of-date.

In some ways, it is actually less serious to let the user browse a known phishing site than it is to let them override a certificate error. In the phishing site case, we won't automatically send the user's cookies or HTTP auth credentials if we proceed (because it is a different site than what they set their cookies for), but in the certificate error case, we *will* send their cookies and/or HTTP auth credentials. (Once the attacker has the cookies or HTTP auth credentials, they basically own the user's account.)

One of the problems with our existing UI is that, once the user clicks through the error page, we basically let them forget that they ignored our warning. IMO, we should have some load and clear reminder that the user is browsing the site against our advise, for as long as the issue remains.

> Furthermore, offering the option to visit the site anyway dilutes the
> security message to our users. If the user were truly about to do a
> dangerous thing, why is Firefox, who supposedly is there to protect him,
> allow him to do it?

I agree.

> To be clear, I'm not unilaterally against adding exceptions. I just want to
> have a clear understanding of why we need one for the malware/phishing cases
> so that I can recommend a safer UI that the user can understand.

Again, we allow overrides just because the information we have isn't perfect and we might be wrong. How likely is it that we're wrong? I don't know.

Comment 15

[Florian Bender]

It's possible to spoof the error / warning page easily. This could be used e. g. to trick users into downloading malware. Just a quick idea for Phone-sized displays: Paint the Address Bar's background red (what is normally blue-gray-ish). The same applies for the cert warning page.

Comment 16

[Florian Bender]

Attachment: quick (not-pixel-perfect) mockup describing the concept of a coloured address bar on warning pages

Comment 17

[Florian Bender]

Sorry for bugspam, wanted to add something: 

My proposal *may* not work (visually) for tablets (or desktop), because the warning box is detached from the browser chrome. If the empty (currently gray) background would be a (very) dark red (resp. yellow/orange), this color could be mimicked in the address bar's background (mimicking gray may look weird). 

I was also rooting for separate site identity icons in Bug 752447 and I still think this is a good idea (at least for desktop). However the mobile mockups do not include an identity icon (or similar), so this does not apply. Or is there something like identity icons in mobile? How do you identify secure sites (green or gray lock icon on Desktop)? 

Another possibility would be using e. g. a bold red text color for the warning page title in the address bar, but that may be less visible, e. g. to color-blinds (changing the address bar's background to e. g. a dark red however may be more recognizable due to less brightness). 

Leaving another possibility: Add icons to the address bar for warning pages. E. g. an orange warning sign or a red stop sign (AFAIU, there are no favicons in the address bar on mobile?).

Comment 18

[Ian Barlow (:ibarlow)]

Thanks for the suggestion Florian. We have actually been exploring designs for this idea as well. 

I wouldn't roll this into this bug though, as the scope here is solely to update the styling of the error pages. We can file a follow up bug for title bar explorations -- UX can drive the design here.

Comment 19

[Florian Bender]

Ok. Did someone of UX read this? Should I file a new bug? (How can I mark as "originally a duplicate of Bug xx"?)

Comment 20

[Peter Retzer (:pretzer)]

I think you mean "Clone This Bug" which is located in the lower right corner of the page, next to "Format For Printing", "XML" and "Top of page"...

Comment 21

[Wesley Johnston (:wesj)]

I've been trying to get all the pages together (along with a WIP implementation) here:

http://people.mozilla.com/~wjohnston/netError/index.html

They're responsive, so should adjust for small/large screens. They're mostly ready except for likely 1000 tweaks I'd like to overlay them on the mockups to compare as well. A few things I know I need to fix:

1.) The buttons on the phishing/malware page wrap text if the screen gets very small. Need to make sure they're both the same size even if only one of them wraps.

2.) The ghosted icon in the background isn't positioned quite right. Its SVG with some filters, so I also want to be careful its performant (but at least its not huge!)

3.) The boxes aren't centered on tablets/desktop.

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Patch based on some new mockups from ian. Build at:

http://people.mozilla.com/~wjohnston/errorpages.apk

has these changes. I'll attach some screenshots too. Need to do some testing on tablets though.

Comment 23

[Wesley Johnston (:wesj)]

Attachment: Portrait error pages

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Landscape error pages

My S3 flips to tablet mode for these in landscape, so it gives some idea what that looks like.

Comment 25

[Ian Barlow (:ibarlow)]

OMG! Wes this is awesome, I thought you were still waiting for me on more finalized designs! Anyway I made a few tweaks to the designs I showed you earlier this week, so that they are more in line with where desktop is headed with their in-content UI. Nothing too major though.  Mocks: http://cl.ly/image/2t1d453D2R0b

Changes: 

* Added 1dp horizontal rule under titles
* Buttons: removed bottom shadow and added a 2dp corner radius
* Changed typeface to Clear Sans (we can do this later when bug 877203 lands, using Open sans is fine for now.)

A few additional comments on your screenshots and then we are GOOD!

* Adjust height of stripy bar - should be 32dp, so a little shorter than the title bar
* Make light / dark stripes equal width. Right now dark looks to be wider than light. 
* Update the error favicons to grey triangle, yellow triangle, red triangle (and red url text?)

Thanks!

Comment 26

[Ian Barlow (:ibarlow)]

Oh, and can our bulleted lists stay indented when text wraps? http://cl.ly/image/0v0n3R0p1l2P

Comment 27

[Florian Bender]

(In reply to Ian Barlow (:ibarlow) from comment #25)
> * Update the error favicons to grey triangle, yellow triangle, red triangle
> (and red url text?)

In addition to the favicons, there should definitely be a special indicator to prevent spoofing (assuming that websites can set the icon in the URL bar, unlike in Fx Desktop). I mocked up another solution in attachment #682743 [details], but changing the URL text color is fine, too.

Comment 28

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - Red text

This makes the title on about:blocked pages red. We need the correct documentUri for this to work. Unfortunately, we don't seem to know that until DOMContentLoaded, so I'm updating it there.

Comment 29

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

This is the real retheming of these pages. TBH, I just threw about our current netError.css and rewrote it from scrath. There's also a little CSS in the xhtml files themselves that has to be updated, and some work to move the titles inside the normal content box.

I need to do a final pass looking at these on a tablet.

Comment 30

[Wesley Johnston (:wesj)]

Attachment: Portrait error pages

Screenshots. Obsoleting some old images just to clean things up.

Comment 31

[Florian Bender]

May you please highlight "yellow" warnings (e. g. untrusted connection) as well? Though you'll probably need a very dark yellow (ocher) to keep the contrast.

Comment 32

[Ian Barlow (:ibarlow)]

(In reply to Florian Bender from comment #31)
> May you please highlight "yellow" warnings (e. g. untrusted connection) as
> well? Though you'll probably need a very dark yellow (ocher) to keep the
> contrast.

I'd actually prefer that we only change the text colour in the url bar in the case of a real emergency like an attack site. For the 'yellow' pages we should leave the text black. 

Wes do you need favicons for these? If so what size(s)

Comment 33

[Wesley Johnston (:wesj)]

I stole some to use here, but we only really need one size. bigger is better. These were 64x64.

Comment 34

[:Margaret Leibovic]

Comment on attachment 786120 [details] [diff] [review]
Patch 1/2 - Red text

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

Looking good, but I have a few questions before giving an r+.

::: mobile/android/base/BrowserToolbar.java
@@ +471,5 @@
>                  break;
> +            case LOADED:
> +                if (Tabs.getInstance().isSelectedTab(tab)) {
> +                    updateTitle();
> +                    break;

This break should go outside the if statement, right? Otherwise we'll fall through and do all the different things below.

@@ +937,5 @@
>          }
>  
> +        // Show the about:blocked page title in red, regardless of prefs
> +        String docURI = tab.getDocumentURI();
> +        if (!TextUtils.isEmpty(docURI) && docURI.startsWith("about:blocked")) {

Any reason you're using startsWith instead of equals?

::: mobile/android/base/Tabs.java
@@ +443,5 @@
>                  } else {
>                      // Default to white if no color is given
>                      tab.setBackgroundColor(Color.WHITE);
>                  }
> +                tab.setDocumentURI(message.optString("documentURI"));

Why do you need to do this? We already set the documentURI in Tab.handleLocationChange, and I'm wondering why it would be different here.

If there's some specific reason you need to do this here for blocked pages, I feel like I would prefer just passing along a flag about the fact that it's a blocked page in that case, since 99.9% of the time this is just going to be redundant with what we're already doing on location change.

If you set some "this page is blocked" property on the tab, that would also let you avoid the check for "about:blocked" above, which feels more robust.

Comment 35

[:Margaret Leibovic]

Comment on attachment 786122 [details] [diff] [review]
Patch 2/2

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

I have enough comments that I'd like to see a new version of the patch, and answers to some questions.

Even though I included some picky CSS comments, I don't actually feel too strongly about optimizing this CSS, since this is a pretty small/simple page. In this case, it's probably best to optimize for code readability over anything else.

::: mobile/android/chrome/content/blockedSite.xhtml
@@ +126,5 @@
>      <style type="text/css">
>        /* Style warning button to look like a small text link in the
>           bottom right. This is preferable to just using a text link
>           since there is already a mechanism in browser.js for trapping
>           oncommand events from unprivileged chrome pages (BrowserOnCommand).*/

This comment is out of date (there is no BrowserOnCommand). We should update it.

@@ +138,5 @@
>          font-size: smaller;
> +        border-bottom: none;
> +        position: absolute;
> +        bottom: 0px;
> +        left: 0px;

You can just use 0 instead of 0px (we have so little CSS, I don't know what our conventions are, but that's a convention on desktop).

@@ +147,2 @@
>        }
>      </style>

I initially wondered why these styles weren't in a CSS file, then realized we're using a shared netError.css file, but then noticed there are styles in that file specific to different error pages. If we're going to have .blockedsite styles in there anyway, I think we might as well put all the CSS in there to keep things organized.

::: mobile/android/themes/core/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,
> +body {

Why do you need to declare these rules on both html and body?

@@ +29,5 @@
> +  margin-right: auto;
> +  padding-top: 10px;
> +}
> +
> +body.certerror {

Is the certerror class ever set on things that aren't the body? If not, it's more performant to just use the class selector.

@@ +63,3 @@
>  }
>  
> +h2.expander {

Same thing here. I suppose it makes the CSS more readable to know what type of element the class is set on, but a comment could add that information as well.

@@ +76,3 @@
>  }
>  
> +div[collapsed="true"] h2.expander {

Parent selectors are more efficient than descendant selectors, so you should use them if you can (I also find it makes rules more explicit).

@@ +82,3 @@
>  }
>  
> +div[collapsed="true"] h2.expander + * {

Can you add a comment about what this is selecting?

@@ +86,4 @@
>  }
>  
> +p {
> +  margin-top: 0px;

Same thing with the 0.

@@ +97,5 @@
>  
> +button {
> +  width: 100%;
> +  border: none;
> +  padding: 1.25rem;

It seems a bit inconsistent to use rem for this an pixels for other styles. Is there a reason you decided to use rem in some places but not others?

@@ +116,4 @@
>    color: white;
>  }
>  
> +@media (min-width: 550px) {

Can you add some comments about what these media queries are selecting?

@@ +121,5 @@
> +    min-width: 160px;
> +    width: auto;
> +  }
> +
> +  button#ignoreWarningButton {

You should get rid of the button part of this selector (I assume there's only one #ignoreWarningButton element and it will always be a button).

@@ +122,5 @@
> +    width: auto;
> +  }
> +
> +  button#ignoreWarningButton {
> +    width: 100%;

I'm curious why this button needs a different width rule than the default button styles.

Comment 36

[Florian Bender]

(In reply to Ian Barlow (:ibarlow) from comment #32)
> I'd actually prefer that we only change the text colour in the url bar in
> the case of a real emergency like an attack site. For the 'yellow' pages we
> should leave the text black. 

This is actually a measurement against spoofing. As "yellow warning pages" do include interactive elements (e. g. confirm cert exception), a user may be lured into clicking on elements in a spoofed error page thus initiating an attack. Immutable (by content) UI can mitigate that, like the red address bar text. (Firefox desktop has the identity icon which serves as an indicator for "native" messages. Firefox Mobile AFAIK displays the Favicon in place of the identity icon which is spoofable.)

Comment 37

[Ian Barlow (:ibarlow)]

Wes, here are the warning icons: http://cl.ly/1P3m1c2C180y

Comment 38

[Florian Bender]

But again, as long as these icons can be resembled by favicons (i. e. a site can use any of these icons as a favicon, and such a site is indistinguishable from an error site), they are of little use to mitigate spoofing.

Comment 39

[Wesley Johnston (:wesj)]

Attachment: errorpage1

(In reply to :Margaret Leibovic from comment #34)
> Any reason you're using startsWith instead of equals?

Error pages tend to have lots of query params on the end to give details of the error. i.e.

about:blocked?e=phishingBlocked

Desktop also has to do this when its checking things in js http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2393

> Why do you need to do this? We already set the documentURI in
> Tab.handleLocationChange, and I'm wondering why it would be different here.

I wasn't seeing the correct uri get set in handleLocationChange. Gavin tells me locationChange doesn't even fire for error pages (although bug 769189 seems to disagree, and I swear I saw it fire, just with the wrong documentUri...). Regardless, I think we want this URI to be set correct for our own use, and if the only time we have it is in DOMContentLoaded, I'd like to send it up then and keep java-gecko in sync.

At that point a flag feels a bit redundant to me. Maybe you'd rather we had a Tab.isBlockedPage() method instead?

Comment 40

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

(In reply to :Margaret Leibovic from comment #35)
> I think we might as well put all the
> CSS in there to keep things organized.

Agreed. Originally I wanted to keep us in sync with desktop (in fact, with these patches, i think we're pretty close to being able to merge back with them). But desktop shouldn't have this either. I'm guessing its the work of some /dom engineer.
 
> Can you add a comment about what this is selecting?

I added a bunch of comments. I'm not sure if they help or not, but I often look at CSS and wonder "why?" so I hope they do.

> It seems a bit inconsistent to use rem for this an pixels for other styles.
> Is there a reason you decided to use rem in some places but not others?
I also moved most things back to rem (except for some very specific background-image, media query, and border rules.

Comment 41

[Larissa Co [:lco]]

Hi, Ian asked me to respond with my opinion since I've been involved in a lot of trusted UI conversations in the past. I understand the security intent here: we want users to be able to distinguish important UI that's coming from us vs. from someone trying to spoof us. But in practice, I don't think that the yellow text (or even the red one) can successfully mitigate that issue*

I think the goal with Ian's design isn't to create an unspoofable UI. Rather, it's to convey the urgency and importance of a threat. Hence, it makes sense to have red text and potentially change the color of the UI when there's a severe warning. But when it comes to a less urgent warning, which is what we use the "yellow" UI for, I agree with him; I think the yellow text is overkill.

That being said, if we are concerned with spoofing, then I'd suggest that we open another bug to look into that. It's certainly worthwhile, but quite hard in practice to design counter-measures for. Perhaps we can find a combination between a technical and design solution which makes it hard to actually serve malicious UI to people, and also make it obvious to the user when it isn't a real warning. 


* The problem is, the concept of trusted UI only works when the user knows what cue to look for, knows what it symbolizes, and looks for it just before he interacts with the page. Even experts have a hard time remembering to do all this. One of the reasons it fails from a cognition standpoint is that it's harder to notice the absence of security indicators rather than the presence of one. Another is that users aren't actively looking for security threats, which means that they'll just visually "skim" what they see on the screen to see the overall message. So if the main content of the browser looks sufficiently scary enough, the user isn't going to stop and think to see if it's a true warning vs. a fake one.

Comment 42

[Larissa Co [:lco]]

(In reply to Florian Bender from comment #36)
> This is actually a measurement against spoofing. As "yellow warning pages"
> do include interactive elements (e. g. confirm cert exception), a user may
> be lured into clicking on elements in a spoofed error page thus initiating
> an attack. Immutable (by content) UI can mitigate that, like the red address
> bar text. (Firefox desktop has the identity icon which serves as an
> indicator for "native" messages. Firefox Mobile AFAIK displays the Favicon
> in place of the identity icon which is spoofable.)

See the above comment (comment 41), which is a response in reference to this comment. (I totally screwed up on this whole commenting business)

Comment 43

[:Margaret Leibovic]

Comment on attachment 788999 [details] [diff] [review]
errorpage1

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

r- because I'm scope creeping this patch to clean up the current unused documentURI stuff.

::: mobile/android/base/Tabs.java
@@ +443,5 @@
>                  } else {
>                      // Default to white if no color is given
>                      tab.setBackgroundColor(Color.WHITE);
>                  }
> +                tab.setDocumentURI(message.optString("documentURI"));

I still don't really like that we're setting the documentURI in two places. This inspired me to do some digging, and I found that we added the existing documentURI stuff in bug 707665, and we aren't even using it for anything!

I think we can stop sending the documentURI along with the location change event, and if we really are only using this to tell if we're on a blocked page, we should do that check in JS, and pass along an isBlockedPage flag with the DOMContentLoaded event. Then we can get rid of the documentURI stuff on Tab and replace it with an isBlocked flag.

What do you think of that?

Comment 44

[:Margaret Leibovic]

Comment on attachment 789156 [details] [diff] [review]
Patch 2/2

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

Looking good, but I do have a few nits and questions.

::: mobile/android/themes/core/jar.mn
@@ +51,2 @@
>    skin/images/errorpage-warning.png         (images/errorpage-warning.png)
> +  skin/images/certerror-warning.png         (images/certerror-warning.png)

Nit: put this up above where it belongs in alphabetical order.

::: mobile/android/themes/core/netError.css
@@ +17,5 @@
> +  background-repeat: repeat-x;
> +
> +  background-color: #f1f1f1;
> +  min-height: 100%;
> +  padding: 0px 20px;

Nit: s/0px/0

@@ +27,5 @@
> +}
> +
> +
> +ul {
> +  /* Shove the list indicator so that its left aligned, but use outside so that text

s/its/it's

@@ +34,2 @@
>    margin: 0;
> +  list-style: round outside none;

I think this is the first time I've ever seen multiple keywords with list-style. Forcing me to go read some docs... and having read them, I'm not sure this is valid. What is round? If you have list-style: none, doesn't that make nothing else matter?

@@ +42,4 @@
>  }
>  
>  h1 {
> +  padding: 1rem 0px;

Nit: s/0px/0 (especially since you're using rem for the other unit)

@@ +62,5 @@
> +  border: none;
> +  padding: 1rem;
> +  font-family: "Open Sans", sans-serif;
> +  background-color: #e0e2e5;
> +  font-size: 1rem; /* Not sure why this has to be specified... */

What happens if you don't specify it?

@@ +70,4 @@
>  }
>  
> +button + button {
> +  margin-top: 1em;  

whitespace

@@ +88,5 @@
>  }
>  
> +#errorPageContainer {
> +  /* If the page is greater than 550px center the content.
> +   * This number should be kept in sync with the media query for tablets below */

Nice comments in here.

@@ +103,5 @@
> +/* Expanders have a structure of
> + * <div collapsed="true/false">
> + *   <h2 class="expander">Title</h2>
> + *   <p>Content</p>
> + * </div>

Super nice comment.

@@ +146,5 @@
>  
> +/* Style warning button to look like a small text link in the
> +   bottom. This is preferable to just using a text link
> +   since there is already a mechanism in browser.js for trapping
> +   oncommand events from unprivileged chrome pages (ErrorPageEventHandler).*/

Nit: Put *s on every comment line to be consistent with the other comments you added above.

Comment 45

[Wesley Johnston (:wesj)]

Attachment: Patch 1

So, I think we need to know if the current page is an error page in Bug 840989 as well (not just blocked), so I thought we might try something like this. This stores an "errorType" enum on Tab that we set in DOMContentLoaded.

TBH, I still like just storing the documentURI, but this feels very java-ish!

Comment 46

[:Margaret Leibovic]

Comment on attachment 789268 [details] [diff] [review]
Patch 1

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

I have mixed feelings about this... on the one hand, it lets Java remain agnostic of the about:whatever implementation of error pages, but on the other hand, we're not using all of these types yet. I'm also a bit worried about the raciness of clearing the type on location change, but setting it on DOMContentLoaded. We call updateTitle() in a bunch of places, and I don't want us to accidentally show an error title when we don't mean to. However, I suppose keeping track of the documentURI instead of an error type doesn't fix this issue, and it actually makes me realize we'd still want to clear the documentURI on location change, so that we don't accidentally update the wrong title. I now think I've convinced myself that this approach is good, but I still want to think about it a bit :)

::: mobile/android/chrome/content/browser.js
@@ +3656,5 @@
>      this.pluginDoorhangerTimeout = null;
>      this.shouldShowPluginDoorhanger = true;
>      this.clickToPlayPluginsActivated = false;
>      // Borrowed from desktop Firefox: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#174
> +    let documentURI = contentWin.document.documentURI;

This is different than the line that was removed up above. Shouldn't this be contentWin.document.documentURIObject.spec?

Comment 47

[Florian Bender]

(In reply to Larissa Co [:lco] from comment #42) 
> See the above comment (comment 41), which is a response in reference to this
> comment. (I totally screwed up on this whole commenting business)

Thanks a lot for your view on this subject, and I see the point you make. I filed bug 904537 to follow up with a different proposal (not necessarily the one I mention in that bug). You can mark bug 904537 as depending on this one if you like, however I think these are separate issues.

Comment 48

[Wesley Johnston (:wesj)]

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

Comment 49

[Wesley Johnston (:wesj)]

Missed adding an image:
https://hg.mozilla.org/integration/fx-team/rev/cc0093fe126a

Comment 50

[:Margaret Leibovic]

Comment on attachment 789268 [details] [diff] [review]
Patch 1

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

I decided I do like this. r+ with comments addressed.

::: mobile/android/base/Tab.java
@@ +278,4 @@
>      }
>  
> +    public void setErrorType(ErrorType type) {
> +        Log.i(LOGTAG, "Set error: " + type);

Remove this logging before landing.

@@ +281,5 @@
> +        Log.i(LOGTAG, "Set error: " + type);
> +        mErrorType = type;
> +    }
> +
> +    public ErrorType getErrorType() {

If ErrorType is package access, maybe these should be, too (or make ErrorType public).

::: mobile/android/chrome/content/browser.js
@@ +3345,5 @@
> +          errorType = "certerror";
> +        else if (docURI.startsWith("about:blocked"))
> +          errorType = "blocked"
> +        else if (docURI.startsWith("about:neterror"))
> +          errorType = "error";

I think using "neterror" would be more consistent. This would also mean updating ErrorType.ERROR to NET_ERROR.

Also nit: put a blank line after this.

@@ +3656,5 @@
>      this.pluginDoorhangerTimeout = null;
>      this.shouldShowPluginDoorhanger = true;
>      this.clickToPlayPluginsActivated = false;
>      // Borrowed from desktop Firefox: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#174
> +    let documentURI = contentWin.document.documentURI;

This is different than the line that was removed up above. Shouldn't this be contentWin.document.documentURIObject.spec?

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0351b9220b15
https://hg.mozilla.org/mozilla-central/rev/cc0093fe126a

Comment 52

[Wesley Johnston (:wesj)]

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

Comment 53

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2cd7f480d997