Last Comment Bug 792286 - Click reader mode button to open a dialog
: Click reader mode button to open a dialog
Status: RESOLVED DUPLICATE of bug 795981
:
Product: Firefox
Classification: Client Software
Component: Toolbars and Customization (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Chelsea Carr
:
Mentors:
Depends on:
Blocks: desktop-reader 793919
  Show dependency treegraph
 
Reported: 2012-09-18 18:37 PDT by Michael Anderson
Modified: 2015-01-02 17:37 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Added a button to the toolbar for reader mode. window pops up when button is clicked. (25.28 KB, patch)
2012-09-24 20:43 PDT, Chelsea Carr
jaws: feedback+
Details | Diff | Review
Moved icon from toolbar to urlbar. Cleaned up code. Added button to all 3 OSs (12.28 KB, patch)
2012-09-29 13:14 PDT, Chelsea Carr
no flags Details | Diff | Review
messed up previous patch. (42.97 KB, patch)
2012-09-29 14:59 PDT, Chelsea Carr
no flags Details | Diff | Review

Description Michael Anderson 2012-09-18 18:37:10 PDT
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_4) AppleWebKit/536.25 (KHTML, like Gecko) Version/6.0 Safari/536.25

Steps to reproduce:

Click on the reader mode button



Expected results:

Reader button should be on toolbar and open a dialog when clicked
Comment 1 Chelsea Carr 2012-09-24 20:43:34 PDT
Created attachment 664341 [details] [diff] [review]
Added a button to the toolbar for reader mode.  window pops up when button is clicked.

This only works for mac right now as we only made the complete changes to pinstripe/browser.css.
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2012-09-25 12:06:26 PDT
Comment on attachment 664341 [details] [diff] [review]
Added a button to the toolbar for reader mode.  window pops up when button is clicked.

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

This patch can't be checkin in in the state that it is in, but I will give some general feedback on the patch-so-far.

I see a pinstripe and winstripe CSS changes. You'll also need to make changes for gnomestripe, and will need to setup a linux build environment to test out those changes.

::: browser/base/content/browser.js
@@ +1963,5 @@
>    }
>  }
>  
> +function BrowserOpenReaderMode(aEvent) {
> + openDialog("http://www.google.com","dlg", " ", "reader", "1");

Replace this with:
gBrowser.selectedBrowser.contentWindow.location = "http://www.google.com";

This will change the location of the current tab to your location of choice. Eventually this could be something like "about:reader?http://www.google.com" (glossing over a lot of details though).

@@ +1967,5 @@
> + openDialog("http://www.google.com","dlg", " ", "reader", "1");
> +}
> +
> +
> +

nit: only have one blank line between functions.

::: browser/base/content/browser.xul
@@ +663,5 @@
>                       ondragexit="homeButtonObserver.onDragExit(event)"
>                       onclick="BrowserGoHome(event);"
>                       aboutHomeOverrideTooltip="&abouthome.pageTitle;"/>
> +	  
> +	  <toolbaritem id="reader-toolbar-button">

This button should be located within the location bar, in the <hbox id="urlbar-icons"> container.

@@ +670,5 @@
> +                     label="&homeButton.label;"
> +                     onclick="BrowserOpenReaderMode(event);"
> +                     aboutHomeOverrideTooltip="&abouthome.pageTitle;"/>
> +    </toolbaritem>
> +	  

nit: make sure not to include lines with blank whitespace, and also to use spaces instead of tabs. Firefox code uses 2-space indents in majority of places. When in doubt, follow the style of the surrounding code.
Comment 3 Chelsea Carr 2012-09-29 13:14:06 PDT
Created attachment 666243 [details] [diff] [review]
Moved icon from toolbar to urlbar.  Cleaned up code.  Added button to all 3 OSs

Hopefully I got all the code cleaned up.  Please let me know if I missed any coding standards.
Comment 4 Chelsea Carr 2012-09-29 14:59:39 PDT
Created attachment 666253 [details] [diff] [review]
messed up previous patch.

I'm hoping this fixes the previous patch merge issue.
Comment 5 Lucas Rocha (:lucasr) 2012-10-01 01:50:31 PDT
I assume you're copying the icon from mobile to use on desktop? If so, the icon you probably want is located at:

  mobile/android/base/resources/drawable/reader.png
Comment 6 Lucas Rocha (:lucasr) 2012-10-01 01:57:42 PDT
Comment on attachment 666253 [details] [diff] [review]
messed up previous patch.

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

::: browser/base/content/browser.js
@@ +1963,5 @@
>    }
>  }
>  
> +function BrowserOpenReaderMode(aEvent) {
> +  gBrowser.selectedBrowser.contentWindow.location = "chrome://browser/content/aboutReader.html";

You haven't included the aboutReader.html file in the patch, have you? What you probably want to do is to setup a new "about:" page for reader (just like in mobile). This way you'd access this page using a "about:reader" URL instead of the direct reference to the file. Also, this page has to be unprivileged for security reasons.

::: browser/themes/gnomestripe/browser.css
@@ +1448,5 @@
>  
> +/* Reader BUTTON */
> +
> +#reader-button {
> +  list-style-image: url("chrome://browser/skin/places/reader-toggle-off-icon-land-mdpi.png");

No need to name the icon file this way. The mdpi/land bits only apply to the multi-density support in mobile.

::: mobile/android/chrome/content/aboutReader.js
@@ +136,4 @@
>      return this._toolbarElementRef.get();
>    },
>  
> +  observe: function Reader_observe(aMessage, aTopic, aData) {

This change seems unrelated.
Comment 7 Dão Gottwald [:dao] 2012-10-01 02:34:39 PDT
This will need UI review.

It might be more reasonable to put this in secondary UI for now, e.g. menus and an optional toolbar button. I don't like how new toolbar items are steadily creeping into the default toolbar set and I'd like to remind everyone of the 80/20/1 rule.
Comment 8 Dão Gottwald [:dao] 2012-10-01 02:37:22 PDT
Oh, so you're moving the button into the location bar in the second patch. (Not sure what the point of the first patch is.) This mitigates my concern, although it's still additional weight on the primary UI.
Comment 9 Lucas Rocha (:lucasr) 2012-10-01 07:12:40 PDT
(In reply to Dão Gottwald [:dao] from comment #8)
> Oh, so you're moving the button into the location bar in the second patch.
> (Not sure what the point of the first patch is.) This mitigates my concern,
> although it's still additional weight on the primary UI.

The intended (final) behaviour is to only show the icon when the page content has readable content (i.e. article-line page). I think, for now, they are just adding the button to be always visible to get things moving.
Comment 10 Jared Wein [:jaws] (please needinfo? me) 2012-10-01 10:22:03 PDT
Comment on attachment 666253 [details] [diff] [review]
messed up previous patch.

This bug now has a much narrower scope.

Bug 795968 will handle adding the about:reader page.
Bug 795981 will handle adding the button the location bar.

This bug will be about making that button navigate the page to about:reader with the correct arguments appended (eg. about:reader?http://www.cnn.com/article1).
Comment 11 :Margaret Leibovic 2015-01-02 17:37:42 PST
(In reply to (Limited availability until January 6) Jared Wein [:jaws] (please 

> This bug will be about making that button navigate the page to about:reader
> with the correct arguments appended (eg.
> about:reader?http://www.cnn.com/article1).

My patch in bug 795981 does exactly this.

*** This bug has been marked as a duplicate of bug 795981 ***

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