Last Comment Bug 463504 - Port new cert error page from Firefox to SeaMonkey
: Port new cert error page from Firefox to SeaMonkey
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Security (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.0a2
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on: 431826 464024
Blocks: 469112 729000
  Show dependency treegraph
 
Reported: 2008-11-06 14:20 PST by Robert Kaiser
Modified: 2012-02-24 00:47 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP patch (37.31 KB, patch)
2008-11-07 09:18 PST, Misak Khachatryan
no flags Details | Diff | Splinter Review
WIP, addresses KaiRo comments, temporarily enables contenaccessible (38.26 KB, patch)
2008-11-23 03:24 PST, Misak Khachatryan
neil: review-
neil: ui‑review-
Details | Diff | Splinter Review
fixed almost all Neil comments (33.10 KB, patch)
2008-11-25 06:16 PST, Misak Khachatryan
neil: review-
neil: ui‑review-
Details | Diff | Splinter Review
fixed comments (32.93 KB, patch)
2008-11-26 06:06 PST, Misak Khachatryan
neil: review-
neil: ui‑review+
Details | Diff | Splinter Review
removed, global cerError.css, fixed all comments (31.08 KB, patch)
2008-11-27 08:38 PST, Misak Khachatryan
neil: review+
Details | Diff | Splinter Review
final patch (30.96 KB, patch)
2008-11-27 10:59 PST, Misak Khachatryan
neil: review+
neil: superreview+
kairo: approval‑seamonkey2.0a2+
Details | Diff | Splinter Review
for checkin (30.76 KB, patch)
2008-11-28 07:50 PST, Misak Khachatryan
no flags Details | Diff | Splinter Review

Description Robert Kaiser 2008-11-06 14:20:04 PST
Firefox 3.1 implements a new certificate error page (see bug 431826 and http://blog.johnath.com/2008/11/06/ssl-error-pages-in-firefox-31/ for the work and some description) which makes it easier for people to grasp cert errors. I think it would be a good idea to port this work to SeaMonkey as well.
Comment 1 Johnathan Nightingale [:johnath] 2008-11-06 14:23:13 PST
For what it's worth, this should net out to less pain than the current way of doing it, I hope (no more xul-in-netError.dtd-overrides).  The implementation of the page itself lives in browser/components/certerror, but really you could implement any about:certerror you wanted to and docshell should redirect to it instead of using the stock netError page.
Comment 2 Misak Khachatryan 2008-11-07 09:18:23 PST
Created attachment 346908 [details] [diff] [review]
WIP patch

This patch is working, but reviewers should decide where to put files. Also it has problems accessing css. Error console says "Security Error: Content at about:certerror?e=nssBadCert&u=https%3A//aaa.xter.net/admin/login.php&c=UTF-8&d=billing.xter.net%20uses%20an%20invalid%20security%20certificate.%0A%0AThe%20certificate%20is%20not%20trusted%20because%20it%20is%20self%20signed.%0AThe%20certificate%20is%20only%20valid%20for%20%3Ca%20id%3D%22cert_domain_link%22%20title%3D%22localhost.localdomain%22%3Elocalhost.localdomain%3C/a%3E%0AThe%20certificate%20expired%20on%2004/18/2008%2010%3A55%20AM.%0A%0A%28Error%20code%3A%20sec_error_expired_issuer_certificate%29%0A may not load or link to chrome://communicator/content/aboutCertError.css." for both css files. So it displaying regular HTML page from 80-ies.
Comment 3 Misak Khachatryan 2008-11-08 06:13:55 PST
In aboutCertError.js newChannel function returns channel object with owner of secMan.getCodebasePrincipal(aURI). If i delete owner and leave it default, page can acces it's css and image files.
I can't understand the purpose of owner object - if it's something related with security, then why about:sessionrestore page  (which i'm also porting) didn't use it? I need advice here.
Comment 4 Robert Kaiser 2008-11-08 10:36:58 PST
Comment on attachment 346908 [details] [diff] [review]
WIP patch

diff --git a/suite/common/jar.mn b/suite/common/jar.mn
--- a/suite/common/jar.mn
+++ b/suite/common/jar.mn
@@ -65,6 +65,8 @@ comm.jar:
    content/communicator/helpOverlay.xul
    content/communicator/aboutSessionRestore.xhtml
    content/communicator/aboutSessionRestore.js
+   content/communicator/aboutCertError.xhtml
+   content/communicator/aboutCertError.css

Please follow alphabetic order in the jar.mn, we tried hard to get there when moving things to suite/ :)
(same is true for the one in locales)

diff --git a/suite/locales/en-US/chrome/common/aboutCertError.dtd b/suite/locales/en-US/chrome/common/aboutCertError.dtd
new file mode 100644
--- /dev/null
+++ b/suite/locales/en-US/chrome/common/aboutCertError.dtd
@@ -0,0 +1,36 @@
+<!ENTITY % brandDTD
+    SYSTEM "chrome://branding/locale/brand.dtd">
+  %brandDTD;

Don't include other files from the .dtd, that's bad style, do that from the original document.

+<!ENTITY certerror.getMeOutOfHere.label "Get me out of here!">

Please use the different string we have for that in netErrorApp.dtd.

That said, you probably should remove our netErrorApp.dtd override stuff completely with this work, as it's only there for the older variant of getting the exceptions stuff onto the netError page.

Thanks for caring about modern already in the WIP :)

And I'm probably not the right person for review here, I feel better with leaving this to someone who actually understands the code, like Neil :)
Comment 5 Johnathan Nightingale [:johnath] 2008-11-08 13:16:18 PST
(In reply to comment #3)
> In aboutCertError.js newChannel function returns channel object with owner of
> secMan.getCodebasePrincipal(aURI). If i delete owner and leave it default, page
> can acces it's css and image files.
> I can't understand the purpose of owner object - if it's something related with
> security, then why about:sessionrestore page  (which i'm also porting) didn't
> use it? I need advice here.

The purpose of that code in about:certerror is to ensure that the content of that page doesn't have chrome privilege.  Imagine Firefox/Seamonkey was discovered to have some flaw in the way it kept iframe content separate, such that content in one frame was allowed to alter another.  Typically, that would be a bad thing, it would let pages tamper with each other, and we would want to fix it.  But if one of those pages had chrome privilege, it would become much worse - you could inject script to do all manner of bad things, including taking over the user's computer.

As a result, we don't want content pages to have privilege if there's any chance of them being something that content can cause.  Content can (obviously) cause the certificate error pages to appear, by just pointing an iframe at a page with a bad certificate, so it clearly falls into this category.  about:sessionrestore is not as easy for content to reference.  Because it does not have the URI_SAFE_FOR_UNTRUSTED_CONTENT flag in its getURIFlags, it can't be linked to directly, and unlike certificate errors, there isn't an obvious way for someone to construct a page which would cause it to be shown (it's only shown at the start of a restored session).  Therefore, it's less pressing that it drop privilege.  But I'd still be happier if it did - the fewer of those pages we have, the better.

I would figure out which code is blocking you from loading those pages, and see if it's appropriate to carve out an exception there for about: pages.  You'll see in my patch for bug 431826 that I had to do something similar in browser.js to let the page access its favicon.
Comment 6 neil@parkwaycc.co.uk 2008-11-08 13:53:11 PST
Comment on attachment 346908 [details] [diff] [review]
WIP patch

Sorry, but this patch only applies to your build that includes the session restore patch so I can't apply it to my tree.
Comment 7 Misak Khachatryan 2008-11-10 00:47:31 PST
(In reply to comment #5)
> I would figure out which code is blocking you from loading those pages, and see
> if it's appropriate to carve out an exception there for about: pages.  You'll
> see in my patch for bug 431826 that I had to do something similar in browser.js
> to let the page access its favicon.

Thank you very much Johnathan for explanation, now it's clear. Will wait you investigation.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-11-10 00:59:32 PST
Firefox's content package is marked contentaccessible, which means that the about page can reference its files. IIRC SeaMonkey's isn't marked contentaccessible.
Comment 9 Robert Kaiser 2008-11-10 04:04:26 PST
So, is it a potential security issue that Firefox' content package is contentaccessible, or is it a bug that SeaMonkey's isn't?
Is there a different way to get that access?
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-11-10 04:15:59 PST
In practice there's not much point to marking your chrome contentaccessible=no. Theoretically it reduces the attack surface if bugs are found in our security model such that being able to load a chrome:// URI somehow enables privilege escalation, but given that the main toolkit package is marked contentaccessible already (and needs to be to not break remote XUL), it's unlikely that SeaMonkey's chrome is going to make a difference there. It was introduced mostly because a bunch of people blogged/hyped the fact that it was possible to identify installed extensions by loading their scripts.
Comment 11 Misak Khachatryan 2008-11-10 06:50:17 PST
Thanks Gavin for explanation, filed bug 464024.
Comment 12 Misak Khachatryan 2008-11-23 03:24:24 PST
Created attachment 349635 [details] [diff] [review]
WIP, addresses KaiRo comments, temporarily enables contenaccessible

I decided to temporarily enable chrome contentaccessible to make some progress here until bug 464024 gets it's resolution.
This patch has some bad behavior, - it displays both cert error dialog and cert error page when mailnews is open. See comment #90 in Bug 429843. I need help here, but i mostly sure that the problem not in this patch, because it didn't add any new code.

(In reply to comment #4)
> Please follow alphabetic order in the jar.mn, we tried hard to get there when
> moving things to suite/ :)
> (same is true for the one in locales)

Fixed.
 
> +++ b/suite/locales/en-US/chrome/common/aboutCertError.dtd
> @@ -0,0 +1,36 @@
> +<!ENTITY % brandDTD
> +    SYSTEM "chrome://branding/locale/brand.dtd">
> +  %brandDTD;
> 
> Don't include other files from the .dtd, that's bad style, do that from the
> original document.

Fixed.
 
> +<!ENTITY certerror.getMeOutOfHere.label "Get me out of here!">
> 
> Please use the different string we have for that in netErrorApp.dtd.
> 

Done.

> That said, you probably should remove our netErrorApp.dtd override stuff
> completely with this work, as it's only there for the older variant of getting
> the exceptions stuff onto the netError page.

This should stay for mailnews part of cert exceptions.
Comment 13 Robert Kaiser 2008-11-23 04:28:09 PST
(In reply to comment #12)
> > That said, you probably should remove our netErrorApp.dtd override stuff
> > completely with this work, as it's only there for the older variant of getting
> > the exceptions stuff onto the netError page.
> 
> This should stay for mailnews part of cert exceptions.

MailNews doesn't use the netError pages, they use a dialog instead, as they don't have a webpage to show. We added the override only to give us some better UI in the browser, as you're replacing that, we don't need the netErrorApp override stuff any more.
Comment 14 neil@parkwaycc.co.uk 2008-11-24 07:06:41 PST
Comment on attachment 349635 [details] [diff] [review]
WIP, addresses KaiRo comments, temporarily enables contenaccessible

I accidentally deleted the first revision of this review, so it's possible that I may have overlooked something this time.

>               const aboutNeterr = /^about:neterror\?/;
>+              const aboutCert = /^about:certerror\?/;
Nit: aboutCerterr for consistency

>+              if (!(aboutNeterr.test(targetDoc.documentURI) ||
>+                  aboutCert.test(targetDoc.documentURI)) ||
Nit: the "about"s line up.

This boolean expression is really hairy but I couldn't improve on it :-(

>+/* Logical CSS rules belong here, but presentation & theming rules
>+   should live in the CSS of the appropriate theme */
I don't think there are enough logical CSS rules here to make it worthwhile.

>+  padding-left: 20px;
>+  left: -20px;
For instance these definitely aren't logical, they're theme-dependent.

diff --git a/suite/common/aboutCertError.xhtml b/suite/common/aboutCertError.xhtml
Please rename this and similar files certError.* for consistency with netError

>+    <link rel="stylesheet" href="chrome://communicator/skin/aboutCertError.css" type="text/css" media="all" />
>+    <link rel="stylesheet" href="chrome://communicator/content/aboutCertError.css" type="text/css" media="all" />
If you really wanted both stylesheets then you would either list the content one first, or you would preferably import it from the theme one, so that the theme can override the content sensibly.

>+    <!-- This page currently uses the same favicon as neterror.xhtml.
>+         If the location of the favicon is changed for both pages, the
>+         FAVICON_ERRORPAGE_URL symbol in toolkit/components/places/src/nsFaviconService.h
>+         should be updated. If this page starts using a different favicon
>+         than neterrorm nsFaviconService->DoSetAndLoadFaviconForPage
>+         should be updated to ignore this one as well. -->
We don't need this comment here.

>+        // Replace the "#1" string in the intro with the hostname.  Trickier
>+        // than it might seem since we want to preserve the <b> tags
Not really. Just use some XPath:
var node = document.evaluate('//text()[string()="#1"]', intro, null, XPathResult.ANY_UNORDERED_NODE_TYPE, null).singleNodeValue;
if (node)
  node.textContent = location.host;

>+        var tech = document.getElementById("technicalContentText");
>+        if (tech)
>+          tech.textContent = getDescription();
>+        
>+        addDomainErrorLink();
>+      }
>+      
>+      /* In the case of SSL error pages about domain mismatch, see if
>+         we can hyperlink the user to the correct site.  We don't want
>+         to do this generically since it allows MitM attacks to redirect
>+         users to a site under attacker control, but in certain cases
>+         it is safe (and helpful!) to do so.  Bug 402210
>+      */
>+      function addDomainErrorLink() {
Might as well do some tail call optimisation by deleting these last ten lines. Also you're now duplicating variables, so you can eliminate that. Then you can move the line that sets the plain description into the inner if block.

>+          // Remove sd's existing children
>+          sd.textContent = "";
It won't have any now.

>+          // Everything up to the link should be text content
>+          sd.appendChild(document.createTextNode(desc.slice(0, result.index)));
But .textContent could have been used here instead to make sure.

>+          // Now create the link itself
>+          var anchorEl = document.createElement("a");
>+          anchorEl.setAttribute("id", "cert_domain_link");
>+          anchorEl.setAttribute("title", result[1]);
>+          anchorEl.appendChild(document.createTextNode(result[1]));
>+          sd.appendChild(anchorEl);
>+          
>+          // Finally, append text for anything after the closing </a>
>+          sd.appendChild(document.createTextNode(desc.slice(desc.indexOf("</a>") + "</a>".length)));
>+        }
>+
>+        var link = document.getElementById('cert_domain_link');
Oh, wait! this is the anchorEl we created just a second ago. The only difference is that we're outside of the if block. Change that to an early return to make this more readable, and fix up the variable names.

>+        var okHost = link.getAttribute("title");
We already know this too, but you might want to move the variable so we can reuse it more often.

>+          link.href = proto + okHost;
Eww, no /s?

>+        if (el.getAttribute("collapsed"))
Nit: hasAttribute

>diff --git a/suite/common/src/aboutCertError.js b/suite/common/src/aboutCertError.js
nsAboutCertError.js for consistency with nsAboutAbout.js


>+//    var principal = secMan.getCodebasePrincipal(aURI);
>+
>+    channel.originalURI = aURI;
>+//    channel.owner = principal;
You meant to uncomment these, right?

>-  if (/^about:neterror\?e=nssBadCert/.test(ownerDoc.documentURI)) {
I wonder whether we should keep this for power users wanting the old page.
>+  if (/^about:certerror/.test(ownerDoc.documentURI)) {
You lost the \?

> bin\components\nsDefaultCLH.js
> bin\components\nsHandlerService.js
> bin\components\nsWebHandlerApp.js
>+bin/components/aboutCertError.js
"\"s perhaps? Also might want it in alphabetical order.

>diff --git a/suite/themes/classic/communicator/aboutCertError.css b/suite/themes/classic/communicator/aboutCertError.css
Lots of this resembles netError.css, should we @import it?

>+  background : url("chrome://communicator/skin/section_expanded.png") left 0 no-repeat;
Nit: no space before :

>diff --git a/suite/themes/classic/communicator/section_collapsed.png b/suite/themes/classic/communicator/section_collapsed.png
I'm wondering whether the twisties currently available at suite/themes/classic/messenger/icons/twisty-*.png are more suitable.

>diff --git a/suite/themes/modern/communicator/aboutCertError.css b/suite/themes/modern/communicator/aboutCertError.css
You can't just reuse the Classic css for Modern! Note that Modern's twisties are suite/themes/modern/global/tree/twisty-*.gif
Comment 15 Misak Khachatryan 2008-11-25 06:16:49 PST
Created attachment 349950 [details] [diff] [review]
fixed almost all Neil comments

I fixed all what understood :)

>>+        var okHost = link.getAttribute("title");
>We already know this too, but you might want to move the variable so we can
>reuse it more often.
this i didn't understood, please explain.

>>-  if (/^about:neterror\?e=nssBadCert/.test(ownerDoc.documentURI)) {
>I wonder whether we should keep this for power users wanting the old page.
Please check, i'm not sure what i'v done here.

>>+  if (/^about:certerror/.test(ownerDoc.documentURI)) {
>You lost the \?
please explain

>Lots of this resembles netError.css, should we @import it?
I need help here, i don't know css.

Also, in modern theme the policeman icon missing, please suggest replacement icon to put instead of "chrome://global/skin/icons/sslWarning.png"
Comment 16 neil@parkwaycc.co.uk 2008-11-25 07:33:39 PST
(In reply to comment #15)
>>>+        var okHost = link.getAttribute("title");
>>We already know this too, but you might want to move the variable so we can
>>reuse it more often.
>this i didn't understood, please explain.
I'll comment in a separate review.

>>>-  if (/^about:neterror\?e=nssBadCert/.test(ownerDoc.documentURI)) {
>>I wonder whether we should keep this for power users wanting the old page.
>Please check, i'm not sure what i'v done here.
Mostly OK but you didn't need the extra ()s.

>>>+  if (/^about:certerror/.test(ownerDoc.documentURI)) {
>>You lost the \?
>please explain
^about:certerror\?

>>Lots of this resembles netError.css, should we @import it?
>I need help here, i don't know css.
OK, we'll leave this.

>Also, in modern theme the policeman icon missing, please suggest replacement
>icon to put instead of "chrome://global/skin/icons/sslWarning.png"
Try chrome://global/skin/icons/alert-security.gif
Comment 17 neil@parkwaycc.co.uk 2008-11-25 08:27:43 PST
Comment on attachment 349950 [details] [diff] [review]
fixed almost all Neil comments

Well there's not much left to do here, although I forgot to ask you to check for trailing whitespace (git-apply claims there are 14 errors).

>diff --git a/suite/common/certError.css b/suite/common/certError.css
Sorry, I meant that I'm not convinced that adding this file is right, and the CSS should be put in skin only. (Anyone else feel free to comment.)

>+        var node = document.evaluate('//text()[string()="#1"]', intro, null,
>+                                     XPathResult.ANY_UNORDERED_NODE_TYPE, null).singleNodeValue;
Hmm, this hasn't indented well, try wrapping the null).singleNodeValue on to the next line.

>+        if (getCSSClass() == "expertBadCert") {
>+          toggle('expertContent');
>+        }
Don't need those {}s

>+        // Rather than textContent, we need to treat description as HTML
>+        var sd = document.getElementById("technicalContentText");
>+        if (sd) {
if (!sd)
  return;
(and outdent the next few lines)

>+          if(!result)
>+            return;
Nit: missing space after if.
You need to set sd.textContent = desc; before returning.

>+        // Now create the link itself
>+        var link = document.createElement("a");
>+        link.setAttribute("id", "cert_domain_link");
>+        link.setAttribute("title", result[1]);
>+        link.appendChild(document.createTextNode(result[1]));
>+        sd.appendChild(link);
>+          
>+        // Finally, append text for anything after the closing </a>
>+        sd.appendChild(document.createTextNode(desc.slice(desc.indexOf("</a>") + "</a>".length)));
>+
>+        if (!link)
>+          return;
>+        
>+        var okHost = link.getAttribute("title");
First, link now always exists, so need to check it and return.
Secondly, we set the title attribute 10 lines ago, so if you set okHost to result[1] first you can use that instead of result[1] later as well.

>+          link.href = proto + "\/\/" + okHost;
Don't need the \s here, "//" will do (2x).

>+  if ((/^about:neterror\?e=nssBadCert/.test(ownerDoc.documentURI)) ||
>+      (/^about:certerror/.test(ownerDoc.documentURI))) {
I've already commented about this but don't forget ;-)

>diff --git a/suite/themes/modern/communicator/certError.css b/suite/themes/modern/communicator/certError.css
This still needs to look more like the existing netError.css (same colours). [Maybe I confused you with the twisties, but that was just so you found them]
Comment 18 Misak Khachatryan 2008-11-26 06:06:47 PST
Created attachment 350144 [details] [diff] [review]
fixed comments

I've fixed all comments, but
>>diff --git a/suite/common/certError.css b/suite/common/certError.css
>Sorry, I meant that I'm not convinced that adding this file is right, and the
>CSS should be put in skin only. (Anyone else feel free to comment.)

Leaving this for authorized people.

>>+          if(!result)
>>+            return;
>Nit: missing space after if.
>You need to set sd.textContent = desc; before returning.

Please check it if i understood you correctly.

>First, link now always exists, so need to check it and return.
maybe you mean "so no need to check it and return." ?
i've deleted these lines :)

>+        if (!link)
>+          return;


Also i personally don't like twisties vertical alignment, but i can't figure out how to center it.
Comment 19 neil@parkwaycc.co.uk 2008-11-27 05:00:30 PST
(In reply to comment #18)
> Also i personally don't like twisties vertical alignment, but i can't figure
> out how to center it.
I think you can use "left center" instead of "left 0"
Comment 20 neil@parkwaycc.co.uk 2008-11-27 05:24:24 PST
Comment on attachment 350144 [details] [diff] [review]
fixed comments

Will get back to you on the other CSS file.

>               if (!aboutNeterr.test(targetDoc.documentURI) ||
>+                  !aboutCerterr.test(targetDoc.documentURI) ||
>                   !uri.schemeIs("chrome")) {
Sorry, this was right before... I think you changed it by mistake; I wanted something else changed (I will comment again on that).

>+        var okHost = result[1];
>+        sd.textContent = desc.slice(0, result.index);
>+
>+        // Now create the link itself
>+        var link = document.createElement("a");
>+        link.setAttribute("id", "cert_domain_link");
>+        link.setAttribute("title", okHost);
>+        link.appendChild(document.createTextNode(result[1]));
You missed this chance to reuse okHost.

>+        /* case #1: 
Nit: trailing space. (You found all the others already.)

>+  if ((/^about:neterror\?e=nssBadCert/.test(ownerDoc.documentURI)) ||
>+      (/^about:certerror\?/.test(ownerDoc.documentURI))) {
These are the lines with too many ()s.

>+  border: 1px solid #494F5D; /* pale yellow extracted from yellow passport icon */
This should be #E8DB99

>+  background : url("chrome://global/skin/tree/twisty-open.gif") left 0 no-repeat;
I tried left center and it definitely looks better. I'm picking on this particular version because there shouldn't be a space before the :
Comment 21 neil@parkwaycc.co.uk 2008-11-27 05:41:40 PST
Comment on attachment 350144 [details] [diff] [review]
fixed comments

>diff --git a/suite/common/certError.css b/suite/common/certError.css
Right, so it's been decided not to bother with this file, but merge the rules into the two theme files instead. (Don't forget to remove the <link> etc.)

>+div[collapsed] > p,
>+div[collapsed] > div {
While you're merging, change these to
#technicalContent[collapsed] > p,
#expertContent[collapsed] > div {
Comment 22 Misak Khachatryan 2008-11-27 08:38:46 PST
Created attachment 350341 [details] [diff] [review]
removed, global cerError.css, fixed all comments

All nits fixed.
Comment 23 neil@parkwaycc.co.uk 2008-11-27 08:59:15 PST
Comment on attachment 350341 [details] [diff] [review]
removed, global cerError.css, fixed all comments

Almost there, just a couple of nits:

>+  border: 1px solid #E8DB99; /* pale yellow extracted from yellow passport icon */
Oops. This one wasn't supposed to change, it should stay #FFBD09

>+#technicalContent[collapsed] > h2,
>+#expertContent[collapsed] > h2{
>+  background-image: url("chrome://messenger/skin/icons/twisty-clsd.png") left center;
Repeating left center here is wrong. Also, a space before the { please.

>+#technicalContentText {
>+  overflow: auto;
>+  white-space: pre-wrap;
>+}
>+
>+#technicalContent[collapsed] > p,
>+#expertContent[collapsed] > div {
>+  display: none;
>+}
Please swap these two blocks over.

>+  border: 1px solid #494F5D; /* pale yellow extracted from yellow passport icon */
This is the one that I wanted to be #E8DB99

>+#technicalContent[collapsed] > h2,
>+#expertContent[collapsed] > h2{
>+  background-image: url("chrome://global/skin/tree/twisty-clsd.gif") left center;
>+}
Same problems as in Classic.

>+
>+#technicalContentText {
>+  overflow: auto;
>+  white-space: pre-wrap;
>+}
>+
>+#technicalContent[collapsed] > p,
>+#expertContent[collapsed] > div {
>+  display: none;
>+}
Same swap as in Classic.
Comment 24 Misak Khachatryan 2008-11-27 10:59:08 PST
Created attachment 350366 [details] [diff] [review]
final patch

> >+#technicalContent[collapsed] > h2,
> >+#expertContent[collapsed] > h2{
> >+  background-image: url("chrome://messenger/skin/icons/twisty-clsd.png") left center;
>Repeating left center here is wrong. Also, a space before the { please.

It should be correct in previous patch, maybe i messed with mq.
Comment 25 neil@parkwaycc.co.uk 2008-11-27 16:05:54 PST
Comment on attachment 350366 [details] [diff] [review]
final patch

Some nits I overlooked before but can be fixed on checkin if necessary:

>+   content/communicator/certError.xhtml
>    content/communicator/askViewZoom.xul
>    content/communicator/askViewZoom.js
>    content/communicator/browserBindings.xul
I totally failed to notice that this should of course be in alphabetical order.

> EXTRA_COMPONENTS = \
> 	nsSuiteGlue.js \
>+       nsAboutCertError.js \
> 	$(NULL)
As should this, and it should be a tab and not spaces.

>+#expertContent[collapsed] > h2{
Speaking of spaces, both CSS files are still missing a space before {
Comment 26 Robert Kaiser 2008-11-27 16:45:11 PST
Comment on attachment 350366 [details] [diff] [review]
final patch

Even if this is probably not as low risk as usual patches in a frozen period should be, I'd see this as high enough reward, and as something that should get alpha testing, so a=me :)
Comment 27 Misak Khachatryan 2008-11-28 00:13:35 PST
Before I upload final patch for checkin, i need to state two things:

1. This patch makes chrome contentaccessible. Bug 464024 should be resolved to make correct final patch.
2. It has bad behavior - when mailnews open, both cert error dialog and page showing up. I'm quite sure that this patch is not guilty, something related elsewhere, someone more experienced should help me to fix it, but that should go to separate bug IMHO.

So, I can upload final patch for checkin, if you can live with those things above.
Comment 28 Robert Kaiser 2008-11-28 04:23:42 PST
1) is OK with me if it is OK with Neil.
2) is something I'd not like to have in the alpha release, Neil, are you seeing this as well?
Comment 29 Robert Kaiser 2008-11-28 07:26:19 PST
As a note, I just tried the patch here, and I don't see both dialog and page here when trying to access https://amazon.com/ with the most recent patch in here.
Comment 30 Misak Khachatryan 2008-11-28 07:50:30 PST
Created attachment 350483 [details] [diff] [review]
for checkin

I did test on clean profile and also didn't notice any bad behavior. Seems my profile from SeaMonkey 1.0 time too old for this, once i figure out what my preference causing that bad behavior, i'll file new bug.

So here is final patch for checkin.
Comment 31 Robert Kaiser 2008-11-28 08:39:29 PST
Pushed as http://hg.mozilla.org/comm-central/rev/1c26ea5bb33d per IRC request.

Note You need to log in before you can comment on or make changes to this bug.