Click reader mode button to open a dialog

RESOLVED DUPLICATE of bug 795981

Status

()

Firefox
Toolbars and Customization
RESOLVED DUPLICATE of bug 795981
5 years ago
2 years ago

People

(Reporter: Michael Anderson, Assigned: Chelsea Carr)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Blocks: 558882
Version: unspecified → 18 Branch
(Reporter)

Updated

5 years ago
Version: 18 Branch → unspecified
Assignee: nobody → ande1474
Component: Untriaged → General
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Reporter)

Updated

5 years ago
Blocks: 793919
(Assignee)

Comment 1

5 years ago
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.
Attachment #664341 - Flags: review?(lucasr.at.mozilla)
Attachment #664341 - Flags: review?(jaws)
Assignee: ande1474 → carrche2
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.
Attachment #664341 - Flags: review?(lucasr.at.mozilla)
Attachment #664341 - Flags: review?(jaws)
Attachment #664341 - Flags: feedback+
(Assignee)

Comment 3

5 years ago
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.
Attachment #666243 - Flags: review?(lucasr.at.mozilla)
Attachment #666243 - Flags: review?(jaws)
(Assignee)

Comment 4

5 years ago
Created attachment 666253 [details] [diff] [review]
messed up previous patch.

I'm hoping this fixes the previous patch merge issue.
Attachment #666253 - Flags: review?(lucasr.at.mozilla)
Attachment #666253 - Flags: review?(jaws)
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 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.
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.
Component: General → Toolbars
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.
(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.
Attachment #664341 - Attachment is obsolete: true
Attachment #666243 - Attachment is obsolete: true
Attachment #666243 - Flags: review?(lucasr.at.mozilla)
Attachment #666243 - Flags: review?(jaws)
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).
Attachment #666253 - Flags: review?(lucasr.at.mozilla)
Attachment #666253 - Flags: review?(jaws)

Comment 11

2 years ago
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 795981
You need to log in before you can comment on or make changes to this bug.