Bug 1724230 - rely on flexbox instead of manual positioning for network/cert error page layout and fix warning icon going off-screen, r=johannh
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Tue, 31 Aug 2021 11:36:43 +0000
changeset 590310 2520566ff8668db3ad579e3d991354e7d55b170f
parent 590309 08e0264ecf61c14f6c88ca2104086ae3304b1e8a
child 590311 1ca1d6846228017a978e0f1c6e4856e35337d046
push id148866
push usergijskruitbosch@gmail.com
push dateTue, 31 Aug 2021 11:39:04 +0000
treeherderautoland@2520566ff866 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjohannh
bugs1724230
milestone93.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1724230 - rely on flexbox instead of manual positioning for network/cert error page layout and fix warning icon going off-screen, r=johannh Differential Revision: https://phabricator.services.mozilla.com/D122913
browser/base/content/certerror/aboutNetError.js
browser/themes/shared/aboutNetError.css
browser/themes/shared/error-pages.css
--- a/browser/base/content/certerror/aboutNetError.js
+++ b/browser/base/content/certerror/aboutNetError.js
@@ -323,17 +323,16 @@ function initPage() {
     }
   }
 
   if (gIsCertError) {
     if (showCaptivePortalUI) {
       initPageCaptivePortal();
     } else {
       initPageCertError();
-      updateContainerPosition();
     }
 
     initCertErrorPageActions();
     setTechnicalDetailsOnCertError();
     return;
   }
 
   setFocus("#netErrorButtonContainer > .try-again");
@@ -589,38 +588,16 @@ async function setNetErrorMessageFromCod
   });
 
   let desc2 = document.getElementById("errorShortDescText2");
   document.l10n.setAttributes(desc2, "cert-error-code-prefix", {
     error: errorCodeStr,
   });
 }
 
