Bug 1547695 - De-dupe #errorTryAgain on about:neterror/certerror. r=prathiksha
authorJohann Hofmann <jhofmann@mozilla.com>
Mon, 13 May 2019 19:38:03 +0000
changeset 532469 7d59f5e3db1b3f0cbd4b095752535c2d5ef09208
parent 532468 5abfe3566f38af45a7ef67964d10f8b501ba90f6
child 532470 0eff48265098e8bac99128577b2baccde799611e
push id11268
push usercsabou@mozilla.com
push dateTue, 14 May 2019 15:24:22 +0000
treeherdermozilla-beta@5fb7fcd568d6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersprathiksha
bugs1547695
milestone68.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 1547695 - De-dupe #errorTryAgain on about:neterror/certerror. r=prathiksha We were using the same ID on two elements, which kind of messed up things everywhere our code reasonably expected only one element of the kind to exist. We just use a class name now. This also cleans up #advancedPanelErrorTryAgain which worked around this issue by using a different ID. Differential Revision: https://phabricator.services.mozilla.com/D30298
browser/actors/NetErrorChild.jsm
browser/base/content/aboutNetError.js
browser/base/content/aboutNetError.xhtml
browser/base/content/test/about/browser.ini
browser/base/content/test/about/browser_bug435325.js
browser/base/content/test/general/browser.ini
browser/base/content/test/general/browser_bug435325.js
browser/themes/shared/aboutNetError.css
toolkit/components/places/tests/browser/browser_bug680727.js
--- a/browser/actors/NetErrorChild.jsm
+++ b/browser/actors/NetErrorChild.jsm
@@ -815,17 +815,18 @@ class NetErrorChild extends ActorChild {
   onClick(event) {
     let {documentURI} = event.target.ownerDocument;
 
     let elmId = event.originalTarget.getAttribute("id");
     if (elmId == "returnButton") {
       this.mm.sendAsyncMessage("Browser:SSLErrorGoBack", {});
       return;
     }
-    if (elmId != "errorTryAgain" || !/e=netOffline/.test(documentURI)) {
+
+    if (!event.originalTarget.classList.contains("try-again") || !/e=netOffline/.test(documentURI)) {
       return;
     }
     // browser front end will handle clearing offline mode and refreshing
     // the page *if* we're in offline mode now. Otherwise let the error page
     // handle the click.
     if (Services.io.offline) {
       event.preventDefault();
       this.mm.sendAsyncMessage("Browser:EnableOnlineMode", {});
--- a/browser/base/content/aboutNetError.js
+++ b/browser/base/content/aboutNetError.js
@@ -68,17 +68,17 @@ function showCertificateErrorReporting()
 function showPrefChangeContainer() {
   const panel = document.getElementById("prefChangeContainer");
   panel.style.display = "block";
   document.getElementById("netErrorButtonContainer").style.display = "none";
   document.getElementById("prefResetButton").addEventListener("click", function resetPreferences(e) {
     const event = new CustomEvent("AboutNetErrorResetPreferences", {bubbles: true});
     document.dispatchEvent(event);
   });
-  addAutofocus("prefResetButton", "beforeend");
+  addAutofocus("#prefResetButton", "beforeend");
 }
 
 function setupAdvancedButton() {
   // Get the hostname and add it to the panel
   var panel = document.getElementById("badCertAdvancedPanel");
   for (var span of panel.querySelectorAll("span.hostname")) {
     span.textContent = document.location.hostname;
   }
@@ -202,17 +202,17 @@ function initPage() {
     initPageCaptivePortal();
     return;
   }
   if (gIsCertError) {
     initPageCertError();
     updateContainerPosition();
     return;
   }
-  addAutofocus("errorTryAgain");
+  addAutofocus("#netErrorButtonContainer > .try-again");
 
   document.body.classList.add("neterror");
 
   var ld = document.getElementById("errorLongDesc");
   if (ld) {
     // eslint-disable-next-line no-unsanitized/property
     ld.innerHTML = errDesc.innerHTML;
   }
@@ -278,17 +278,17 @@ function initPage() {
 
   var event = new CustomEvent("AboutNetErrorLoad", {bubbles: true});
   document.dispatchEvent(event);
 
   if (err == "inadequateSecurityError" || err == "blockedByPolicy") {
     // Remove the "Try again" button from pages that don't need it.
     // For HTTP/2 inadequate security or pages blocked by policy, trying
     // again won't help.
-    document.getElementById("errorTryAgain").style.display = "none";
+    document.getElementById("netErrorButtonContainer").style.display = "none";
 
     var container = document.getElementById("errorLongDesc");
     for (var span of container.querySelectorAll("span.hostname")) {
       span.textContent = document.location.hostname;
     }
   }
 }
 
@@ -313,33 +313,33 @@ function updateContainerPosition() {
 
 function initPageCaptivePortal() {
   document.body.className = "captiveportal";
   document.getElementById("openPortalLoginPageButton")
           .addEventListener("click", () => {
     RPMSendAsyncMessage("Browser:OpenCaptivePortalPage");
   });
 
-  addAutofocus("openPortalLoginPageButton");
+  addAutofocus("#openPortalLoginPageButton");
   setupAdvancedButton();
 
   // When the portal is freed, an event is sent by the parent process
   // that we can pick up and attempt to reload the original page.
   RPMAddMessageListener("AboutNetErrorCaptivePortalFreed", () => {
     document.location.reload();
   });
 }
 
 function initPageCertError() {
   document.body.classList.add("certerror");
   for (let host of document.querySelectorAll(".hostname")) {
     host.textContent = document.location.hostname;
   }
 
-  addAutofocus("returnButton");
+  addAutofocus("#returnButton");
   setupAdvancedButton();
 
   document.getElementById("learnMoreContainer").style.display = "block";
 
   let checkbox = document.getElementById("automaticallyReportInFuture");
   checkbox.addEventListener("change", function({target: {checked}}) {
     document.dispatchEvent(new CustomEvent("AboutNetErrorSetAutomatic", {
       detail: checked,
@@ -366,32 +366,28 @@ function initPageCertError() {
 }
 
 /* Only do autofocus if we're the toplevel frame; otherwise we
    don't want to call attention to ourselves!  The key part is
    that autofocus happens on insertion into the tree, so we
    can remove the button, add @autofocus, and reinsert the
    button.
 */
-function addAutofocus(buttonId, position = "afterbegin") {
+function addAutofocus(selector, position = "afterbegin") {
   if (window.top == window) {
-      var button = document.getElementById(buttonId);
+      var button = document.querySelector(selector);
       var parent = button.parentNode;
       button.remove();
       button.setAttribute("autofocus", "true");
       parent.insertAdjacentElement(position, button);
   }
 }
 
-let errorTryAgain = document.getElementById("errorTryAgain");
-errorTryAgain.addEventListener("click", function() {
-  retryThis(this);
-});
-
-let advancedPanelErrorTryAgain = document.getElementById("advancedPanelErrorTryAgain");
-advancedPanelErrorTryAgain.addEventListener("click", function() {
-  retryThis(this);
-});
+for (let button of document.querySelectorAll(".try-again")) {
+  button.addEventListener("click", function() {
+    retryThis(this);
+  });
+}
 
 // Note: It is important to run the script this way, instead of using
 // an onload handler. This is because error pages are loaded as
 // LOAD_BACKGROUND, which means that onload handlers will not be executed.
 initPage();
--- a/browser/base/content/aboutNetError.xhtml
+++ b/browser/base/content/aboutNetError.xhtml
@@ -191,32 +191,32 @@
         <div id="prefChangeContainer" class="button-container">
           <p>&prefReset.longDesc;</p>
           <button id="prefResetButton" class="primary">&prefReset.label;</button>
         </div>
 
         <div id="certErrorAndCaptivePortalButtonContainer" class="button-container">
           <button id="returnButton" class="primary" data-telemetry-id="return_button_top">&returnToPreviousPage1.label;</button>
           <button id="openPortalLoginPageButton" class="primary">&openPortalLoginPage.label2;</button>
-          <button id="errorTryAgain" class="primary">&retry.label;</button>
+          <button class="primary try-again">&retry.label;</button>
           <button id="advancedButton" data-telemetry-id="advanced_button">&advanced2.label;</button>
         </div>
       </div>
 
       <div id="netErrorButtonContainer" class="button-container">
-        <button id="errorTryAgain" class="primary">&retry.label;</button>
+        <button class="primary try-again">&retry.label;</button>
       </div>
 
       <div id="advancedPanelContainer">
         <div id="badCertAdvancedPanel" class="advanced-panel">
           <p id="badCertTechnicalInfo"/>
           <a id="viewCertificate" href="javascript:void(0)">&viewCertificate.label;</a>
           <div id="advancedPanelButtonContainer" class="button-container">
             <button id="advancedPanelReturnButton" class="primary" data-telemetry-id="return_button_adv">&returnToPreviousPage1.label;</button>
-            <button id="advancedPanelErrorTryAgain" class="primary">&retry.label;</button>
+            <button class="primary try-again">&retry.label;</button>
             <div class="exceptionDialogButtonContainer">
               <button id="exceptionDialogButton" data-telemetry-id="exception_button">&securityOverride.exceptionButton1Label;</button>
             </div>
           </div>
         </div>
 
         <div id="certificateErrorReporting">
             <p class="toggle-container-with-text">
--- a/browser/base/content/test/about/browser.ini
+++ b/browser/base/content/test/about/browser.ini
@@ -20,8 +20,10 @@ prefs =
 [browser_aboutHome_search_searchbar.js]
 [browser_aboutHome_search_suggestion.js]
 skip-if = os == "mac" || (os == "linux" && (!debug || bits == 64)) || (os == 'win' && os_version == '10.0' && bits == 64 && !debug) # Bug 1399648, bug 1402502
 [browser_aboutHome_search_telemetry.js]
 [browser_aboutNetError.js]
 [browser_aboutStopReload.js]
 [browser_aboutSupport.js]
 [browser_aboutSupport_newtab_security_state.js]
+[browser_bug435325.js]
+skip-if = verify && !debug && os == 'mac'
rename from browser/base/content/test/general/browser_bug435325.js
rename to browser/base/content/test/about/browser_bug435325.js
--- a/browser/base/content/test/general/browser_bug435325.js
+++ b/browser/base/content/test/about/browser_bug435325.js
@@ -25,17 +25,17 @@ add_task(async function checkSwitchPageT
     // Re-enable the proxy so example.com is resolved to localhost, rather than
     // the actual example.com.
     await SpecialPowers.pushPrefEnv({"set": [["network.proxy.type", proxyPrefValue]]});
     let changeObserved = TestUtils.topicObserved("network:offline-status-changed");
 
     // Click on the 'Try again' button.
     await ContentTask.spawn(browser, null, async function() {
       ok(content.document.documentURI.startsWith("about:neterror?e=netOffline"), "Should be showing error page");
-      content.document.getElementById("errorTryAgain").click();
+      content.document.querySelector("#netErrorButtonContainer > .try-again").click();
     });
 
     await changeObserved;
     ok(!Services.io.offline, "After clicking the 'Try Again' button, we're back online.");
   });
 });
 
 registerCleanupFunction(function() {
--- a/browser/base/content/test/general/browser.ini
+++ b/browser/base/content/test/general/browser.ini
@@ -101,19 +101,16 @@ skip-if = true # bug 428712
 [browser_bug424101.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_bug427559.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_bug431826.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_bug432599.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
-[browser_bug435325.js]
-skip-if = verify && !debug && os == 'mac'
-# DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_bug441778.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_bug455852.js]
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_bug462289.js]
 skip-if = toolkit == "cocoa"
 # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
 [browser_bug462673.js]
--- a/browser/themes/shared/aboutNetError.css
+++ b/browser/themes/shared/aboutNetError.css
@@ -70,37 +70,34 @@ button:disabled {
 body:not(.neterror) #certErrorAndCaptivePortalButtonContainer {
   display: flex;
 }
 
 body:not(.neterror) #netErrorButtonContainer {
   display: none;
 }
 
-#errorTryAgain {
+#netErrorButtonContainer > .try-again {
   margin-top: 1.2em;
 }
 
 #advancedButton {
   display: none;
 }
 
 body.captiveportal #returnButton {
   display: none;
 }
 
 body:not(.captiveportal) #openPortalLoginPageButton {
   display: none;
 }
 
-body:not(.clockSkewError) #errorTryAgain {
-  display: none;
-}
-
-body:not(.clockSkewError) #advancedPanelErrorTryAgain {
+body:not(.clockSkewError) #certErrorAndCaptivePortalButtonContainer > .try-again,
+body:not(.clockSkewError) #advancedPanelContainer .try-again {
   display: none;
 }
 
 #openPortalLoginPageButton {
   margin-inline-start: 0;
 }
 
 body:not(.neterror) #advancedButton {
@@ -228,31 +225,30 @@ body:not(.neterror) #advancedButton {
 .clockSkewError #returnButton {
   display: none;
 }
 
 .clockSkewError #advancedButton {
   display: none;
 }
 
-.clockSkewError #advancedPanelErrorTryAgain,
-.clockSkewError #errorTryAgain {
+.clockSkewError .try-again {
   display: block;
   margin-top: 0.3em;
 }
 
 .clockSkewError #exceptionDialogButton {
   display: none;
 }
 
 .clockSkewError #advancedPanelReturnButton {
   display: none;
 }
 
