Closed Bug 795981 Opened 8 years ago Closed 6 years ago

Add a reader mode button to the location bar

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 38
Tracking Status
firefox38 --- verified

People

(Reporter: jaws, Assigned: Margaret, NeedInfo)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

A reader mode button will need to be added to the location bar.

This button should only be visible if reader.enabled (bug 795973) is true.

When the button is clicked, it should navigate the current tab to about:reader (bug 795968).
Scratch that last part of this bug. This bug should only be to add the button the location bar and make it only visible if reader.enabled=true.

Bug 792286 will handle clicking on the button.
I added a declaration for ReaderMode that we will add more validation and functionality to later.
Attachment #669800 - Flags: review?(lucasr.at.mozilla)
Attachment #669800 - Flags: review?(jaws)
Comment on attachment 669800 [details] [diff] [review]
Added a button to the URL bar, in all 3 OSs.  Added in beginning functionality for manipulating the reader button.  Right now the button can be hidden based on what the pref "reader.enabled" is set to

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

::: browser/base/content/browser.js
@@ +1964,5 @@
>    }
>  }
>  
> +//Reader Mode On-Click
> +function BrowserOpenReaderMode(aEvent) {

This function should be part of the ReaderMode object.

@@ +1971,5 @@
> +
> +//Reader Hidden button
> +//function BrowserHideReaderModeButton() {
> +	//gBrowser.selectedBrowser.getElementById('reader-button').hidden = !gPrefService.getBoolPref('reader.enabled');
> +//}

This dead code can be deleted.

@@ +5606,5 @@
>      gPageStyleMenu.disableStyle();
>  }
>  
> +var ReaderMode = {
> +  _inited: false,

_inited is unused.

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

This is fine for now, but it will need to be changed later to use a preference observer. See https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIPrefBranch2 for more information.

::: browser/themes/gnomestripe/browser.css
@@ +1426,5 @@
>    list-style-image: url("chrome://browser/skin/places/pageStarred.png");
>  }
>  
> +/* Reader Button */
> +#star-button {

This doesn't seem correct.

::: browser/themes/winstripe/browser.css
@@ +1703,5 @@
> +/* reader button */
> +
> +#reader-button {
> +  list-style-image: url("chrome://browser/skin/places/reader.png");
> +  -moz-image-region: rect(0px 24px 24px 0px);

The reader icon should just be 16x16, so I'm not sure why the icon here is larger than that, and it must be even larger than 24x24 if -moz-image-region is needed.

::: browser/themes/winstripe/jar.mn
@@ +85,4 @@
>          skin/classic/browser/places/libraryToolbar.png               (places/libraryToolbar.png)
>          skin/classic/browser/places/starred48.png                    (places/starred48.png)
>          skin/classic/browser/places/unstarred48.png                  (places/unstarred48.png)
> +        skin/classic/browser/places/reader.png											 (places/reader.png)

You'll need to convert these tab characters to spaces (here and throughout the other files in this patch) and the blank line below can be removed.
Attachment #669800 - Flags: review?(lucasr.at.mozilla)
Attachment #669800 - Flags: review?(jaws)
Attachment #669800 - Flags: feedback+
I'm leaving the previous attachment for reference of other fixes on this bug
Attachment #670062 - Flags: review?(lucasr.at.mozilla)
Attachment #670062 - Flags: review?(jaws)
Attachment #670062 - Attachment is patch: true
Comment on attachment 670062 [details] [diff] [review]
Fixed the previous attachment.  Added buttons to URL bar and fixed spacing.  Also condensed function into readermode var

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

I see that the BrowserOpenReaderMode function got moved and one of the CSS issues was fixed. Are you planning on fixing the other issues that were mentioned in comment #3?

