Last Comment Bug 558882 - (desktop-reader) Ship Readability in the desktop browser (reader mode)
(desktop-reader)
: Ship Readability in the desktop browser (reader mode)
Status: RESOLVED FIXED
: meta
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 39 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
http://lab.arc90.com/experiments/read...
: 791575 (view as bug list)
Depends on: fix-readability 1113795 1132659 1134363 1135587 1135593 1135915 1136419 1140233 1144117 1147467 1147626 1148052 1149837 1150759 1151150 1151732 1151734 1151735 1153775 1157123 1163526 1163715 792286 793917 793919 793920 795968 795973 795981 907079 927124 961994 1107097 1111142 1117258 1120518 1120735 1123471 1123910 1124011 1124271 1126967 1127337 1128724 1128757 1128916 1129029 1129106 1130094 1130206 1130588 1131303 1131458 1132307 1132547 1132656 1132665 1132674 1134441 1134443 1134658 1134932 1134965 1135009 1135351 1135553 1135589 1136231 1136245 1136265 1136704 1136716 1137211 1137258 1138628 1139026 1139174 1139678 1139967 1140045 1140172 1140340 1140345 1142298 1142499 1143844 1144822 1146358 1146373 1147122 1147337 1148050 1149261 1149277 1149649 1150229 1150276 1150476 1151415 1151685 1151737 1151793 1151795 1152121 1154028 1155536 1156211 1157682 1159684 1162750 1163519 1163523 1163534 1165670 1166240 1166648 1166651 1166704
Blocks: desktop-readinglist 1149138 962433 1126109 1184362
  Show dependency treegraph
 
Reported: 2010-04-12 13:36 PDT by Justin Dolske [:Dolske]
Modified: 2016-08-21 08:22 PDT (History)
94 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reader Mode works! (187.36 KB, patch)
2012-10-29 09:07 PDT, Chelsea Carr
no flags Details | Diff | Splinter Review
Reader mode, with toolbar (82.12 KB, patch)
2012-11-11 17:10 PST, Chelsea Carr
no flags Details | Diff | Splinter Review
A cleaned up version of all the reader mode for desktop source code (109.51 KB, patch)
2012-11-18 13:45 PST, Chelsea Carr
no flags Details | Diff | Splinter Review
Cleaned up a couple more things and moved common files to toolkit/components/reader/ (93.62 KB, patch)
2012-11-19 17:32 PST, Chelsea Carr
lucasr.at.mozilla: review-
jaws: feedback+
Details | Diff | Splinter Review
Complete version of Reader Mode (92.85 KB, text/plain)
2012-12-02 16:22 PST, Chelsea Carr
no flags Details
Complete version of Reader Mode (92.85 KB, patch)
2012-12-02 16:25 PST, Chelsea Carr
jaws: feedback+
Details | Diff | Splinter Review
Made some more changes (92.16 KB, patch)
2012-12-05 09:23 PST, Chelsea Carr
lucasr.at.mozilla: review-
jaws: review-
Details | Diff | Splinter Review
Added missing entries for reader mode icons and aboutreader.css in /browser/themes/winstripe/jar.mn under the aero section (107.90 KB, patch)
2013-09-13 01:44 PDT, Eklavya Mirani
jaws: feedback+
Details | Diff | Splinter Review
[WIP]Implementing Readability for Firefox (Desktop) (20.13 KB, patch)
2014-01-07 11:58 PST, Eklavya Mirani
blair: review-
Details | Diff | Splinter Review
Implemented Reader Mode for Desktop (26.39 KB, patch)
2014-01-09 13:20 PST, Eklavya Mirani
blair: review-
Details | Diff | Splinter Review
Screenshots of desktop reader mode (1.78 MB, application/x-zip-compressed)
2014-01-09 13:34 PST, Eklavya Mirani
no flags Details
ReaderMode_Fira.png (1.14 MB, image/png)
2014-06-02 06:56 PDT, Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com)
no flags Details
ReaderMode_menu.png (1.15 MB, image/png)
2014-06-02 06:57 PDT, Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com)
no flags Details

Description Justin Dolske [:Dolske] 2010-04-12 13:36:51 PDT
Scene: A website appears. It's typography and styling is terrible.

<limi> We should just ship Readability in the browser
<dolske> We do have View -> Page Style...

This would be neat. Limi also points out that on mobile, a clean text-only view of the page would be compelling too.

Not sure what the licensing is on Readability, but it probably won't be hard to cleanbox something similar to it, if needed.
Comment 1 Jason Orendorff [:jorendorff] 2010-07-29 11:25:01 PDT
My wife *loves* Readability. At first she thought it was a new Firefox feature, and I was sad to have to tell her it wasn't.
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2012-09-16 15:58:00 PDT
*** Bug 791575 has been marked as a duplicate of this bug. ***
Comment 3 Chelsea Carr 2012-10-29 09:07:57 PDT
Created attachment 676183 [details] [diff] [review]
Reader Mode works!

Spacing is off with aboutreader.js and a few other files.  Files will be moved around in the next patch as we have put them in directories that are not accessible by the mobile version.  Formatting still needs to be applied to the UI.  Hiding the button is only partially working right now.
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-11-02 09:55:51 PDT
Comment on attachment 676183 [details] [diff] [review]
Reader Mode works!

Review of attachment 676183 [details] [diff] [review]:
-----------------------------------------------------------------

I only looked at browser.js, since most of the other code is either trivial or copied from mobile. The copied files will need to live in Toolkit so they can be shared by mobile and desktop though.

