Closed Bug 808636 Opened 7 years ago Closed 6 years ago

Update visual style of mobile error pages

Categories

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

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 26
Tracking Status
firefox26 --- verified
fennec + ---

People

(Reporter: ibarlow, Assigned: wesj)

References

Details

Attachments

(4 files, 10 obsolete files)

Attached image mockups (obsolete) —
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.
Duplicate of this bug: 808396
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'.
OS: Mac OS X → Android
Hardware: x86 → ARM
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.
tracking-fennec: --- → ?
Attached image Desktop phishing/malware page (obsolete) —
Desktop you can click 'ignore this warning' to bypass.
bug 422410 / bug 400731 added this ui in Firefox 3
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.
tracking-fennec: ? → 19+
Strings might not make any uplift, but structural changes could
Assignee: nobody → 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.
(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*.
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/
Attached image 7" Tablet Error Page Mockups (obsolete) —
Attached image 10" Tablet Error Page Mockups (obsolete) —
Added mock ups for 7" and 10" tablets
Attachment #681186 - Attachment description: 7' Tablet Error Page Mockups → 7" Tablet Error Page Mockups
Attachment #681186 - Attachment filename: Error Pages 7' Tablet.png → Error Pages 7" Tablet.png
Since we have a single XHTML page, we'll need to use a responsive design to achieve the mockups.
(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.
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.
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?).
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.
Ok. Did someone of UX read this? Should I file a new bug? (How can I mark as "originally a duplicate of Bug xx"?)
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"...
tracking-fennec: 19+ → +
Whiteboard: ui-hackathon
Status: NEW → ASSIGNED
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.
Assignee: bnicholson → wjohnston
Keywords: polish
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Flags: needinfo?(ibarlow)
Attached image Portrait error pages (obsolete) —
Attached image Landscape error pages (obsolete) —
My S3 flips to tablet mode for these in landscape, so it gives some idea what that looks like.
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!
Flags: needinfo?(ibarlow)
Oh, and can our bulleted lists stay indented when text wraps? http://cl.ly/image/0v0n3R0p1l2P
(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.
Attached patch Patch 1/2 - Red text (obsolete) — Splinter Review
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.
Attachment #783602 - Attachment is obsolete: true
Attachment #786120 - Flags: review?(margaret.leibovic)
Attached patch Patch 2/2 (obsolete) — Splinter Review
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.
Attachment #786122 - Flags: review?(margaret.leibovic)
Attached image Portrait error pages
Screenshots. Obsoleting some old images just to clean things up.
Attachment #678355 - Attachment is obsolete: true
Attachment #678471 - Attachment is obsolete: true
Attachment #681186 - Attachment is obsolete: true
Attachment #681192 - Attachment is obsolete: true
Attachment #783603 - Attachment is obsolete: true
Attachment #783605 - Attachment is obsolete: true
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.
(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)
I stole some to use here, but we only really need one size. bigger is better. These were 64x64.
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.
Attachment #786120 - Flags: review?(margaret.leibovic) → feedback+
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.
Attachment #786122 - Flags: review?(margaret.leibovic) → feedback+
(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.)
Wes, here are the warning icons: http://cl.ly/1P3m1c2C180y
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.
Attached patch errorpage1 (obsolete) — Splinter Review
(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?
Attachment #788999 - Flags: review?(margaret.leibovic)
Attached patch Patch 2/2Splinter Review
(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.
Attachment #786122 - Attachment is obsolete: true
Attachment #789156 - Flags: review?(margaret.leibovic)
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.
(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 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?
Attachment #788999 - Flags: review?(margaret.leibovic) → review-
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.
Attachment #789156 - Flags: review?(margaret.leibovic) → review+
Attached patch Patch 1Splinter Review
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!
Attachment #786120 - Attachment is obsolete: true
Attachment #789268 - Flags: review?(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?
(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.
https://hg.mozilla.org/integration/fx-team/rev/0351b9220b15
Keywords: polish
Whiteboard: ui-hackathon → [leave-open]
Blocks: 840989
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?
Attachment #789268 - Flags: review?(margaret.leibovic) → review+
Attachment #788999 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/2cd7f480d997
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Status: RESOLVED → VERIFIED
See Also: → 947073
You need to log in before you can comment on or make changes to this bug.