Bug 1344431 - Tell parent the non-reader-able reader page is not readable, r=gijs draft
authorTimothy Guan-tin Chien <timdream@gmail.com>
Mon, 06 Mar 2017 16:31:47 +0800
changeset 495113 8a087cddefa0c3f8ab6fb4cd43661dae204024d7
parent 493721 8d026c60151005ad942e3d4389318fe28a0c8c54
child 548288 df904f9be3aa129ea7290dac589bd21a75adab02
push id48238
push userbmo:timdream@gmail.com
push dateWed, 08 Mar 2017 09:09:03 +0000
reviewersgijs
bugs1344431, 1260276, 1126967
milestone54.0a1
Bug 1344431 - Tell parent the non-reader-able reader page is not readable, r=gijs 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>