-.malformedURI #errorTryAgain {
+.malformedURI .try-again {
   display: none;
 }
 
 #wrongSystemTimePanel {
   display: none;
 }
 
 #wrongSystemTimeWithoutReferencePanel {
--- a/toolkit/components/places/tests/browser/browser_bug680727.js
+++ b/toolkit/components/places/tests/browser/browser_bug680727.js
@@ -62,19 +62,19 @@ function errorAsyncListener(aURI, aIsVis
 
   // Now press the "Try Again" button, with offline mode off.
   Services.io.offline = false;
 
   BrowserTestUtils.waitForContentEvent(ourTab.linkedBrowser, "DOMContentLoaded")
                   .then(reloadListener);
 
   ContentTask.spawn(ourTab.linkedBrowser, null, function() {
-    Assert.ok(content.document.getElementById("errorTryAgain"),
-      "The error page has got a #errorTryAgain element");
-    content.document.getElementById("errorTryAgain").click();
+    Assert.ok(content.document.querySelector("#netErrorButtonContainer > .try-again"),
+      "The error page has got a .try-again element");
+    content.document.querySelector("#netErrorButtonContainer > .try-again").click();
   });
 }
 
 // ------------------------------------------------------------------------------
 // listen to reload of neterror.
 function reloadListener() {
   // This listener catches "DOMContentLoaded" on being called
   // nsIWPL::onLocationChange(...). That is right *AFTER*