::: browser/base/content/browser.js
@@ +47,5 @@
>  
> + [
> +   ["AboutReader", "chrome://browser/content/aboutReader.js"],
> + ].forEach(function (aScript) {
> +   let [name, script] = aScript;

This is a little confusing to me. I see what it is doing, but I hope there is a simpler way of doing the same thing. Nesting these arrays when there is only element of the nested array seems overly complicated.

@@ +242,5 @@
>      if (event.persisted)
>        gPluginHandler.reshowClickToPlayNotification();
>    }
> +  
> +  Reader.parseDocumentFromTab(gBrowser.contentWindow.location.id, function (article) {

There isn't an |id| property on window's location object. This part needs rethinking.

@@ +249,5 @@
> +        let tabURL = gBrowser.currentURI.specIgnoringRef;
> +        if (article == null || (article.url != tabURL)) {
> +          // Don't clear the article for about:reader pages since we want to
> +          // use the article from the previous page
> +          if (!/^about:reader/i.test(tabURL))

This can be made simpler by using:
if (!tabURL.startsWith("about:reader"))

@@ +261,5 @@
> + 				if (savedArticle != null) {
> + 				      document.getElementById("reader-button").hidden = false;//!gPrefService.getBoolPref("reader.enabled");
> + 				}
> + 				else {
> + 				  document.getElementById("reader-button").hidden = true;//!gPrefService.getBoolPref("reader.enabled");

You should add a getter for "reader-button" so that the getElementById code doesn't need to be duplicated everywhere.

@@ +5633,4 @@
>      gPageStyleMenu.disableStyle();
>  }
>  
> +var newTabBrowser = null;

This variable shouldn't be necessary.

@@ +7710,5 @@
>  }
> +
> +var savedArticle = null;
> +
> +let Reader = {

All of this looks to be copied. We shouldn't have this duplication. Can you get started with moving mobile's files to toolkit?
Comment 5 Jason Orendorff [:jorendorff] 2012-11-02 10:35:40 PDT
(In reply to Jared Wein [:jaws] from comment #4)
> > + [
> > +   ["AboutReader", "chrome://browser/content/aboutReader.js"],
> > + ].forEach(function (aScript) {
> > +   let [name, script] = aScript;
> 
> This is a little confusing to me. [...]

I agree. A nice way to write this is:

  {
    let name = "AboutReader", script = "chrome://browser/content/aboutReader.js";
    ...
  }

If it later needs to be turned into an array, that is easily done:

  for (let [name, script] of [
    ["AboutReader", "chrome://browser/content/aboutReader.js"]
  ]) {
    ...
  }
Comment 6 Chelsea Carr 2012-11-11 17:10:06 PST
Created attachment 680492 [details] [diff] [review]
Reader mode, with toolbar

We will be adding the changes that were suggested above in our next patch.  The images and css files still need to be added to winstripe and gnomestripe.  We will also be working on the functionality of the buttons on the toolbar in reader mode (share, reading list, etc.).  We may also try to implement an overlay, however if that doesn't work, we will switch the current functionality to load reader mode in the same tab as the original article.  We also need some help with getting the common files between mobile and desktop set up properly in toolkit. (creating a makefile.in is what we are stuck on right now.)
Comment 7 Jared Wein [:jaws] (please needinfo? me) 2012-11-12 14:20:28 PST
Comment on attachment 680492 [details] [diff] [review]
Reader mode, with toolbar

Review of attachment 680492 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/profile/firefox.js
@@ +24,5 @@
> +pref("reader.margin_size", 5);
> +pref("reader.font_size", 12);
> +pref("reader.color_scheme", "light");
> +pref("reader.has_used_toolbar", false);
> +pref("reader.enabled", false);

These prefs should be moved to the bottom of this file. There isn't a technical reason, but that is where most new preferences go :)

::: browser/base/content/aboutReader.html
@@ +1,1 @@
> +<!DOCTYPE html>

This needs a license header, similar to http://mxr.mozilla.org/mozilla-central/source/browser/base/content/abouthome/aboutHome.xhtml

::: browser/base/content/aboutReader.js
@@ +22,5 @@
> +
> +let gStrings = Services.strings.createBundle("chrome://browser/locale/aboutReader.properties");
> +
> +let AboutReader = function(doc, win) {
> +  // dump("Init()");

You'll need to remove this and the other commented out code.

@@ +42,5 @@
> +   this._contentElementRef = Cu.getWeakReference(doc.getElementById("reader-content"));
> +   this._toolbarElementRef = Cu.getWeakReference(doc.getElementById("reader-toolbar"));
> + 
> +   this._toolbarEnabled = true;
> + 

nitpick: leading and trailing whitespace here and other places.

::: browser/base/content/browser.xul
@@ +612,4 @@
>                     class="urlbar-icon"
>                     hidden="true"
>                     onclick="SocialShareButton.onClick(event);"/>
> +						<image id="reader-button"

nitpick: replace these tab characters with spaces.

::: browser/base/jar.mn
@@ +56,5 @@
>          content/browser/pageinfo/permissions.js       (content/pageinfo/permissions.js)
>          content/browser/pageinfo/security.js          (content/pageinfo/security.js)
> +        content/browser/Readability.js                (content/Readability.js)
> +        content/browser/JSDOMParser.js                (content/JSDOMParser.js)
> +        content/browser/readerWorker.js               (content/readerWorker.js)

These three files are mentioned in this jar.mn file, but I don't see them as part of this patch.

::: browser/themes/gnomestripe/browser.css
@@ +1427,5 @@
>  }
>  
> +/* Reader Button */
> +#reader-button {
> +  padding: 1px;

No need for the padding and the /* Reader Button */ comment can be removed.

::: mobile/android/themes/core/aboutReader.css
@@ +416,2 @@
>    bottom: -18px;
> +  background-image: url('chrome://browser/skin/places/reader-dropdown-arrow-hdpi.png');

I'm not sure why the styling of the mobile reader mode had to change, especially changes from mdpi to hdpi images. Can this be un-done?
Comment 8 Chelsea Carr 2012-11-18 13:45:26 PST
Created attachment 682930 [details] [diff] [review]
A cleaned up version of all the reader mode for desktop source code

We cleaned up the code (hopefully we got all the misc. whitespace and extra files out of there).  Still working on make .jsm files in toolkit/components/reader, so that both mobile and desktop can use those files.  We also attempted to change the "lazily loading arrays" code to the mentioned above changes, however we could not get it to work.  We will work on getting this fixed before we submit another patch.    We will also be removing the tags for the buttons on the toolbar within reader mode (share, add to reading list, view reading list).  We were getting some strange errors when we viewed the page with them removed.  We will do our best to have those removed before our next submitted patch.
Comment 9 Chelsea Carr 2012-11-19 17:32:23 PST
Created attachment 683389 [details] [diff] [review]
Cleaned up a couple more things and moved common files to toolkit/components/reader/

Please give us feedback on pieces of code that need to change.  We are still attempting to do the "lazily load"without arrays.  We also tried converting JSDOMParser.js and Readability.js to .jsm, however we were unsuccessful.  These two files are only used in readerWorker.js, which we successfully converted to a .jsm.  Please see notes on previous upload for more changes that were made recently.
Comment 10 Lucas Rocha (:lucasr) 2012-11-20 04:23:21 PST
Comment on attachment 683389 [details] [diff] [review]
Cleaned up a couple more things and moved common files to toolkit/components/reader/

Review of attachment 683389 [details] [diff] [review]:
-----------------------------------------------------------------

I'm only reviewing the copied files from mobile here. Needs a new version with all the suggested cleanups. But looks good overall!

::: browser/base/content/aboutReader.html
@@ +1,4 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +   

Remove trailing spaces here.

@@ +23,5 @@
> +  <div id="reader-content" class="content">
> +  </div>
> +
> +  <ul id="reader-toolbar" class="toolbar">
> +    <li><a id="share-button" class="button share-button" href="#"></a></li>

Remove this as it's not really used on desktop Firefox.

@@ +34,5 @@
> +        <hr></hr>
> +        <div id="margin-size-control" class="step-control"></div>
> +      </li>
> +    </ul>
> +    <li><a id="list-button" class="button list-button" href="#"></a></li>

Same here.

@@ +35,5 @@
> +        <div id="margin-size-control" class="step-control"></div>
> +      </li>
> +    </ul>
> +    <li><a id="list-button" class="button list-button" href="#"></a></li>
> +    <li><a id="toggle-button" class="button toggle-button" href="#"></a></li>

And here.

::: browser/base/content/aboutReader.js
@@ +42,5 @@
> +
> +  this._scrollOffset = win.pageYOffset;
> +
> +  let body = doc.body;
> +  body.addEventListener("touchstart", this, false);

You'll have to replace the touch-based logic with mouse-based events here.

@@ +46,5 @@
> +  body.addEventListener("touchstart", this, false);
> +  body.addEventListener("click", this, false);
> +
> +  win.addEventListener("scroll", this, false);
> +  win.addEventListener("popstate", this, false);

The popstate is used here to handle Android's system "Back" button press as a way to dismiss popups inside the reader UI. This is not necessary on desktop. Replace this logic by simply handling Escape key to dismiss popups.

@@ +50,5 @@
> +  win.addEventListener("popstate", this, false);
> +  win.addEventListener("resize", this, false);
> +
> +  this._setupAllDropdowns();
> +  this._setupButton("toggle-button", this._onReaderToggle.bind(this));

Remove all the code related to reading list toggling for now.

@@ +51,5 @@
> +  win.addEventListener("resize", this, false);
> +
> +  this._setupAllDropdowns();
> +  this._setupButton("toggle-button", this._onReaderToggle.bind(this));
> +  this._setupButton("list-button", this._onList.bind(this));

Remove all code related to list for now.

@@ +52,5 @@
> +
> +  this._setupAllDropdowns();
> +  this._setupButton("toggle-button", this._onReaderToggle.bind(this));
> +  this._setupButton("list-button", this._onList.bind(this));
> +  this._setupButton("share-button", this._onShare.bind(this));

Share button is not really implementable on desktop right now. Remove it.

@@ +78,5 @@
> +
> +  dump("Decoding query arguments");
> +  let queryArgs = this._decodeQueryString(win.location.href);
> +
> +  this._isReadingListItem = (queryArgs.readingList == "1");

Remove all code related to reading list for now.

@@ +169,5 @@
> +      classes.remove("on");
> +    }
> +  },
> +
> +  _onReaderToggle: function Reader_onToggle() {

You can remove this method.

@@ +187,5 @@
> +      }.bind(this));
> +    }
> +  },
> +
> +  _onList: function Reader_onList() {

You can remove this method.

@@ +192,5 @@
> +    if (!this._article)
> +      return;
> +  },
> +
> +  _onShare: function Reader_onShare() {

Same here.

@@ +312,5 @@
> +  },
> +
> +
> +
> +  _updateImageMargins: function Reader_updateImageMargins() {

This logic here will probably have to be re-done for the larger screens on desktop. But let's get the core feature working first before tweaking this.

@@ +553,5 @@
> +      }.bind(this), true);
> +    }
> +  },
> +
> +  _pushDropdownState: function Reader_pushDropdownState() {

You can probably remove this method.

::: mobile/android/themes/core/aboutReader.css
@@ +10,4 @@
>  }
>  
>  body {
> +  font-family: "OpenSansRegular",sans-serif;

Is Open Sans available in desktop at all? I don't think it is. We're in the process of licensing new fonts for Reader Mode so you just use "sans-serif" for now.

@@ +37,4 @@
>  }
>  
>  .header > h1 {
> +  font-family: "OpenSansLight",sans-serif;

Same here.

@@ +496,5 @@
>  
> +/*.toggle-button.on {
> +	background-image: url('chrome://browser/skin/places/reader-toggle-on-icon-port-hdpi.png');
> +	width: 45px;
> +	height: 36px;

Remove the commented code.

@@ +520,3 @@
>  
> +.style-button {
> +  background-image: url('chrome://browser/skin/places/reader-style-icon-port-hdpi.png');

You should probably rename the image names to be consistent with the rest of the assets on desktop Firefox. No need to name it for different densities AFAIK.

::: toolkit/components/reader/readerWorker.jsm
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. 
> + */
> +
> +
> +var EXPORTED_SYMBOLS = ["self.onmessage"];

Do you actually need to export self.onmessage here?
Comment 11 Jared Wein [:jaws] (please needinfo? me) 2012-11-26 15:17:57 PST
Comment on attachment 683389 [details] [diff] [review]
Cleaned up a couple more things and moved common files to toolkit/components/reader/

I'll wait for a new patch to be uploaded before looking it over. Nice work so far!
Comment 12 Chelsea Carr 2012-12-02 16:22:46 PST
Created attachment 687581 [details]
Complete version of Reader Mode

We have fixed our bugs and added in previously mentioned changes.  Please try applying the patch to make sure that we will be able to commit the code at the end of the project :)
Comment 13 Chelsea Carr 2012-12-02 16:25:57 PST
Created attachment 687582 [details] [diff] [review]
Complete version of Reader Mode