::: browser/base/content/browser.js
@@ +5607,5 @@
> +    document.getElementById("reader-button").hidden = gPrefService.getBoolPref("reader.enabled");
> +  },
> +  
> +  //Reader Mode On-Click
> +  BrowserOpenReaderMode: function(aEvent) {

This function doesn't need to have such a descriptive name now that it is a member of the ReaderMode object. It can be renamed to "openSelectedTab".
Attachment #670062 - Flags: review?(lucasr.at.mozilla)
Attachment #670062 - Flags: review?(jaws)
Attachment #670062 - Flags: review-
I'm going to pick this up.

mmaslaney, can you let me know if there is anything special I need to know about the design for this button? I'm planning to start by making the behavior match Android, and work off the mock-up here for the visual design:

http://people.mozilla.org/~mmaslaney/readermode/index.html
Assignee: ande1474 → margaret.leibovic
Flags: needinfo?(mmaslaney)
Attachment #669800 - Attachment is obsolete: true
Attachment #670062 - Attachment is obsolete: true
Duplicate of this bug: 795973
Hi Margaret, 

Would you like this asset in PNG or SVG?
Flags: needinfo?(mmaslaney) → needinfo?(margaret.leibovic)
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #8)
> Hi Margaret, 
> 
> Would you like this asset in PNG or SVG?

It's been a while since I've written a patch for desktop... Jared, can you help tell me what kind of assets I'll need for this button?
Flags: needinfo?(margaret.leibovic) → needinfo?(jaws)
We still use PNG for all toolbar-related icons, so let's stick with PNG for this bug.
Flags: needinfo?(jaws)
/r/1987 - Bug 795981 - (Part 1) Hook up reader mode for desktop
/r/1989 - Bug 795981 - (Part 2) Add reader mode button to toolbar

Pull down these commits:

hg pull review -r 937e5a6ca0c05a82eb895fc1b34fc313b5d1d22a
Comment on attachment 8543458 [details]
MozReview Request: bz://795981/margaret

I don't know how to ask for feedback through review board, so I'll just do it here :)

There's still a bunch of work to do here, but I want to make sure I'm on the right track. However, once we have the basic button functionality working, we can land this with "reader.parse-on-load.enabled" set to false, and the button should never appear.

You can look at mobile/android/chrome/content/content.js and mobile/android/chrome/content/Reader.js to see I took most of this new desktop logic from there :)
Attachment #8543458 - Flags: feedback?(jaws)
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #8)
> Hi Margaret, 
> 
> Would you like this asset in PNG or SVG?

PNG, please :)
Flags: needinfo?(mmaslaney)
Duplicate of this bug: 793919
Duplicate of this bug: 792286
Attached file Readermode_Icon.zip
Icons attached.
Flags: needinfo?(mmaslaney)
Blocks: 1120022
Attachment #8543458 - Flags: feedback?(jaws)
/r/1987 - Bug 795981 - (Part 1) Hook up reader mode for desktop
/r/1989 - Bug 795981 - (Part 2) Add reader mode button to toolbar

Pull down these commits:

hg pull review -r 4da9bf1124ef363bd28c7d71d7a5b13ceccfac21
https://reviewboard.mozilla.org/r/1985/#review1511

::: browser/base/content/browser.xul
(Diff revision 2)
> +                <image id="reader-mode-button"

Are we going to allow custom placements for this button or can it only exist within the location bar? This patch doesn't allow movement of the button.

::: browser/app/profile/firefox.js
(Diff revision 2)
> +// The default state of reader mode works on loaded a page.

This comment is confusing to me.

::: browser/app/profile/firefox.js
(Diff revision 2)
> +// Force to enable reader mode to parse on loaded a page.

This one reads equally awkward.

::: browser/themes/osx/browser.css
(Diff revision 2)
> +    width: 22px;

Does this 22px come from a 16px width, with 2px left and 2px right padding, and 1px left and 1px right border?

::: browser/modules/ReaderParent.jsm
(Diff revision 2)
> +     BrowserApp.deck.addEventListener("TabSelect", this, true);

Where is BrowserApp defined? I see it is referenced within /mobile on MXR but I see no references to it within the desktop browser codebase.

