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 379139 4153103b32cc4e617f9c61d2e569b1566e974311
parent 379138 2c6189b62427a436363233adcd0a43743604fef5
child 379140 1171236daacc7fb79e5400c2ac00c881f93042f0
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs, gchang
bugs1344431, 1260276, 1126967
milestone53.0
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
@@ -253,17 +253,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);
@@ -271,17 +271,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;
     }
@@ -289,16 +289,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) {
@@ -310,22 +315,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
@@ -772,17 +772,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>