Closed
Bug 558882
(desktop-reader)
Opened 14 years ago
Closed 8 years ago
[meta] Ship Readability in the desktop browser (reader mode)
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
People
(Reporter: Dolske, Unassigned)
References
(Depends on 10 open bugs, Blocks 1 open bug, )
Details
(Keywords: meta)
Attachments
(2 files, 11 obsolete files)
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.
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → dolske
Comment 1•14 years ago
|
||
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.
Reporter | ||
Updated•13 years ago
|
Assignee: dolske → nobody
Comment 3•12 years ago
|
||
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.
Attachment #676183 -
Flags: review?(lucasr.at.mozilla)
Attachment #676183 -
Flags: review?(jaws)
Comment 4•12 years ago
|
||
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?
Attachment #676183 -
Flags: review?(jaws)
Comment 5•12 years ago
|
||
(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•12 years ago
|
||
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.)
Attachment #676183 -
Attachment is obsolete: true
Attachment #676183 -
Flags: review?(lucasr.at.mozilla)
Attachment #680492 -
Flags: review?(lucasr.at.mozilla)
Attachment #680492 -
Flags: review?(jaws)
Comment 7•12 years ago
|
||
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?
Attachment #680492 -
Flags: review?(jaws)
Comment 8•12 years ago
|
||
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.
Attachment #680492 -
Attachment is obsolete: true
Attachment #680492 -
Flags: review?(lucasr.at.mozilla)
Attachment #682930 -
Flags: review?(lucasr.at.mozilla)
Attachment #682930 -
Flags: review?(jaws)
Comment 9•12 years ago
|
||
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.
Attachment #682930 -
Attachment is obsolete: true
Attachment #682930 -
Flags: review?(lucasr.at.mozilla)
Attachment #682930 -
Flags: review?(jaws)
Attachment #683389 -
Flags: review?(lucasr.at.mozilla)
Attachment #683389 -
Flags: review?(jaws)
Comment 10•12 years ago
|
||
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?
Attachment #683389 -
Flags: review?(lucasr.at.mozilla) → review-
Comment 11•12 years ago
|
||
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!
Attachment #683389 -
Flags: review?(jaws) → feedback+
Comment 12•11 years ago
|
||
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 :)
Attachment #683389 -
Attachment is obsolete: true
Attachment #687581 -
Flags: review?(lucasr.at.mozilla)
Attachment #687581 -
Flags: review?(jaws)
Comment 13•11 years ago
|
||
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.
Attachment #687581 -
Attachment is obsolete: true
Attachment #687581 -
Flags: review?(lucasr.at.mozilla)
Attachment #687581 -
Flags: review?(jaws)
Updated•11 years ago
|
Attachment #687582 -
Flags: review?(lucasr.at.mozilla)
Attachment #687582 -
Flags: review?(jaws)
Comment 14•11 years ago
|
||
(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•11 years ago
|
||
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.
Attachment #687582 -
Flags: review?(jaws) → feedback+
Comment 16•11 years ago
|
||
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?
Attachment #687582 -
Attachment is obsolete: true
Attachment #687582 -
Flags: review?(lucasr.at.mozilla)
Attachment #688825 -
Flags: review?(lucasr.at.mozilla)
Attachment #688825 -
Flags: review?(jaws)
Comment 17•11 years ago
|
||
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•11 years ago
|
||
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.
Attachment #688825 -
Flags: review?(lucasr.at.mozilla) → review-
Comment 19•11 years ago
|
||
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.
Attachment #688825 -
Flags: review?(jaws) → review-
Comment 20•11 years ago
|
||
Has private browsing been considered in the implementation of the reader mode?
Comment 21•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
For the record, IndexedDB is completely disabled in private tabs/windows.
Comment hidden (advocacy) |
Comment hidden (advocacy) |
Comment hidden (advocacy) |
Comment hidden (advocacy) |
Comment 35•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
Tested working with the parent revision on Windows Server 2012.
Attachment #804314 -
Flags: review?(lucasr.at.mozilla)
Attachment #804314 -
Flags: review?(jaws)
Comment 45•11 years ago
|
||
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.
Attachment #804314 -
Flags: review?(lucasr.at.mozilla)
Comment 46•11 years ago
|
||
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.
Attachment #804314 -
Flags: review?(jaws) → feedback+
Comment 47•11 years ago
|
||
Sure thing Jared, happy to help.
Comment 48•11 years ago
|
||
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?
Updated•11 years ago
|
Summary: Ship Readability in the browser → Ship Readability in the desktop browser (reader mode)
Comment 49•10 years ago
|
||
While I am waiting for final review on Bug 927124, I'll start working on this.
Updated•10 years ago
|
Assignee: nobody → eklavyamirani
Status: NEW → ASSIGNED
Updated•10 years ago
|
Blocks: metrobacklog
Whiteboard: [feature] p=0
Updated•10 years ago
|
No longer blocks: metrobacklog, 927124
Whiteboard: [feature] p=0
Comment 50•10 years ago
|
||
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.
Attachment #804314 -
Attachment is obsolete: true
Attachment #8356724 -
Flags: review?(jaws)
Updated•10 years ago
|
Comment 51•10 years ago
|
||
I know my comment will be totally useless but I'm really looking for this feature. If I can help...? Cheer-up!
Updated•10 years ago
|
Attachment #8356724 -
Flags: review?(bmcbride)
Updated•10 years ago
|
Attachment #688825 -
Attachment is obsolete: true
Comment 52•10 years ago
|
||
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•10 years ago
|
||
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!
Attachment #8356724 -
Attachment is obsolete: true
Attachment #8356724 -
Flags: review?(jaws)
Attachment #8356724 -
Flags: review?(bmcbride)
Attachment #8357978 -
Flags: review?(mbrubeck)
Attachment #8357978 -
Flags: review?(jaws)
Attachment #8357978 -
Flags: review?(bmcbride)
Comment 55•10 years ago
|
||
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.
Flags: needinfo?(mmaslaney)
Comment 56•10 years ago
|
||
(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•10 years ago
|
||
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.
Attachment #8357978 -
Flags: review?(mbrubeck)
Comment 58•10 years ago
|
||
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".
Flags: needinfo?(mmaslaney)
Comment 59•10 years ago
|
||
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.
Attachment #8357978 -
Flags: review?(jaws)
Comment 60•10 years ago
|
||
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.
Attachment #8356724 -
Flags: review-
Comment 61•10 years ago
|
||
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.
Attachment #8357978 -
Flags: review?(bmcbride) → review-
Comment 62•10 years ago
|
||
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•10 years ago
|
||
(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?
Flags: needinfo?(jaws)
Comment 64•10 years ago
|
||
(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.
Flags: needinfo?(jaws)
Comment 65•10 years ago
|
||
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•10 years ago
|
||
Please find the Reader Mode spec below: http://people.mozilla.org/~mmaslaney/readermode/index.html
Updated•10 years ago
|
Flags: firefox-backlog?
Comment 67•10 years ago
|
||
(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 ?
Updated•10 years ago
|
Flags: firefox-backlog? → firefox-backlog+
Comment 68•10 years ago
|
||
Eklavya, now that Michael posted an awesome spec to work with, would you be comfortable working on this some more? What are your plans?
Flags: needinfo?(eklavyamirani+bugmail)
Comment 69•10 years ago
|
||
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
Flags: needinfo?(eklavyamirani+bugmail)
Comment 70•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
Comment 75•10 years ago
|
||
http://dev.carrois.com/fira-3-1/
Comment 76•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8357990 -
Flags: review?(ywang)
Comment 77•10 years ago
|
||
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 hidden (me-too) |
Comment hidden (me-too) |
Updated•10 years ago
|
Flags: needinfo?(eklavyamirani+bugmail)
Comment 80•9 years ago
|
||
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.
Flags: needinfo?(eklavyamirani+bugmail)
Comment 81•9 years ago
|
||
(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! :)
Flags: needinfo?(eklavyamirani+bugmail)
Comment 82•9 years ago
|
||
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
Flags: needinfo?(eklavyamirani+bugmail)
Comment 83•9 years ago
|
||
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
Flags: needinfo?(jaws)
Comment 84•9 years ago
|
||
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.
Flags: needinfo?(jaws)
Comment 85•9 years ago
|
||
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!
Updated•9 years ago
|
Attachment #8357978 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8357990 -
Attachment is obsolete: true
Updated•9 years ago
|
Alias: desktop-reader
Updated•9 years ago
|
Blocks: desktop-readinglist
Updated•9 years ago
|
See Also: → desktop-readinglist
Updated•9 years ago
|
Flags: firefox-backlog+
Updated•9 years ago
|
Updated•9 years ago
|
Assignee: eklavyamirani+bugmail → nobody
Comment 86•9 years ago
|
||
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•9 years ago
|
||
(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.
Reporter | ||
Updated•9 years ago
|
Depends on: fix-readability
Comment 88•8 years ago
|
||
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•8 years ago
|
||
(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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Summary: Ship Readability in the desktop browser (reader mode) → [meta] Ship Readability in the desktop browser (reader mode)
You need to log in
before you can comment on or make changes to this bug.
Description
•