795981 - Add a reader mode button to the location bar

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

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).

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 2

[Chelsea Carr]

Attachment: 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

I added a declaration for ReaderMode that we will add more validation and functionality to later.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 4

[Chelsea Carr]

Attachment: Fixed the previous attachment. Added buttons to URL bar and fixed spacing. Also condensed function into readermode var

I'm leaving the previous attachment for reference of other fixes on this bug

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

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".

Comment 6

[:Margaret Leibovic]

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

Comment 8

[Michael Maslaney [:mmaslaney] ([e-mail])]

Hi Margaret, 

Would you like this asset in PNG or SVG?

Comment 9

[:Margaret Leibovic]

(In reply to Michael Maslaney [:mmaslaney] ([e-mail]) 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?

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

We still use PNG for all toolbar-related icons, so let's stick with PNG for this bug.

Comment 11

[:Margaret Leibovic]

Attachment: MozReview Request: bz://795981/margaret

Comment 12

[:Margaret Leibovic]

/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 13

[:Margaret Leibovic]

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 :)

Comment 14

[:Margaret Leibovic]

(In reply to Michael Maslaney [:mmaslaney] ([e-mail]) from comment #8)
> Hi Margaret, 
> 
> Would you like this asset in PNG or SVG?

PNG, please :)

Comment 17

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Readermode_Icon.zip

Icons attached.

Comment 18

[:Margaret Leibovic]

/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

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 20

[:Margaret Leibovic]

(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.

Comment 21

[:Margaret Leibovic]

(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.

Comment 22

[:Margaret Leibovic]

/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

Comment 23

[:Margaret Leibovic]

/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

Comment 24

[:Margaret Leibovic]

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.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

https://reviewboard.mozilla.org/r/1985/#review1751

Ship It!

Comment 29

[:Margaret Leibovic]

(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.

Comment 30

[:Margaret Leibovic]

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.

Comment 31

[:Margaret Leibovic]

Green try run: https://tbpl.mozilla.org/?tree=Try&rev=162f58455dba

https://hg.mozilla.org/integration/fx-team/rev/ad3867772421
https://hg.mozilla.org/integration/fx-team/rev/a5db16049be1
https://hg.mozilla.org/integration/fx-team/rev/739b6d124285

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ad3867772421
https://hg.mozilla.org/mozilla-central/rev/a5db16049be1
https://hg.mozilla.org/mozilla-central/rev/739b6d124285

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 34

[Andrei Vaida [:avaida]]

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.

Comment 36

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 795981 - (Part 3) Disable reader mode by default

Comment 37

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 795981 - (Part 2) Add reader mode button to toolbar

Comment 38

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 795981 - (Part 1) Hook up reader mode for desktop