Bug 1344431 - Tell parent the non-reader-able reader page is not readable, r=Gijs a=gchang
authorTimothy Guan-tin Chien <timdream@gmail.com>
Mon, 06 Mar 2017 16:31:47 +0800
changeset 395271 55b98dce58cbc80e733012ae7922f1f6214b2d81
parent 395270 af228c5bb2a60f45c04411cdd41025af3785a285
child 395272 d5511eaf29c9825b9887f596f37fb97aee8c0916
push id1468
push userasasaki@mozilla.com
push dateMon, 05 Jun 2017 19:31:07 +0000
treeherdermozilla-release@0641fc6ee9d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs, gchang
bugs1344431, 1260276, 1126967
milestone54.0a2
Bug 1344431 - Tell parent the non-reader-able reader page is not readable, r=Gijs a=gchang Although regression window testing pin this to bug 1260276, I believe this is a regression from bug 1126967. Bug 1260276 just make it more visible because we stop automatically redirect users to the original page. This patch fix the bug by checking if the current page is in readerable state (i.e. not error state), and send the message accordingly. MozReview-Commit-ID: B5UJcPvVlAc
browser/base/content/tab-content.js
mobile/android/chrome/content/content.js
toolkit/components/reader/AboutReader.jsm
toolkit/components/reader/test/browser.ini
toolkit/components/reader/test/browser_readerMode.js
toolkit/components/reader/test/readerModeNonArticle.html
--- a/browser/base/content/tab-content.js
+++ b/browser/base/content/tab-content.js
@@ -255,17 +255,17 @@ var AboutPrivateBrowsingListener = {
   },
 };
 AboutPrivateBrowsingListener.init(this);
 
 var AboutReaderListener = {
 
   _articlePromise: null,
 
-  _isLeavingReaderMode: false,
+  _isLeavingReaderableReaderMode: false,
 
   init() {
     addEventListener("AboutReaderContentLoaded", this, false, true);
     addEventListener("DOMContentLoaded", this, false);
     addEventListener("pageshow", this, false);
     addEventListener("pagehide", this, false);
     addMessageListener("Reader:ToggleReaderMode", this);
     addMessageListener("Reader:PushState", this);
@@ -273,17 +273,17 @@ var AboutReaderListener = {
 
   receiveMessage(message) {
     switch (message.name) {
       case "Reader:ToggleReaderMode":
         if (!this.isAboutReader) {
           this._articlePromise = ReaderMode.parseDocument(content.document).catch(Cu.reportError);
           ReaderMode.enterReaderMode(docShell, content);
         } else {
-          this._isLeavingReaderMode = true;
+          this._isLeavingReaderableReaderMode = this.isReaderableAboutReader;
           ReaderMode.leaveReaderMode(docShell, content);
         }
         break;
 
       case "Reader:PushState":
         this.updateReaderButton(!!(message.data && message.data.isArticle));
         break;
     }
@@ -291,16 +291,21 @@ var AboutReaderListener = {
 
   get isAboutReader() {
     if (!content) {
       return false;
     }
     return content.document.documentURI.startsWith("about:reader");
   },
 
+  get isReaderableAboutReader() {
+    return this.isAboutReader &&
+      !content.document.documentElement.dataset.isError;
+  },
+
   handleEvent(aEvent) {
     if (aEvent.originalTarget.defaultView != content) {
       return;
     }
 
     switch (aEvent.type) {
       case "AboutReaderContentLoaded":
         if (!this.isAboutReader) {
@@ -312,22 +317,22 @@ var AboutReaderListener = {
           sendAsyncMessage("Reader:UpdateReaderButton");
           new AboutReader(global, content, this._articlePromise);
           this._articlePromise = null;
         }
         break;
 
       case "pagehide":
         this.cancelPotentialPendingReadabilityCheck();
-        // this._isLeavingReaderMode is used here to keep the Reader Mode icon
+        // this._isLeavingReaderableReaderMode is used here to keep the Reader Mode icon
         // visible in the location bar when transitioning from reader-mode page
-        // back to the source page.
-        sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: this._isLeavingReaderMode });
-        if (this._isLeavingReaderMode) {
-          this._isLeavingReaderMode = false;
+        // back to the readable source page.
+        sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: this._isLeavingReaderableReaderMode });
+        if (this._isLeavingReaderableReaderMode) {
+          this._isLeavingReaderableReaderMode = false;
         }
         break;
 
       case "pageshow":
         // If a page is loaded from the bfcache, we won't get a "DOMContentLoaded"
         // event, so we need to rely on "pageshow" in this case.
         if (aEvent.persisted) {
           this.updateReaderButton();
--- a/mobile/android/chrome/content/content.js
+++ b/mobile/android/chrome/content/content.js
@@ -17,17 +17,17 @@ var dump = Cu.import("resource://gre/mod
 
 var global = this;
 
 // This is copied from desktop's tab-content.js. See bug 1153485 about sharing this code somehow.
 var AboutReaderListener = {
 
   _articlePromise: null,
 
-  _isLeavingReaderMode: false,
+  _isLeavingReaderableReaderMode: false,
 
   init: function() {
     addEventListener("AboutReaderContentLoaded", this, false, true);
     addEventListener("DOMContentLoaded", this, false);
     addEventListener("pageshow", this, false);
     addEventListener("pagehide", this, false);
     addMessageListener("Reader:ToggleReaderMode", this);
     addMessageListener("Reader:PushState", this);
@@ -36,31 +36,36 @@ var AboutReaderListener = {
   receiveMessage: function(message) {
     switch (message.name) {
       case "Reader:ToggleReaderMode":
         let url = content.document.location.href;
         if (!this.isAboutReader) {
           this._articlePromise = ReaderMode.parseDocument(content.document).catch(Cu.reportError);
           ReaderMode.enterReaderMode(docShell, content);
         } else {
-          this._isLeavingReaderMode = true;
+          this._isLeavingReaderableReaderMode = this.isReaderableAboutReader;
           ReaderMode.leaveReaderMode(docShell, content);
         }
         break;
 
       case "Reader:PushState":
         this.updateReaderButton(!!(message.data && message.data.isArticle));
         break;
     }
   },
 
   get isAboutReader() {
     return content.document.documentURI.startsWith("about:reader");
   },
 
+  get isReaderableAboutReader() {
+    return this.isAboutReader &&
+      !content.document.documentElement.dataset.isError;
+  },
+
   get isErrorPage() {
     return content.document.documentURI.startsWith("about:neterror") ||
         content.document.documentURI.startsWith("about:certerror") ||
         content.document.documentURI.startsWith("about:blocked");
   },
 
   handleEvent: function(aEvent) {
     if (aEvent.originalTarget.defaultView != content) {
@@ -79,22 +84,22 @@ var AboutReaderListener = {
         // valid message will follow. See bug 925983.
         if (content.document.body) {
           new AboutReader(global, content, this._articlePromise);
           this._articlePromise = null;
         }
         break;
 
       case "pagehide":
-        // this._isLeavingReaderMode is used here to keep the Reader Mode icon
+        // this._isLeavingReaderableReaderMode is used here to keep the Reader Mode icon
         // visible in the location bar when transitioning from reader-mode page
         // back to the source page.
-        sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: this._isLeavingReaderMode });
-        if (this._isLeavingReaderMode) {
-          this._isLeavingReaderMode = false;
+        sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: this._isLeavingReaderableReaderMode });
+        if (this._isLeavingReaderableReaderMode) {
+          this._isLeavingReaderableReaderMode = false;
         }
         break;
 
       case "pageshow":
         // If a page is loaded from the bfcache, we won't get a "DOMContentLoaded"
         // event, so we need to rely on "pageshow" in this case.
         if (aEvent.persisted) {
           this.updateReaderButton();
--- a/toolkit/components/reader/AboutReader.jsm
+++ b/toolkit/components/reader/AboutReader.jsm
@@ -757,17 +757,22 @@ AboutReader.prototype = {
     this._contentElement.style.display = "none";
 
     let errorMessage = gStrings.GetStringFromName("aboutReader.loadError");
     this._messageElement.textContent = errorMessage;
     this._messageElement.style.display = "block";
 
     this._doc.title = errorMessage;
 
+    this._doc.documentElement.dataset.isError = true;
+
     this._error = true;
+
+    this._doc.dispatchEvent(
+      new this._win.CustomEvent("AboutReaderContentError", { bubbles: true, cancelable: false }));
   },
 
   // This function is the JS version of Java's StringUtils.stripCommonSubdomains.
   _stripHost(host) {
     if (!host)
       return host;
 
     let start = 0;
--- a/toolkit/components/reader/test/browser.ini
+++ b/toolkit/components/reader/test/browser.ini
@@ -1,12 +1,13 @@
 [DEFAULT]
 support-files = head.js
 [browser_readerMode.js]
 support-files =
+  readerModeNonArticle.html
   readerModeArticle.html
   readerModeArticleHiddenNodes.html
 [browser_readerMode_hidden_nodes.js]
 support-files =
   readerModeArticleHiddenNodes.html
 [browser_readerMode_with_anchor.js]
 support-files =
   readerModeArticle.html
--- a/toolkit/components/reader/test/browser_readerMode.js
+++ b/toolkit/components/reader/test/browser_readerMode.js
@@ -48,18 +48,19 @@ add_task(function* test_reader_button() 
   yield tourPopupShownPromise;
   is_element_visible(readerButton, "Reader mode button is present on a reader-able page");
   ok(UITour.isInfoOnTarget(window, "readerMode-urlBar"),
      "Info panel should be anchored at the reader mode button");
   ok(Services.prefs.getBoolPref("browser.reader.detectedFirstArticle"),
      "Should have detected the first article");
 
   // Switch page into reader mode.
+  let promiseTabLoad = promiseTabLoadEvent(tab);
   readerButton.click();
-  yield promiseTabLoadEvent(tab);
+  yield promiseTabLoad;
   ok(!UITour.isInfoOnTarget(window, "readerMode-urlBar"), "Info panel should have closed");
 
   let readerUrl = gBrowser.selectedBrowser.currentURI.spec;
   ok(readerUrl.startsWith("about:reader"), "about:reader loaded after clicking reader mode button");
   is_element_visible(readerButton, "Reader mode button is present on about:reader");
 
   is(gURLBar.value, readerUrl, "gURLBar value is about:reader URL");
   is(gURLBar.textValue, url.substring("http://".length), "gURLBar is displaying original article URL");
@@ -79,26 +80,45 @@ add_task(function* test_reader_button() 
   let promisePageShow = BrowserTestUtils.waitForContentEvent(tab.linkedBrowser, "pageshow");
   readerButton.click();
   yield promisePageShow;
   is(gBrowser.selectedBrowser.currentURI.spec, url,
     "Back to the original page after clicking active reader mode button");
   ok(gBrowser.selectedBrowser.canGoForward,
     "Moved one step back in the session history.");
 
+  let nonReadableUrl = TEST_PATH + "readerModeNonArticle.html";
+
   // Load a new tab that is NOT reader-able.
   let newTab = gBrowser.selectedTab = gBrowser.addTab();
-  yield promiseTabLoadEvent(newTab, "about:robots");
+  yield promiseTabLoadEvent(newTab, nonReadableUrl);
   yield promiseWaitForCondition(() => readerButton.hidden);
   is_element_hidden(readerButton, "Reader mode button is not present on a non-reader-able page");
 
   // Switch back to the original tab to make sure reader mode button is still visible.
   gBrowser.removeCurrentTab();
   yield promiseWaitForCondition(() => !readerButton.hidden);
   is_element_visible(readerButton, "Reader mode button is present on a reader-able page");
+
+  // Load a new tab in reader mode that is NOT reader-able in the reader mode.
+  newTab = gBrowser.selectedTab = gBrowser.addTab();
+  let promiseAboutReaderError = BrowserTestUtils.waitForContentEvent(newTab.linkedBrowser, "AboutReaderContentError");
+  yield promiseTabLoadEvent(newTab, "about:reader?url=" + nonReadableUrl);
+  yield promiseAboutReaderError;
+  yield promiseWaitForCondition(() => !readerButton.hidden);
+  is_element_visible(readerButton, "Reader mode button is present on about:reader even in error state");
+
+  // Switch page back out of reader mode.
+  promisePageShow = BrowserTestUtils.waitForContentEvent(newTab.linkedBrowser, "pageshow");
+  readerButton.click();
+  yield promisePageShow;
+  is(gBrowser.selectedBrowser.currentURI.spec, nonReadableUrl,
+    "Back to the original non-reader-able page after clicking active reader mode button");
+  yield promiseWaitForCondition(() => readerButton.hidden);
+  is_element_hidden(readerButton, "Reader mode button is not present on a non-reader-able page");
 });
 
 add_task(function* test_getOriginalUrl() {
   let { ReaderMode } = Cu.import("resource://gre/modules/ReaderMode.jsm", {});
   let url = "http://foo.com/article.html";
 
   is(ReaderMode.getOriginalUrl("about:reader?url=" + encodeURIComponent(url)), url, "Found original URL from encoded URL");
   is(ReaderMode.getOriginalUrl("about:reader?foobar"), null, "Did not find original URL from malformed reader URL");
new file mode 100644
--- /dev/null
+++ b/toolkit/components/reader/test/readerModeNonArticle.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>Non article title</title>
+<meta name="description" content="This is the non-article description." />
+</head>
+<body>
+<header>Site header</header>
+<div>
+<h1>Non article title</h1>
+<p>Woot!</p>
+</div>
+</body>
+</html>