Previous attachment wasn't a patch.  oops.  Please try to apply this patch, so that we know our changes are able to be added to firefox.
Comment 14 Jared Wein [:jaws] (please needinfo? me) 2012-12-04 08:58:20 PST
(In reply to Chelsea Carr from comment #13)
> Created attachment 687582 [details] [diff] [review]
> Complete version of Reader Mode
> 
> Previous attachment wasn't a patch.  oops.  Please try to apply this patch,
> so that we know our changes are able to be added to firefox.

This patch applied cleanly and passed all browser-chrome tests for Linux, Windows, and OS X: https://tbpl.mozilla.org/?tree=Try&rev=cf612baa3fef
Comment 15 Jared Wein [:jaws] (please needinfo? me) 2012-12-04 09:40:11 PST
Comment on attachment 687582 [details] [diff] [review]
Complete version of Reader Mode

Review of attachment 687582 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't finish reviewing this patch yet, but here's some preliminary feedback that can be addressed before continuing. I pushed this patch to try server (and enabled reader mode by default). The results can be found here: https://tbpl.mozilla.org/?tree=Try&rev=21cd067568ee

::: browser/app/profile/firefox.js
@@ +26,1 @@
>  pref("browser.chromeURL","chrome://browser/content/");

nit: these blank lines above here should be removed

::: browser/base/content/browser.js
@@ +46,5 @@
>  
> + [
> +   ["AboutReader", "chrome://browser/content/aboutReader.js"],
> + ].forEach(function (aScript) {
> +   let [name, script] = aScript;

Comment #5 provided a possible way to clean this up. Please make the change that was requested there.

@@ +5677,5 @@
>  }
>  
> +var newTabBrowser = null;
> +var savedArticle = null;
> +var ar = null;

The |ar| variable is unused.

@@ +5680,5 @@
> +var savedArticle = null;
> +var ar = null;
> +
> +var ReaderMode = {
> +  _inited: false,

_inited is never used.

@@ +5695,5 @@
> +  BrowserOpenReaderMode: function(aEvent) {
> +    document.getElementById("reader-button").hidden = true;                            
> +    openUILink("about:reader?url=" + gBrowser.currentURI.specIgnoringRef, {target:gBrowser.selectedTab});
> +    newTabBrowser = gBrowser.getBrowserForTab(gBrowser.selectedTab);
> +    newTabBrowser.addEventListener("DOMContentLoaded", ReaderMode.handleDomContentLoaded, true);    

We shouldn't be storing a reference to a browser in a global variable. This will leak the tab when it is closed.

@@ +5701,5 @@
> +  
> +  PageShowHandler: function() {
> +    ReaderMode.checkReaderMode();
> +    if (savedArticle == null) {
> +      document.getElementById("reader-button").hidden = true;

These lines can be simplified to:
document.getElementById("reader-button").hidden = (savedArticle == null);

@@ +5711,5 @@
> +  
> +  TabSwitchHandler: function(event) {
> +    ReaderMode.checkReaderMode();
> +    if (savedArticle == null) {
> +      document.getElementById("reader-button").hidden = true;

document.getElementById("reader-button").hidden = (savedArticle == null);

@@ +5719,5 @@
> +    }
> +  },
> +  
> +  handleDomContentLoaded: function(event) {
> +    if (/^about:reader/.test(newTabBrowser.currentURI.specIgnoringRef)) {

You can get the current URI by doing:
var location = event.target.ownerDocument.defaultView.top.location;
var currentURI;
try {
  currentURI = makeURI(location);
} catch (e) { }
if (currentURI && /^about:reader/.test(currentURI.specIgnoringRef)) {
  ...
}

@@ +5721,5 @@
> +  
> +  handleDomContentLoaded: function(event) {
> +    if (/^about:reader/.test(newTabBrowser.currentURI.specIgnoringRef)) {
> +		  new AboutReader(newTabBrowser.contentDocument, newTabBrowser.contentWindow);
> +      newTabBrowser = gBrowser.getBrowserForTab(gBrowser.selectedTab);

This could just be gBrowser.selectedBrowser;

@@ +5731,5 @@
> +    savedArticle = null;
> +    if (gPrefService.getBoolPref("reader.enabled")) {
> +    
> +			Reader.parseDocumentFromTab(gBrowser.selectedTab, function (article) {
> +		     // Do nothing if there's no article or the page in this tab has

nit: mixed tabs and spaces here. This file's conventions are to use two-space indentation and no tabs.

@@ +5747,5 @@
> +				if (savedArticle != null) {
> +						 document.getElementById("reader-button").hidden = false;
> +				}
> +				else {
> +					document.getElementById("reader-button").hidden = true;

It would probably be a good idea to add a getter for the reader button so the "document.getElementById..." isn't duplicated everywhere.

You could add the following:
get readerButton () {
  return document.getElementById("reader-button");
}

You can learn more about getters and setters in JavaScript here, https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Operators/get

@@ +7791,5 @@
> +                                  success: success,
> +                                  title: title,
> +                                  url: url,
> +                                  }
> +                                  });*/

This dead code should just be deleted.
Comment 16 Chelsea Carr 2012-12-05 09:23:50 PST
Created attachment 688825 [details] [diff] [review]
Made some more changes

The suggestion for handledomcontentloaded doesn't quite work, so we implemented a different fix.  We also aren't sure how to push the patch to tinderbox to test it.  How do we do that?
Comment 17 Jared Wein [:jaws] (please needinfo? me) 2012-12-06 13:04:03 PST
I pushed the latest patch to the Try server to get some tests run on it, https://tbpl.mozilla.org/?tree=Try&rev=e3c71c398317

(In reply to Chelsea Carr from comment #16)
> We also aren't sure how to push the patch to
> tinderbox to test it.  How do we do that?

Let's talk on IRC about getting you level 1 check-in permissions so you can push your patches to Try server.
Comment 18 Lucas Rocha (:lucasr) 2012-12-10 04:42:11 PST
Comment on attachment 688825 [details] [diff] [review]
Made some more changes

Review of attachment 688825 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good overall but still needs some more cleanups. I'm reviewing the reader-specific bits btw. I expect Jared to review the desktop Firefox bits.

::: browser/base/content/aboutReader.html
@@ +1,4 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +  

nit: remove trailing spaces.

::: browser/base/content/aboutReader.js
@@ +19,5 @@
> +function dump(s) {
> +  Services.console.logStringMessage("AboutReader: " + s);
> +}
> +
> +let gStrings = Services.strings.createBundle("chrome://browser/locale/aboutReader.properties");

The aboutReader.properties file is not included in the patch. It should be.

@@ +26,5 @@
> +  this._docRef = Cu.getWeakReference(doc);
> +  this._winRef = Cu.getWeakReference(win);
> +
> +  this._article = null;
> + 

nit: remove trailing spaces.

@@ +44,5 @@
> +
> +  win.addEventListener("click", this, false);
> +
> +  win.addEventListener("scroll", this, false);
> +  win.addEventListener("popstate", this, false);

popstate is used in mobile to allow us to dismiss the style dialog when the user presses the Android Back button. This doesn't really apply to desktop Firefox. You should probably replace this with something as simple as Escape key press handling.

@@ +141,5 @@
> +          this._scrolled = true;
> +          this._setToolbarVisibility(false);
> +        }
> +        break;
> +      case "popstate":

As I said above, you should probably replace this with Escape key press handling.

@@ +216,5 @@
> +  },
> +
> +  _setToolbarVisibility: function Reader_setToolbarVisibility(visible) {
> +    let win = this._win;
> +    if (win.history.state)

This code only makes sense in conjunction with the popstate events. You can probably remove it.

@@ +264,5 @@
> +        this._showError(gStrings.GetStringFromName("aboutReader.loadError"));
> +    }.bind(this));
> +  },
> +
> +

nit: remove these extra empty lines here.

@@ +369,5 @@
> +
> +  _setupStepControl: function Reader_setupStepControl(id, name, callback) {
> +    let doc = this._doc;
> +    let stepControl = doc.getElementById(id);
> +    

nit: remove trailing spaces here.

@@ +373,5 @@
> +    
> +    while (stepControl.hasChildNodes()) {
> +      stepControl.removeChild(stepControl.lastChild);
> +    }
> +    

nit: remove trailing spaces here.

@@ +413,5 @@
> +          if (segmentedButton.lastChild.tagName == "LI") {
> +            segmentedButton.removeChild(segmentedButton.lastChild);
> +          }
> +        }
> +    }

nit: add empty line here.

@@ +512,5 @@
> +
> +        let dropdownClasses = dropdown.classList;
> +
> +        if (dropdownClasses.contains("open")) {
> +          win.history.back();

You can probably replace this with dropdownClasses.remove("open") on desktop.

@@ +516,5 @@
> +          win.history.back();
> +        } else {
> +          updatePopupPosition();
> +          if (!this._closeAllDropdowns())
> +            this._pushDropdownState();

You can probably remove this pushDropdownState() method on desktop.

@@ +524,5 @@
> +      }.bind(this), true);
> +    }
> +  },
> +
> +  _pushDropdownState: function Reader_pushDropdownState() {

You can remove this method together with the popstate handling.

::: mobile/android/themes/core/aboutReader.css
@@ -7,1 @@
>  }

This patch is applying on the mobile version of aboutReader.css. Is that intended?

@@ -563,5 @@
> -  }
> -
> -  .toolbar {
> -    background-size: 56px;
> -    background-image: url('chrome://browser/skin/images/reader-toolbar-bg-xlarge-mdpi.png');

The image names can probably be simplified on desktop as you don't have to handle this many screen densities.
Comment 19 Jared Wein [:jaws] (please needinfo? me) 2012-12-11 14:59:41 PST
Comment on attachment 688825 [details] [diff] [review]
Made some more changes

Review of attachment 688825 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +228,5 @@
> +    if (event.persisted) {
> +      gPluginHandler.reshowClickToPlayNotification();
> +    }
> +    
> +    ReaderMode.PageShowHandler();

The function body here got an extra indention that should be reverted.

@@ +1285,5 @@
> +      if (event.target == content.document)
> +        setTimeout(pageShowEventHandlers, 0, event);
> +    }, true);
> +    
> +    gBrowser.tabContainer.addEventListener("TabSelect", function(evt) { setTimeout(ReaderMode.TabSwitchHandler, 0, evt); }, true);

Change the variable name from evt to event.

@@ +5680,5 @@
> +  // ReaderMode Public Methods
> +  init: function ()
> +  {
> +    //Reader Button disable/enable
> +    document.getElementById("reader-button").hidden = true;//!gPrefService.getBoolPref("reader.enabled");

This function isn't necessary. The "reader-button" should have the hidden="true" attribute in browser.xul.

@@ +5685,5 @@
> +  },
> +
> +  //Reader Mode On-Click
> +  BrowserOpenReaderMode: function(aEvent) {
> +    document.getElementById("reader-button").hidden = true;                            

A good text editor should be able to remove trailing white-space with a single command and convert tabs to 2-space indents.

@@ +5691,5 @@
> +    ReaderMode.selectedReaderTab.addEventListener("DOMContentLoaded", ReaderMode.handleDomContentLoaded, true);    
> +  },
> +  
> +  PageShowHandler: function() {
> +    ReaderMode.checkReaderMode();

You should check if the pref is enabled, and if not then the other functions should return immediately.

@@ +5701,5 @@
> +    ReaderMode.readerButton.hidden = (savedArticle == null);
> +  },
> +  
> +  handleDomContentLoaded: function(event) {
> +		if(/^about:reader/.test(ReaderMode.selectedReaderTab.currentURI.specIgnoringRef)) {

This will break if the location is aBoUt:rEaDeR, so this should be tweaked a little. Should call .toLowerCase() on the specIgnoringRef. See https://bug716464.bugzilla.mozilla.org/attachment.cgi?id=591602 for a similar bug that happened with about:Addons.

::: browser/base/content/browser.xul
@@ +607,5 @@
>                     hidden="true"
>                     onclick="SocialShareButton.onClick(event);"/>
> +            <image id="reader-button"
> +                   class="urlbar-icon"
> +                   onclick="ReaderMode.BrowserOpenReaderMode(event);"/>

Add a hidden="true" here just like the SocialShareButton above.

::: browser/themes/gnomestripe/jar.mn
@@ +79,5 @@
> +  skin/classic/browser/places/readerDropdownArrow@2x.png     (places/readerDropdownArrow@2x.png)
> +  skin/classic/browser/places/readerMinusIcon@2x.png  (places/readerMinusIcon@2x.png)
> +  skin/classic/browser/places/readerPlusIcon@2x.png   (places/readerPlusIcon@2x.png)
> +  skin/classic/browser/places/readerStyleIcon@2x.png  (places/readerStyleIcon@2x.png)
> +  skin/classic/browser/places/readerToolbarBg@2x.png  (places/readerToolbarBg@2x.png)

It doesn't make sense to include HiDPI images for Linux.

::: mobile/android/themes/core/aboutReader.css
@@ +321,4 @@
>  }
>  
>  .toolbar {
> +   font-family: helvetica,arial,clean,sans-serif;

I'm not sure why the Android theme is changing here. It should remain how it was.

::: toolkit/components/reader/readerWorker.jsm
@@ +5,5 @@
> +
> +
> +
> +
> +importScripts("JSDOMParser.js", "Readability.js");

nit: remove these empty lines above. only have one blank line between the license header and the importScripts call.
Comment 20 :Ehsan Akhgari 2012-12-11 15:29:34 PST
Has private browsing been considered in the implementation of the reader mode?
Comment 21 Jared Wein [:jaws] (please needinfo? me) 2012-12-11 15:33:03 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> Has private browsing been considered in the implementation of the reader
> mode?

I don't think this patch has PB implications. When the reading list feature is added later in a followup, PB will need to be handled.
Comment 22 Caspy7 2012-12-11 15:57:28 PST
Forgive me if I'm replying out of embarrassing ignorance, but does Readability store the cleaned up page in a temporary location?
If so is this then cleared out when the page is closed?
Or is this just not accessible in PB mode?
Comment 23 :Ehsan Akhgari 2012-12-11 17:23:57 PST
Does readability store anything to the disk?  Does it cache anything in memory which can later be used in other tabs or windows?  If the answer to both of these questions is no, then we're probably fine.
Comment 24 Lucas Rocha (:lucasr) 2012-12-12 02:50:48 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #23)
> Does readability store anything to the disk?  Does it cache anything in
> memory which can later be used in other tabs or windows?  If the answer to
> both of these questions is no, then we're probably fine.

Reading list handling is not implemented yet so nothing is written to disk (yet). I guess we'll have to disable reading list features while in a private tab. Reader mode attaches the result of the readability parsing to the tab in memory so that I can be quickly fetched once you switch to reader mode. This data is disposed with the tab when the tab is closed (can't really be used by other tabs).
Comment 25 Caspy7 2012-12-12 07:50:17 PST
Cool. This sounds good.

It may be another discussion, but we allow users to access their tabs from other computers in PB mode, so I don't see the need to disallow accessing the Reading list.  Just reread, perhaps you weren't suggesting that.
Adding to it though would seem another question.  We allow users to save bookmarks while in PB mode, is adding to the reading list much different?
Comment 26 :Ehsan Akhgari 2012-12-12 08:08:50 PST
Please CC me on the bug which wants to implement the reading list on both desktop and mobile, and we can discuss the privacy implications there.  According to comment 24, there's no more concerns with regard to PB mode here.  Thanks for the clarification!
Comment 27 Jared Wein [:jaws] (please needinfo? me) 2012-12-12 08:16:28 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #26)
> Please CC me on the bug which wants to implement the reading list on both
> desktop and mobile, and we can discuss the privacy implications there.

To be clear (and this isn't the right bug but the discussion is here), reading list exists today in Firefox for Android. Bug 750707 appears to be the bug that added this feature, but Lucas will know more about it.
Comment 28 :Ehsan Akhgari 2012-12-12 08:22:44 PST
(In reply to comment #27)
> (In reply to Ehsan Akhgari [:ehsan] from comment #26)
> > Please CC me on the bug which wants to implement the reading list on both
> > desktop and mobile, and we can discuss the privacy implications there.
> 
> To be clear (and this isn't the right bug but the discussion is here), reading
> list exists today in Firefox for Android. Bug 750707 appears to be the bug that
> added this feature, but Lucas will know more about it.

I see.  Then I guess bug 820608 will cover that.  I'd appreciate if people in the know comment there.  Thanks!
Comment 29 Lucas Rocha (:lucasr) 2012-12-12 08:48:29 PST
(In reply to Mark S. from comment #25)
> Cool. This sounds good.
> 
> It may be another discussion, but we allow users to access their tabs from
> other computers in PB mode, so I don't see the need to disallow accessing
> the Reading list.  Just reread, perhaps you weren't suggesting that.
> Adding to it though would seem another question.  We allow users to save
> bookmarks while in PB mode, is adding to the reading list much different?

The reading list is essentially a separate bookmark root with bookmarks in it. So, that part is exactly like bookmarks. On mobile though, we also persist the content of the reading list item on disk via an IndexedDB owned by the about:reader page. This is necessary on mobile because you don't want to rely on internet connection to be able to read your saved stuff. That might not be as relevant on desktop though.
Comment 30 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-12-12 08:59:07 PST
For the record, IndexedDB is completely disabled in private tabs/windows.
Comment 31 Valerio 2013-02-16 08:52:21 PST Comment hidden (advocacy)
Comment 32 persona-stobor 2013-03-07 07:26:25 PST Comment hidden (advocacy)
Comment 33 Lucas Rocha (:lucasr) 2013-03-13 03:41:38 PDT Comment hidden (advocacy)
Comment 34 Caspy7 2013-03-17 11:50:57 PDT Comment hidden (advocacy)
Comment 35 Eklavya Mirani 2013-09-07 11:45:48 PDT
I am having troubles getting the reader mode to work. 

I cloned the parent rev, built it, imported the patch, rebuilt, changed setting in about:config but I dont see the reader mode toolbar.

What am I missing here?

PS: working on https://github.com/Yoric/Mozilla-Student-Projects/issues/57 so I thought this should be the place to start.
Comment 36 Lucas Rocha (:lucasr) 2013-09-09 04:24:49 PDT
(In reply to eklavyamirani from comment #35)
> I am having troubles getting the reader mode to work. 
> 
> I cloned the parent rev, built it, imported the patch, rebuilt, changed
> setting in about:config but I dont see the reader mode toolbar.
> 
> What am I missing here?

You should only see the reader icon in the toolbar if the current page is detected as "readable content" (i.e. article-like pages). Have you tried visiting an article in, say, nytimes.com?
Comment 37 Eklavya Mirani 2013-09-09 12:05:08 PDT
Yeah, I did try a few (How to geek, nytimes, Times of India, etc). Didn't work. I am using Windows Server 2012, btw.

I am trying to debug it, but I can't seem to get anywhere. 

So what should 'about:reader?url=someurl' return?
I get a page with 2 bulleted lists with nothing but '_____________________________'

I'll try to seek some help in the IRC.
Comment 38 Lucas Rocha (:lucasr) 2013-09-10 03:00:08 PDT
(In reply to eklavyamirani from comment #37)
> So what should 'about:reader?url=someurl' return?
> I get a page with 2 bulleted lists with nothing but
> '_____________________________'
> 
> I'll try to seek some help in the IRC.

This probably means the page is not working properly. Have a look at the GeckoConsole log messages in 'adb logcat' and try to spot any errors or warnings related to aboutReader.html, aboutReader.js, etc.

Accessing about:reader?url=someurl should download, parse, and display the target page in reader mode.
Comment 39 Eklavya Mirani 2013-09-10 09:44:41 PDT
Found the problem.

let request = window.indexedDB.open("about:reader", this.DB_VERSION);

throws an unknown exception with the message "The operation failed for reasons unrelated to the database itself and not covered by any other error code.", which I believe has something to do with this: https://bugzilla.mozilla.org/show_bug.cgi?id=643318.

request.onerror is not called, and request object itself becomes null.
Comment 40 Lucas Rocha (:lucasr) 2013-09-11 02:57:27 PDT
(In reply to eklavyamirani from comment #39)
> Found the problem.
> 
> let request = window.indexedDB.open("about:reader", this.DB_VERSION);
> 
> throws an unknown exception with the message "The operation failed for
> reasons unrelated to the database itself and not covered by any other error
> code.", which I believe has something to do with this:
> https://bugzilla.mozilla.org/show_bug.cgi?id=643318.
> 
> request.onerror is not called, and request object itself becomes null.

Ouch. Just so that you don't block on that, I'd probably consider initially disabling all the IndexedDB part of the code initially. The IndexedDB bits are only used for caching reading list items for offline usage. I suggest focusing only on the reader mode functionality (parsing and reformatting pages) first. Leave the reading list handling for follow-up patches.
Comment 41 Eklavya Mirani 2013-09-11 04:04:51 PDT
I also identified a few bugs, which lead to the readability toolbar not showing up, and fixed them.

Mostly, the files added to jar.mn in winstripe required those files to also be listed in the aero section which was not present.

As a result, it was looking for those files in source/browser/obj/chrome/.... . However, I fixed it, and the icon now shows.

Also, as a result, now about:reader?url=someurl shows an empty page with the list-style toolbar not displaying on the page as bulleted lists, but as the reader-toolbar (with font controls, etc) however clicking it does nothing and the page itself is completely blank.
Comment 42 Lucas Rocha (:lucasr) 2013-09-11 04:29:00 PDT
(In reply to eklavyamirani from comment #41)
> I also identified a few bugs, which lead to the readability toolbar not
> showing up, and fixed them.
> 
> Mostly, the files added to jar.mn in winstripe required those files to also
> be listed in the aero section which was not present.
> 
> As a result, it was looking for those files in
> source/browser/obj/chrome/.... . However, I fixed it, and the icon now shows.
> 
> Also, as a result, now about:reader?url=someurl shows an empty page with the
> list-style toolbar not displaying on the page as bulleted lists, but as the
> reader-toolbar (with font controls, etc) however clicking it does nothing
> and the page itself is completely blank.

I would probably go through aboutReader.js, adding some logging, and debug at which point it's failing to run.
Comment 43 Eklavya Mirani 2013-09-11 04:31:55 PDT
Update:
It now works completely on Windows with caching disabled. Tested url http://communities.washingtontimes.com/neighborhood/red-pill-blue-bill/2013/sep/11/two-two-morse-and-giron-both-recalled/

Lots of article based websites are not supported however.
Comment 44 Eklavya Mirani 2013-09-13 01:44:44 PDT
Created attachment 804314 [details] [diff] [review]
Added missing entries for reader mode icons and aboutreader.css in /browser/themes/winstripe/jar.mn under the aero section

Tested working with the parent revision on Windows Server 2012.
Comment 45 Lucas Rocha (:lucasr) 2013-09-13 02:56:50 PDT
Comment on attachment 804314 [details] [diff] [review]
Added missing entries for reader mode icons and aboutreader.css in /browser/themes/winstripe/jar.mn under the aero section

Review of attachment 804314 [details] [diff] [review]:
-----------------------------------------------------------------

jaws and/or mbrubeck are probably more appropriate reviewers here. I can give feedback on the readability-specific bits.
Comment 46 Jared Wein [:jaws] (please needinfo? me) 2013-09-13 12:38:47 PDT
Comment on attachment 804314 [details] [diff] [review]
Added missing entries for reader mode icons and aboutreader.css in /browser/themes/winstripe/jar.mn under the aero section

Review of attachment 804314 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for updating the patches here. Do you think you can rebase them off of mozilla-central tip? We no longer have a winstripe directory, it's been renamed to 'windows'. 'gnomestripe' -> 'linux', and 'pinstripe' -> 'osx', also.
Comment 47 Eklavya Mirani 2013-10-15 15:23:22 PDT
Sure thing Jared, happy to help.
Comment 48 uy2251+bugzilla 2013-10-15 15:36:31 PDT
Is the current work being done on this bug mean reader mode will be enabled on OS X or is it restricted to just windows?
Comment 49 Eklavya Mirani 2014-01-03 11:31:07 PST
While I am waiting for final review on Bug 927124, I'll start working on this.
Comment 50 Eklavya Mirani 2014-01-07 11:58:47 PST
Created attachment 8356724 [details] [diff] [review]
[WIP]Implementing Readability for Firefox (Desktop)

Follows Bug 927124. Since most of the code remains the same, I copied the files directly from MetroFX after that patch is applied.

Currently implemented only for Windows. Once its complete on Windows, I'll add the files over to linux and osx.
Comment 51 Erwann Mest 2014-01-07 12:45:37 PST
I know my comment will be totally useless but I'm really looking for this feature.

If I can help...?

Cheer-up!
Comment 52 Blair McBride [:Unfocused] (UNAVAILABLE) 2014-01-07 16:56:30 PST
Wow, you have awesome timing - one of the questions I was about to ask in my review in bug 927124 was what would need changed to help support this on Desktop Firefox :) This is the perfect way to help answer that.
Comment 53 Eklavya Mirani 2014-01-09 13:20:47 PST
Created attachment 8357978 [details] [diff] [review]
Implemented Reader Mode for Desktop

Again, follows patch in Bug 927124.

*Each tab now handles readability support properly.
*Added preference to disable reader mode

:P sorry for posting another one before the review, but I wanted to finish this one off before looking for a new project for this semester!
Comment 54 Eklavya Mirani 2014-01-09 13:34:17 PST
Created attachment 8357990 [details]
Screenshots of desktop reader mode

looks good, yuan? :)
Comment 55 Yuan Wang(:Yuan) – Mobile Firefox Design Lead 2014-01-09 13:48:25 PST
Nice work, Eklavya! I would like to get our visual design Michael to take a final look and see if we need any visual tweak before land this.

Michael, seems like the font option has been changed to Charis and Clear Sans. I think the reader icon and states might need tweaks. Please see the screenshots above for details.
Comment 56 Blair McBride [:Unfocused] (UNAVAILABLE) 2014-01-09 16:43:31 PST
(In reply to Eklavya from comment #53)
> :P sorry for posting another one before the review, but I wanted to finish
> this one off before looking for a new project for this semester!

Heh, no worries. Sorry I'm lagging on the reviews here a bit - I'm fighting the flu this week, and I'm been inundated with dozens of reviews. I'm think I'm most of the way through the previous patch in this bug and the Metro bug. I'll probably post the review for the old patch, and use that to work on the review for the new patch.
Comment 57 Matt Brubeck (:mbrubeck) 2014-01-13 10:57:45 PST
Comment on attachment 8357978 [details] [diff] [review]
Implemented Reader Mode for Desktop

Review of attachment 8357978 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this!  I haven't reviewed this thoroughly (and I'm not officially a reviewer for desktop Firefox), but here is some feedback:

::: browser/base/content/global-scripts.inc
@@ +6,4 @@
>  <script type="application/javascript" src="chrome://global/content/printUtils.js"/>
>  <script type="application/javascript" src="chrome://global/content/viewZoomOverlay.js"/>
>  <script type="application/javascript" src="chrome://browser/content/places/browserPlacesViews.js"/>
> +<script type="application/javascript" src="chrome://browser/content/aboutReader.js"/>

Is this needed in the main window, or only in aboutReader.html?  If it's not used in the chrome window, you should remove this line.

::: browser/themes/windows/browser.css
@@ +1246,5 @@
>  }
>  
> +#urlbar > #reader-button {
> +  list-style-image: url("chrome://browser/skin/urlbar-reader.png");
> +  -moz-image-region: rect(0, 14px, 14px, 0px);

We'll need similar changes for the Mac and Linux themes.
Comment 58 Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) 2014-01-14 09:19:45 PST
Additional UX work is needed before we ship:

• Reader mode glyphs should be themed and sized for each platform

• Desktop's form factor differs from android, therefore, we will need to adjust the typesetting for each of the three size options

• We need to make sure that the Reader mode UI can scale for future features like "Reading List".
Comment 59 Jared Wein [:jaws] (please needinfo? me) 2014-01-14 09:26:33 PST
Comment on attachment 8357978 [details] [diff] [review]
Implemented Reader Mode for Desktop

Review of attachment 8357978 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! I'm clearing review for now, please address the comments below and reupload.

::: browser/base/content/browser.js
@@ +158,5 @@
>    "about:home",
>    "about:privatebrowsing",
>    "about:welcomeback",
> +  "about:sessionrestore",
> +  "about:reader"

why does about:reader need to be added to gInitialPages?

@@ +804,5 @@
>      // setup history swipe animation
>      gHistorySwipeAnimation.init();
> +    Reader.init();
> +    ReaderMode.init();
> +    gBrowser.addEventListener("DOMContentLoaded", function(aEvent) {

Should this event listener be removed at some point in the future?

@@ +806,5 @@
> +    Reader.init();
> +    ReaderMode.init();
> +    gBrowser.addEventListener("DOMContentLoaded", function(aEvent) {
> +      if (ReaderMode.IsPrefEnabled) {
> +        if (aEvent.originalTarget.documentURI.toLowerCase().startsWith("about:reader") && aEvent.originalTarget.defaultView == gBrowser.contentWindow) {

nit, this line is quite long. Can you break it up to separate lines?

@@ +814,5 @@
> +            Services.console.logStringMessage("\nAboutReader : " + ex + "\n");
> +          }
> +        }
> +        let currentTab = gBrowser.selectedTab;
> +        currentTab.isReadabilitySupported = undefined;

It would be better if we don't add this type of data to the Tab element. What would happen if the tab is duplicated (Ctrl+drag) or dragged out of the browser to create a new window?

::: browser/base/content/browser.xul
@@ +647,5 @@
>                               tooltiptext="&goEndCap.tooltip;"/>
> +              <toolbarbutton id="reader-button"
> +                             class="chromeclass-toolbar-additional"
> +                             onclick="ReaderMode.BrowserOpenReaderMode(event)"
> +                             />

nit, you can self close the tag on the line of the last attribute instead of on its own line.

::: browser/base/content/readerMode.js
@@ +29,2 @@
>        this.readerButton.hidden = true;
> +      gBrowser.tabContainer.addEventListener("TabSelect", ReaderMode.TabSelectHandler, false);

nit, you can drop the last argument here.

@@ +47,4 @@
>    // Reader Mode On-Click
>    BrowserOpenReaderMode: function(aEvent) {
>      this.readerButton.hidden = true;
> +    this._openReaderPage("about:reader?url=" + gBrowser.contentWindow.location.href);

_openReaderPage should add the "about:reader?url=" prefix so that all of the callers of the function don't have to supply it.

@@ +56,5 @@
>    },
>  
>    PageShowHandler: function(aEvent) {
> +    let tab = aEvent.target;
> +    if (tab.isReadabilitySupported == undefined) {

Using undefined here causes "isReadabilitySupported" to be a three-state variable, which isn't obvious at first. Can you find a better way to write this code? (Maybe introduce another variable?)

@@ +65,5 @@
>    },
>  
> +  TabSelectHandler: function(aEvent) {
> +    ReaderMode.Log("Tab switched. Calling PageShowHandler");
> +    ReaderMode.readerButton.hidden = true;

We should only show/hide the button once per TabSelect+PageShow so it doesn't blink when switching between about:reader pages.
Comment 60 Blair McBride [:Unfocused] (UNAVAILABLE) 2014-01-14 17:03:48 PST
Comment on attachment 8356724 [details] [diff] [review]
[WIP]Implementing Readability for Firefox (Desktop)

Review of attachment 8356724 [details] [diff] [review]:
-----------------------------------------------------------------

My comments here go hand-in-hand with my comments in 927124, so should be taken together. Haven't looked at the differences between this and the new patch yet, will get to it when I can.

A lot of these files seem to need basically no changes, which is what I was hoping for. So I think we should move the following into /toolkit/ so they can be used by both Desktop and Metro with minimal code duplication:
* aboutReader.html
* aboutReader.js
* aboutReader.css (see my comments in bug 927124).

Generally, most things should be in toolkit. An application should only need a few small-ish things to add support for Reader mode:
* JS code for integration (called readerMode.js in these patches)
* CSS styles for toolbars/dropdowns
* CSS styles for form-factor specific tweaks to the page (making sure the typography is appropriate for a phone vs a desktop screen)
* Any UI in the main window to switch to reader mode
* An about: redirector

::: browser/base/content/aboutReader.html
@@ +5,4 @@
>    <meta content="text/html; charset=UTF-8" http-equiv="content-type">
>    <meta name="viewport" content="width=device-width; user-scalable=0" />
>  
> +  <link rel="stylesheet" href="chrome://browser/skin/aboutReader.css" type="text/css"/>

The downside of moving this to /toolkit/ is that you can't use overlays with HTML documents to make changes specific to Desktop or Metro :\

But there is an alternative:

You can add one <link> to load a stylesheet in /toolkit/ that contains all the page styles - this is the generic styles used by both desktop and metro. Then another <link> to load a separate stylesheet in /toolkit/ that contains all the styles for the toolbars/dropdown on desktop (the default target for toolkit is desktop). Then in Metro, add an "override" directive in a jar.mn file to override the toolbars/dropdown stylesheet with Metro's own stylesheet (same syntax as what you did in bug 927124 with aboutReader.css).

::: browser/base/content/browser.js
@@ +10,4 @@
>  Cu.import("resource://gre/modules/NotificationDB.jsm");
>  Cu.import("resource:///modules/RecentWindow.jsm");
>  Cu.import("resource://gre/modules/WindowsPrefSync.jsm");
> +Cu.import("resource://gre/modules/Reader.jsm");

This should be lazy-loaded.
Comment 61 Blair McBride [:Unfocused] (UNAVAILABLE) 2014-01-14 20:57:15 PST
Comment on attachment 8357978 [details] [diff] [review]
Implemented Reader Mode for Desktop

Review of attachment 8357978 [details] [diff] [review]:
-----------------------------------------------------------------

I think between my comments for the previous version of this patch, Jared's comments on this version of the patch, and the comments in bug 927124, this patch will already has enough changes scheduled that any further comments on this version won't be much use.

One important thing that did just come out of bug 927124 though: The about redirector needs to change to be *unprivileged* (ie, add URI_SAFE_FOR_UNTRUSTED_CONTENT). Should probably hide it from about:about too.
Comment 62 Caspy7 2014-01-22 22:19:51 PST
Perhaps it has been discussed elsewhere (if so please point me) or I missed it here when scanning, but is this designed such that it can be easily enough extended by addons?
I'm sure there will be users who would like to add more options or increase granularity of control for display.  
For me personally, I would love to use this as a one-click way to clean up articles being sent to Evernote.  It's very useful functionality that can be leveraged in multiple ways I think.
Comment 63 Eklavya Mirani 2014-01-28 08:14:04 PST
(In reply to Mark S. from comment #62)
> Perhaps it has been discussed elsewhere (if so please point me) or I missed
> it here when scanning, but is this designed such that it can be easily
> enough extended by addons?

Unless the addon works with the page uri *only* (for e.g. the Pocket addon), I think readability parsed pages should work with the addons out of the box, since all we are doing is displaying an html document.

(In reply to Jared Wein [:jaws] from comment #59)
> > +        let currentTab = gBrowser.selectedTab;
> > +        currentTab.isReadabilitySupported = undefined;
> 
> It would be better if we don't add this type of data to the Tab element.
> What would happen if the tab is duplicated (Ctrl+drag) or dragged out of the
> browser to create a new window?

I understand that duplicating a tab *should* duplicate this new property too, but as soon as "DOMContentLoaded" is fired, it is set to undefined. Not the cleanest way to do it, but we need a tab/browser specific property that we can use to check if the page is readability supported, instead of making an XHR everytime a tab is switched (even though the page doesn't change.)

Where do you suggest I attach the property?
 
> > +  TabSelectHandler: function(aEvent) {
> > +    ReaderMode.Log("Tab switched. Calling PageShowHandler");
> > +    ReaderMode.readerButton.hidden = true;
> 
> We should only show/hide the button once per TabSelect+PageShow so it
> doesn't blink when switching between about:reader pages.

In that case, the reader icon remains there for a second or so, before being hidden on an unsupported page. And clicking it opens about:reader page for that unsupported page.

Resetting the visibility will cause the reader icon to show up only if the page is readability supported, after the XHR request completes (or the property is read)

Does this sound good? or do you suggest changing it?
Comment 64 Jared Wein [:jaws] (please needinfo? me) 2014-01-29 12:56:22 PST
(In reply to Eklavya from comment #63)
> (In reply to Jared Wein [:jaws] from comment #59)
> > > +        let currentTab = gBrowser.selectedTab;
> > > +        currentTab.isReadabilitySupported = undefined;
> > 
> > It would be better if we don't add this type of data to the Tab element.
> > What would happen if the tab is duplicated (Ctrl+drag) or dragged out of the
> > browser to create a new window?
> 
> I understand that duplicating a tab *should* duplicate this new property
> too, but as soon as "DOMContentLoaded" is fired, it is set to undefined. Not
> the cleanest way to do it, but we need a tab/browser specific property that
> we can use to check if the page is readability supported, instead of making
> an XHR everytime a tab is switched (even though the page doesn't change.)
> 
> Where do you suggest I attach the property?

You can put it on the browser (gBrowser.selectedBrowser). For example, see things like userTypedClear and mIconURL in /toolkit/content/widgets/browser.xml. But you will need to get OK from a toolkit peer to add this to the toolkit binding.

> > > +  TabSelectHandler: function(aEvent) {
> > > +    ReaderMode.Log("Tab switched. Calling PageShowHandler");
> > > +    ReaderMode.readerButton.hidden = true;
> > 
> > We should only show/hide the button once per TabSelect+PageShow so it
> > doesn't blink when switching between about:reader pages.
> 
> In that case, the reader icon remains there for a second or so, before being
> hidden on an unsupported page. And clicking it opens about:reader page for
> that unsupported page.
> 
> Resetting the visibility will cause the reader icon to show up only if the
> page is readability supported, after the XHR request completes (or the
> property is read)
> 
> Does this sound good? or do you suggest changing it?

Ok, you can leave this unchanged.
Comment 65 dorian.goepp 2014-02-07 04:47:23 PST
Hi !

There is also an other readability service around there, called wallbag (wallbag.org, formerly poche). I'm not part of the team but regular user and beleive that they would be happy (https://twitter.com/wallabagapp/status/431770343943180288) if it were possible to work together to allow a kind of synchronisation with one's wallbag instance (everyone can install it on he's server).

By the way, I personally would love it it were possible to have the text displayed with justification (maybe by an option to chose). I feel it much easier to read.

Still in dreams, is it planned to add e-pub export one day ?

It's very likely that this comment doesn't have its place here. If it's the case, please let me know where I should move it.

Thank a lot for your work. I love it ! (from what I've seen in the screenshots)
Comment 66 Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) 2014-02-27 07:36:00 PST
Please find the Reader Mode spec below:

http://people.mozilla.org/~mmaslaney/readermode/index.html
Comment 67 dorian.goepp 2014-05-21 05:06:35 PDT
(In reply to dorian.goepp from comment #65)
> Hi !
> 
> There is also an other readability service around there, called wallbag
> (wallbag.org, formerly poche). I'm not part of the team but regular user and
> beleive that they would be happy
> (https://twitter.com/wallabagapp/status/431770343943180288) if it were
> possible to work together to allow a kind of synchronisation with one's
> wallbag instance (everyone can install it on he's server).
> 
> By the way, I personally would love it it were possible to have the text
> displayed with justification (maybe by an option to chose). I feel it much
> easier to read.
> 
> Still in dreams, is it planned to add e-pub export one day ?
> 
> It's very likely that this comment doesn't have its place here. If it's the
> case, please let me know where I should move it.
> 
> Thank a lot for your work. I love it ! (from what I've seen in the
> screenshots)

My bad, the application name is Wallabag and their website is https://www.wallabag.org/.

Sounds good, no ?
Comment 68 Mike de Boer [:mikedeboer] 2014-05-28 02:15:07 PDT
Eklavya, now that Michael posted an awesome spec to work with, would you be comfortable working on this some more?
What are your plans?
Comment 69 Eklavya Mirani 2014-05-28 02:50:00 PDT
Certainly. I am wrapping up the work for reader mode in metro firefox, since it is almost done.
You can find all my changes at https://github.com/eklavyamirani/gecko-dev
Comment 70 Mike de Boer [:mikedeboer] 2014-05-28 02:51:40 PDT
(In reply to Eklavya Mirani from comment #69)
> Certainly. I am wrapping up the work for reader mode in metro firefox, since
> it is almost done.
> You can find all my changes at https://github.com/eklavyamirani/gecko-dev

Did you know that Metro Firefox was discontinued?
Comment 71 Eklavya Mirani 2014-05-28 02:58:18 PDT
Yes. But since it's almost done, I really wanted to see it through to the end (atleast v)

Since my semester is about to end, I should be able to actively get back to desktop reader mode by next week.

Is there a specific deadline/milestone that I should keep in mind to finish v1 on desktop? :)
Comment 72 Mike de Boer [:mikedeboer] 2014-05-28 03:00:37 PDT
(In reply to Eklavya Mirani from comment #71)
> Yes. But since it's almost done, I really wanted to see it through to the
> end (atleast v)

Alright, good to hear!

> Since my semester is about to end, I should be able to actively get back to
> desktop reader mode by next week.

Even better to hear!!

> Is there a specific deadline/milestone that I should keep in mind to finish
> v1 on desktop? :)

Nope. But as always, the sooner the better. ;)
Comment 73 Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) 2014-06-02 06:56:29 PDT
Created attachment 8432516 [details]
ReaderMode_Fira.png

An amendment to the spec.

We will be using Fira Sans instead of Clear sans for the Sans Serif typeface, specifically the Bold weight for the headers and the book weights for the body. The CSS values will remain the same as they appear in the spec.

If needed I can update the official spec.
Comment 74 Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) 2014-06-02 06:57:39 PDT
Created attachment 8432519 [details]
ReaderMode_menu.png
Comment 75 Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) 2014-06-02 07:29:26 PDT
http://dev.carrois.com/fira-3-1/
Comment 76 Caspy7 2014-06-06 02:44:37 PDT
(In reply to mmaslaney from comment #74)
> Created attachment 8432519 [details]
> ReaderMode_menu.png

I really think we should expose UI allowing users to change the font size.

I'd also float the idea that there is no need to include the font name as there is little to no benefit gained from it.  Reading it may slow down the interaction.
Comment 77 Eklavya Mirani 2014-08-25 03:28:30 PDT
Hi, sorry for not posting updates on this. I am trying to squeeze in time for this, but between my first job and relocating from my house to another part of the country (and waiting for a new laptop to be delivered), I haven't been able to complete this.

Still committed to completing this. :)
Comment 78 Dannyzen 2014-11-17 08:24:54 PST Comment hidden (me-too)
Comment 79 Erwann Mest 2014-11-17 08:31:33 PST Comment hidden (me-too)
Comment 80 Eklavya Mirani 2014-12-10 02:54:57 PST
Hi,

I am rebasing the patch today. Will post it for review as I am done. (and in case anybody wants to pick it up...)

For some odd reason DOMContentLoaded is not firing the first time the page is being loaded but it is fired when I switch tabs.
Comment 81 :Margaret Leibovic 2014-12-10 12:11:44 PST
(In reply to Eklavya Mirani from comment #80)

> I am rebasing the patch today. Will post it for review as I am done. (and in
> case anybody wants to pick it up...)

That's awesome! I would definitely be interested in helping you land your work. I've been working on reader mode/reading list in Firefox for Android, and I want to move our code into a shared place that desktop can use as well.

It looks like the patch in this bug depends on some other changes that aren't landed in the tree. Could you give a summary of your current work and the bugs at play here?

I would love to get involved and help you out, but I don't want to step on any toes! :)
Comment 82 Eklavya Mirani 2014-12-11 07:12:35 PST
Thanks Margaret, I will sit tonight and document my work and try to fix as much as I can and share it with you. 

Here's a summary of the bugs that this bug depends upon/used:
Bug 795973 : Adds a pref to enable/disable reader mode from about:config. Implemented and tested. (Not assigned to me though, anyway, it was a part of MetroFX work I did. Bug 941850)

Bug 795981 : Add a reader mode button to the location bar. Implemented and tested on Windows and OS X. Not sure about Linux, but it should work. (Added functionality in the next bug as well.)

Bug 920712 - Add Reader code to metro firefox so the Reader Icon is only displayed if the page is supported for parsing by Readability. So if you change tabs and the page is not readability supported, reader mode icon is hidden, and when you switch back, it shows up. (Similarly works in reader mode desktop as well)

Bug 963657 - UX work - Readability UI improvements
So far in this, I have implemented the UI, and a buggy drag and drop of the reader toolbar. However in my Reader Mode V1, I didn't implement any backend for reader lists due to a bug in indexedDB. This is what I have to complete for reader mode desktop.

Bug 961994 - Move aboutReader.html to /toolkit - This last patch was never reviewed. But I moved about reader.js, about reader.html, readability.js to /toolkit, since both MetroFX and Firefox desktop use the same code without any changes. The plan was for all three of the platforms to use the same codebase.

Bug 962433 - Use Reader Mode for printing articles
Nothing fancy here, but once the reader mode was implemented, we could also use for printing the readability parsed article.

If you want I can share my code with you, the only problem being DOMContentLoaded not firing as the earlier version of firefox that I was using, which was driving the code for checking if the page is readability supported and creating a new AboutReader object in case the uri was a reader page (about:reader?url=...)

Thanks,
Eklavya
Comment 83 Eklavya Mirani 2014-12-11 11:15:38 PST
Okay, here is what I have found:

1. We are using E10s now. My patch doesn't work on e10s since DOMContentLoaded/other page load events are not firing until I switch the active tab. (I loaded non-e10 window, and everything but the reader mode api works. See #2)

2. Reader Mode Api from Android has changed and requires rebasing the code on the new version of reader.js
Comment 84 Jared Wein [:jaws] (please needinfo? me) 2014-12-11 11:20:25 PST
Yeah, a lot has changed between then and now. The process of rebasing the patches may not be worth the effort. If you would like to, you could probably work side-by-side with Margaret on reimplementing many of the ideas/efforts that were attempted in the previous approach.
Comment 85 :Margaret Leibovic 2014-12-12 11:06:01 PST
Eklavya, thanks so much for the summary.

I'm currently working in bug 793920 to move our current mobile reader mode code into toolkit, and after that I plan to work on factoring out some of the mobile-specific parts, as well as making the shared code e10s-friendly (I will file new bugs for these parts). After these changes, we should be ready to start implementing the desktop UI components on top of the shared code. I think this bug here should actually turn into a meta bug, and we can use the sub-bugs to track progress on different parts of this effort.

Given the fact that a lot has changed since you last worked on this, I think it would make sense to wait for me to finish these larger pieces of work, and then you can jump in to help out with some of the desktop UI pieces you've already worked on. Once the big pieces land, there should be lots of things we can do in parallel, and I would definitely appreciate your help!
Comment 86 Caspy7 2015-02-21 20:01:45 PST
Since the e10s-friendly redesign, with this feature still be accessible to addons?

I can imagine imagine an uber featured addon which allows more custom display or one that uses the output to clean up pages before sending them to Evernote.
Comment 87 :Margaret Leibovic 2015-02-22 11:40:39 PST
(In reply to Caspy7 from comment #86)
> Since the e10s-friendly redesign, with this feature still be accessible to
> addons?
> 
> I can imagine imagine an uber featured addon which allows more custom
> display or one that uses the output to clean up pages before sending them to
> Evernote.

To be honest, I haven't spent much time thinking about how add-ons will want to interact with reader view, but I imagine they could follow the same strategies add-ons use to interact with content in general (such as whatever any page mode add-on would need to do). I think it could be interesting to see add-ons add more capabilities to reader view.

And as for an add-on that just wants to use the basic Readability functionality without modifying the reader view, ReaderMode.jsm provides APIs that would let them directly process content.
Comment 88 Daniel Holbert [:dholbert] 2016-01-20 21:22:15 PST
What is the current purpose of this bug? (and does it make sense that it's still open?)

The bug-title is "Ship Readability in the desktop browser (reader mode)", and we did that last June in Firefox 38.0.5.

Seems like this bug should either be closed as FIXED (with concerns/issues still tracked via 'depends-on' entries), or its summary should be updated to reflect the reality of whatever work is remaining to be done here (if any).
Comment 89 :Margaret Leibovic 2016-01-21 11:30:00 PST
(In reply to Daniel Holbert [:dholbert] from comment #88)
> What is the current purpose of this bug? (and does it make sense that it's
> still open?)
> 
> The bug-title is "Ship Readability in the desktop browser (reader mode)",
> and we did that last June in Firefox 38.0.5.
> 
> Seems like this bug should either be closed as FIXED (with concerns/issues
> still tracked via 'depends-on' entries), or its summary should be updated to
> reflect the reality of whatever work is remaining to be done here (if any).

At this point this bug is just a dumping ground of issues we found while implementing desktop reader mode.

We could just close this. Someone could triage the list of open bugs, but unless this becomes more of a priority I don't think anyone is going to have the time to work on them.

Note You need to log in before you can comment on or make changes to this bug.