-// This function centers the error container after its content updates.
-// It is currently duplicated in NetErrorChild.jsm to avoid having to do
-// async communication to the page that would result in flicker.
-// TODO(johannh): Get rid of this duplication.
-function updateContainerPosition() {
-  let textContainer = document.getElementById("text-container");
-  // Using the vh CSS property our margin adapts nicely to window size changes.
-  // Unfortunately, this doesn't work correctly in iframes, which is why we need
-  // to manually compute the height there.
-  if (window.parent == window) {
-    textContainer.style.marginTop = `calc(50vh - ${textContainer.clientHeight /
-      2}px)`;
-  } else {
-    let offset =
-      document.documentElement.clientHeight / 2 -
-      textContainer.clientHeight / 2;
-    if (offset > 0) {
-      textContainer.style.marginTop = `${offset}px`;
-    }
-  }
-}
-
 function initPageCaptivePortal() {
   document.body.className = "captiveportal";
   document
     .getElementById("openPortalLoginPageButton")
     .addEventListener("click", () => {
       RPMSendAsyncMessage("Browser:OpenCaptivePortalPage");
     });
 
@@ -831,17 +808,16 @@ function setCertErrorDetails(event) {
       if (es) {
         // eslint-disable-next-line no-unsanitized/property
         es.innerHTML = errWhatToDo.innerHTML;
       }
       if (est) {
         // eslint-disable-next-line no-unsanitized/property
         est.innerHTML = errWhatToDoTitle.innerHTML;
       }
-      updateContainerPosition();
       break;
 
     // This error code currently only exists for the Symantec distrust
     // in Firefox 63, so we add copy explaining that to the user.
     // In case of future distrusts of that scale we might need to add
     // additional parameters that allow us to identify the affected party
     // without replicating the complex logic from certverifier code.
     case "MOZILLA_PKIX_ERROR_ADDITIONAL_POLICY_CONSTRAINT_FAILED":
@@ -856,17 +832,16 @@ function setCertErrorDetails(event) {
 
       let adminDesc = document.createElement("p");
       document.l10n.setAttributes(
         adminDesc,
         "cert-error-symantec-distrust-admin"
       );
 
       learnMoreLink.href = baseURL + "symantec-warning";
-      updateContainerPosition();
       break;
 
     case "MOZILLA_PKIX_ERROR_MITM_DETECTED":
       let autoEnabledEnterpriseRoots = RPMGetBoolPref(
         "security.enterprise_roots.auto-enabled",
         false
       );
       if (mitmPrimingEnabled && autoEnabledEnterpriseRoots) {
@@ -889,18 +864,16 @@ function setCertErrorDetails(event) {
       desc = document.getElementById("ed_mitm");
       // eslint-disable-next-line no-unsanitized/property
       document.getElementById("errorShortDescText").innerHTML = desc.innerHTML;
 
       // eslint-disable-next-line no-unsanitized/property
       es.innerHTML = errWhatToDo.innerHTML;
       // eslint-disable-next-line no-unsanitized/property
       est.innerHTML = errWhatToDoTitle.innerHTML;
-
-      updateContainerPosition();
       break;
 
     case "MOZILLA_PKIX_ERROR_SELF_SIGNED_CERT":
       learnMoreLink.href = baseURL + "security-error";
       break;
 
     // In case the certificate expired we make sure the system clock
     // matches the remote-settings service (blocklist via Kinto) ping time
@@ -1016,17 +989,16 @@ function setCertErrorDetails(event) {
           // eslint-disable-next-line no-unsanitized/property
           es.innerHTML = errWhatToDo.innerHTML;
         }
         if (est) {
           // eslint-disable-next-line no-unsanitized/property
           est.textContent = errWhatToDoTitle.textContent;
           est.style.fontWeight = "bold";
         }
-        updateContainerPosition();
       }
       break;
   }
 }
 
 // The optional argument is only here for testing purposes.
 async function setTechnicalDetailsOnCertError(
   failedCertInfo = document.getFailedCertSecurityInfo()
--- a/browser/themes/shared/aboutNetError.css
+++ b/browser/themes/shared/aboutNetError.css
@@ -17,17 +17,16 @@ body {
 @media (prefers-contrast) {
   body {
     --warning-color: var(--in-content-page-color);
   }
 }
 
 body.certerror {
   width: 100%;
-  justify-content: normal;
 }
 
 body.captiveportal .title {
   background-image: url("wifi.svg");
 }
 
 body.certerror .title {
   background-image: url("chrome://global/skin/icons/warning.svg");
@@ -128,16 +127,17 @@ body:not(.neterror) #advancedButton {
 
 #blockingErrorReporting {
   display: none;
   padding-bottom: 10px;
 }
 
 #advancedPanelContainer {
   width: 100%;
+  position: absolute;
   left: 0;
 }
 
 .advanced-panel {
   /* Hidden until the link is clicked */
   display: none;
   background-color: var(--in-content-box-background);
   border: 1px solid var(--in-content-box-border-color);
@@ -264,13 +264,20 @@ body:not(.neterror) #advancedButton {
 @media (max-width: 970px) {
   body.certerror .title {
     /* !important is necessary here until Bug 1723718 is resolved */
     background-image: url("chrome://global/skin/icons/warning.svg") !important;
     background-position: top left;
     padding-top: 60px;
     margin-top: -60px;
   }
+  /* Reduce the negative margin on small viewport sizes to avoid exceeding the
+   * 38px body vertical padding error-pages.css sets, leaving 8px space: */
+  @media (max-height: 480px) {
+    body.certerror .title {
+      margin-top: -30px;
+    }
+  }
 
   body.certerror .title:dir(rtl) {
     background-position-x: right;
   }
 }
--- a/browser/themes/shared/error-pages.css
+++ b/browser/themes/shared/error-pages.css
@@ -82,21 +82,19 @@ body {
   /* When the critter is hidden, also use the full screen
    * size for the error text */
   .illustrated #text-container {
     margin: unset;
     padding-inline-start: 0;
   }
 }
 
-/* For small window height, shift the stripes up by 10px.
- * We could just change the background size, but that changes
- * the angle of the stripes so just shifting up is easier. */
 @media only screen and (max-height: 480px) {
   body {
-    background-position: 10px -10px;
+    /* Note: if you change the top padding, also update the image positioning
+     * media query in aboutNetError.css for the certificate error case. */
     padding-top: 38px;
     /* We get rid of bottom padding for width < 640px, but
      * for height < 480px a bit of space between the content
      * and the viewport edge is nice. */
     padding-bottom: 38px;
   }
 }