::: browser/modules/ReaderParent.jsm
(Diff revision 2)
> +    if (pair[0] !== "url") {

Please use https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams here.

::: browser/base/content/content.js
(Diff revision 2)
> +      case "Reader:SavedArticleGet":

Are there plans for making this store more than one article in memory?

Or is it storing one article per tab? The fact that the JSM needs to get the specific message manager for the browser makes me think that there might be one 'content' object per 'browser'.

::: toolkit/components/reader/ReaderMode.jsm
(Diff revision 2)
> +    // XXX: This isn't working in the content process.

Should this block of code be deleted if it is dead? How similar is it to the code at ReaderParent._getArticle?

::: browser/base/content/content.js
(Diff revision 2)
> +    return content.document.documentURI.startsWith("about:reader");

Do we want to blacklist more pages than just about:reader? I'm not sure if we want pages like about:credits to get the Reader Mode treatment, but maybe that is not that big of a deal for v1.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19)

Thanks for the feedback!

> https://reviewboard.mozilla.org/r/1985/#review1511
> 
> ::: browser/base/content/browser.xul
> (Diff revision 2)
> > +                <image id="reader-mode-button"
> 
> Are we going to allow custom placements for this button or can it only exist
> within the location bar? This patch doesn't allow movement of the button.

Good question. I was just basing it off of the mock-ups, but maybe we do want this to be customizable. NI to mmaslaney to handle this one.

> ::: browser/app/profile/firefox.js
> (Diff revision 2)
> > +// The default state of reader mode works on loaded a page.
> 
> This comment is confusing to me.
> 
> ::: browser/app/profile/firefox.js
> (Diff revision 2)
> > +// Force to enable reader mode to parse on loaded a page.
> 
> This one reads equally awkward.

I copied these out of mobile.js without reading them, but I agree, I can update them to make them more clear (and maybe actually put the defaults in all.js, since these are currently identical to what we have in mobile.js).

> ::: browser/themes/osx/browser.css
> (Diff revision 2)
> > +    width: 22px;
> 
> Does this 22px come from a 16px width, with 2px left and 2px right padding,
> and 1px left and 1px right border?

I was trying to use the inspector to figure out what was going on here, but I couldn't... I just copied this from the styles used for #page-report-button. Maybe you can help me figure out why we have that for #page-report-button? And why we don't have it for other platforms?

> ::: browser/modules/ReaderParent.jsm
> (Diff revision 2)
> > +     BrowserApp.deck.addEventListener("TabSelect", this, true);
> 
> Where is BrowserApp defined? I see it is referenced within /mobile on MXR
> but I see no references to it within the desktop browser codebase.

Oop, that should be removed. Copy/paste mistake from Fennec's Reader.js (BrowserApp is like our gBrowser).

> ::: browser/modules/ReaderParent.jsm
> (Diff revision 2)
> > +    if (pair[0] !== "url") {
> 
> Please use https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams
> here.

Good pointer, thanks.

> ::: browser/base/content/content.js
> (Diff revision 2)
> > +      case "Reader:SavedArticleGet":
> 
> Are there plans for making this store more than one article in memory?
>
> Or is it storing one article per tab? The fact that the JSM needs to get the
> specific message manager for the browser makes me think that there might be
> one 'content' object per 'browser'.

AIUI, one content.js gets loaded for each browser, so this will hold one article in memory for every browser. This is what we currently do for Fennec, so I was just modeling it after that (well, we used to just store the article directly on our tab object, but I made it more generic for the sake of sharing logic between desktop/mobile).

> ::: toolkit/components/reader/ReaderMode.jsm
> (Diff revision 2)
> > +    // XXX: This isn't working in the content process.
> 
> Should this block of code be deleted if it is dead? How similar is it to the
> code at ReaderParent._getArticle?

Yeah, we could probably just delete that. This code has been evolving as I've been refactoring it, so what made sense once might not make sense anymore. Having this cache check would be nice to avoid extra work, but we can punt on that to another bug to see if we can make this work in an e10s friendly way (it appears the problem I'm running into is that the crypto methods we're using to create hashed file names for the articles isn't working in the content process, but maybe there's a way to work around that, or change the logic to work in the content process).

> ::: browser/base/content/content.js
> (Diff revision 2)
> > +    return content.document.documentURI.startsWith("about:reader");
> 
> Do we want to blacklist more pages than just about:reader? I'm not sure if
> we want pages like about:credits to get the Reader Mode treatment, but maybe
> that is not that big of a deal for v1.

We actually have logic in ReaderMode.parseDocument that will bail early for about: URLs:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm#186

Maybe we don't actually need this isAboutReader check in the pageshow listener, since it will be handled more generally in parseDocument.
Flags: needinfo?(mmaslaney)
(In reply to :Margaret Leibovic from comment #20)

> > ::: browser/themes/osx/browser.css
> > (Diff revision 2)
> > > +    width: 22px;
> > 
> > Does this 22px come from a 16px width, with 2px left and 2px right padding,
> > and 1px left and 1px right border?
> 
> I was trying to use the inspector to figure out what was going on here, but
> I couldn't... I just copied this from the styles used for
> #page-report-button. Maybe you can help me figure out why we have that for
> #page-report-button? And why we don't have it for other platforms?

I figured this out for myself. This is because there's 3px left/right padding set on .urlbar-icon:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#2188

I was also confused about why this width was only declared for the 2x case, but it now makes sense to me that we're "scaling down" the 32px icon to fit in a 16px space.
Attachment #8543458 - Flags: review?(mark.finkle)
Attachment #8543458 - Flags: review?(jaws)
/r/1987 - Bug 795981 - (Part 1) Hook up reader mode for desktop
/r/1989 - Bug 795981 - (Part 2) Add reader mode button to toolbar
/r/2371 - Bug 795981 - (Part 3) Disable reader mode by default

Pull down these commits:

hg pull review -r 7d1e4220aec246c1d1481ffea8383e3f843865c1
/r/1987 - Bug 795981 - (Part 1) Hook up reader mode for desktop
/r/1989 - Bug 795981 - (Part 2) Add reader mode button to toolbar
/r/2371 - Bug 795981 - (Part 3) Disable reader mode by default

Pull down these commits:

hg pull review -r 54ee18f625f2d3b9f48604c361a13cfdee6fa11e
I flagged mfinkle for review on the second patch because I updated some mobile code in order to keep the desktop/mobile implementations in sync.

I think we should land this disabled, and keep it disabled until we fix bug 1117258.

Also, I need to write some tests for this... any pointers for where to get started with that? Probably we should start with a simple browser-chrome tests that flips the parse-on-load pref, loads a reader-mode-able page, and then checks that the button is visible. Then after that we could also click the button and make sure about:reader loads correctly.

Also, if UX decides this button should be customiazble, we can address that in a follow-up bug.
Attachment #8543458 - Flags: review?(mark.finkle) → review+
https://reviewboard.mozilla.org/r/1985/#review1565

The mobile parts look good. Using the <browser> makes sense as a cross-platform/cross-product way of providing some consistency.
(In reply to :Margaret Leibovic from comment #24)
> Also, I need to write some tests for this... any pointers for where to get
> started with that?

I briefly looked through browser/components/customizableui/test and browser/base/content/test to see if there were any tests that referred to a button that flips its enabled state based on the page content and then acts upon it when it is enabled but I didn't see one. We do have a few of these types of buttons already though. The Character Encoding button, Bookmarks button, and the Share Page button all can get disabled via various reasons. They are all disabled for about:blank.

> Probably we should start with a simple browser-chrome
> tests that flips the parse-on-load pref, loads a reader-mode-able page, and
> then checks that the button is visible. Then after that we could also click
> the button and make sure about:reader loads correctly.

Yeah, that sounds good. Is there a way that we can make sure that some of the content pre-Reader Mode is removed from the page? This may provide a very simple "is the Reader Mode JS code working" sanity check.
https://reviewboard.mozilla.org/r/1985/#review1745

::: browser/modules/ReaderParent.jsm
(Diff revisions 2 - 4)
> -      case "Reader:UpdateReaderButton":
> +      case "Reader:UpdateReaderButton": {

Nit, we don't need the extra set of braces surrounding the body of this case statement. The other case blocks above don't have them.
Attachment #8543458 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #26)

> > Probably we should start with a simple browser-chrome
> > tests that flips the parse-on-load pref, loads a reader-mode-able page, and
> > then checks that the button is visible. Then after that we could also click
> > the button and make sure about:reader loads correctly.
> 
> Yeah, that sounds good. Is there a way that we can make sure that some of
> the content pre-Reader Mode is removed from the page? This may provide a
> very simple "is the Reader Mode JS code working" sanity check.

We currently do have some mobile tests that cover this shared code. We have a basic test to parse a few different test pages, and we check whether or not they actually get parsed as articles:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testReadingListCache.js#70

But I'll file a follow-up bug to get some desktop tests in place for the desktop UI.
https://reviewboard.mozilla.org/r/1985/#review1785

> Nit, we don't need the extra set of braces surrounding the body of this case statement. The other case blocks above don't have them.

I believe we do need these braces because we declare a local variable within the body of the case statement. It may be a bit overly defensive, but this is a convention we follow in our Fennec JS.
Depends on: 1123471
https://reviewboard.mozilla.org/r/1985/#review1803

> I believe we do need these braces because we declare a local variable within the body of the case statement. It may be a bit overly defensive, but this is a convention we follow in our Fennec JS.

OMG, I swear I looked for a local `let` variable and never saw it. Yes, this change looks good to me. Thanks for the reply.
Depends on: 1124217
Depends on: 1124271
Depends on: 1125364
Depends on: 1125396
Verified fixed on Nightly 38.0a1 (2015-02-20), using Windows 7 (x64), Windows 8.1 (x86), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Filed a separate bug for the button's compatibility with high contrast Windows themes.
Status: RESOLVED → VERIFIED
Depends on: 1135587
Flags: qe-verify+
Depends on: 1136704
Depends on: 1134658
Attachment #8543458 - Attachment is obsolete: true
Attachment #8618000 - Flags: review+
Attachment #8618001 - Flags: review+
Attachment #8618002 - Flags: review+
You need to log in before you can comment on or make changes to this bug.