Closed Bug 750686 Opened 12 years ago Closed 12 years ago

Reader Mode: Implement reader style toolbar

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox21 verified)

VERIFIED FIXED
Firefox 16
Tracking Status
firefox21 --- verified

People

(Reporter: lucasr, Unassigned)

References

Details

(Keywords: uiwanted)

Attachments

(3 files, 2 obsolete files)

The toolbar would probably show up on screen tap and allow user to tweak font style and size and color scheme. See design page for some initial ideas:

https://wiki.mozilla.org/Fennec/NativeUI/UserExperience/ReaderMode
Blocks: reader
Attached image Updated toolbar UI
Lucas, attached is a new toolbar / menu design for you to work from. Note there are a few changes from Patryk's original design

1. Moving the font + and - out of the toolbar and into the menu, so all page layout controls can live in the same place.

2. Changing Font and Margin controls from sliders to +/- buttons

Also included here are some default font specs we can start with. The text is Droid Serif for now, because I am still looking for a good open source serif font to use. We'll continue to massage the type in follow-up bugs -- this is a good place to start though.
Attachment #634012 - Flags: review?(mark.finkle)
Attached image Screenshot
The only thing missing in this patch is the actual icons for each screen density to use in the UI. I'm still waiting for Ian to send those. But the code is ready for review. Here's how it looks now.
Comment on attachment 634012 [details] [diff] [review]
Implement style toolbar for Reader Mode

>diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js

>+pref("reader.color_scheme", "light");
>\ No newline at end of file

Add a newline

>diff --git a/mobile/android/chrome/content/aboutReader.html b/mobile/android/chrome/content/aboutReader.html
>--- a/mobile/android/chrome/content/aboutReader.html
>+++ b/mobile/android/chrome/content/aboutReader.html
>@@ -1,24 +1,38 @@
> <!DOCTYPE html>
> <html>
> 
> <head>
>   <title></title>
>   <meta content="text/html; charset=UTF-8" http-equiv="content-type">
>-  <meta name="viewport" content="width=480; initial-scale=.6667; user-scalable=0" />
>+  <meta name="viewport" content="width=device-width; user-scalable=0" />
> 
>   <link rel="icon" type="image/png" href="chrome://branding/content/favicon32.png" />
>   <link rel="stylesheet" href="chrome://browser/skin/aboutReader.css" type="text/css"/>
> </head>
> 
> <body onload="AboutReader.init();" onunload="AboutReader.uninit();">
>   <div id="reader-header" class="header">
>   </div>
> 
>   <div id="reader-content" class="content">
>   </div>
> 
>+  <ul id="reader-toolbar" class="toolbar">
>+    <a id="share-button" class="button share-button" href="#"></a>
>+    <li class="dropdown">
>+      <a class="dropdown-toggle button style-button" href="#"></a>
>+      <div class="dropdown-popup">
>+        <ul id="color-scheme-buttons" class="segmented-button"></ul>
>+        <hr></hr>
>+        <div id="font-size-control" class="step-control"></div>
>+        <hr></hr>
>+        <div id="margin-size-control" class="step-control"></div>
>+      </div>
>+    </li>
>+  </ul>
>+
>   <script type="application/javascript;version=1.8" src="chrome://browser/content/aboutReader.js">
>   </script>
> </body>
> 
> </html>

>diff --git a/mobile/android/chrome/content/aboutReader.js b/mobile/android/chrome/content/aboutReader.js

> let AboutReader = {
>+  _STEP_INCREMENT: 0,
>+  _STEP_DECREMENT: 1,
>+
>   init: function Reader_init() {
>     dump("Init()");
> 
>-    dump("Feching header and content notes from about:reader");
>+    this._article = null;
>+
>+    dump("Feching toolbar, header and content notes from about:reader");
>     this._titleElement = document.getElementById("reader-header");
>     this._contentElement = document.getElementById("reader-content");
>+    this._toolbarElement = document.getElementById("reader-toolbar");
>+
>+    this._scrollOffset = window.pageYOffset;
>+
>+    this._onToolbarTransitionEnd = function() {
>+      if (!this._getToolbarVisibility())
>+        this._toolbarElement.display = "none";
>+    }.bind(this);
>+
>+    this._toolbarElement.addEventListener("transitionend", this._onToolbarTransitionEnd, true);
>+
>+    let body = document.body;
>+    body.addEventListener("touchstart", this, false);
>+    body.addEventListener("click", this, false);
>+    window.addEventListener("scroll", this, false);
>+
>+    this._setupAllDropdowns();
>+    this._setupButton("share-button", this._onShare.bind(this));
>+
>+    let colorSchemeOptions = [
>+      { name: gStrings.GetStringFromName("aboutReader.colorSchemeLight"),
>+        value: "light"},
>+      { name: gStrings.GetStringFromName("aboutReader.colorSchemeDark"),
>+        value: "dark"},
>+      { name: gStrings.GetStringFromName("aboutReader.colorSchemeSepia"),
>+        value: "sepia"}
>+    ];
>+
>+    let colorScheme = Services.prefs.getCharPref("reader.color_scheme");
>+    this._setupSegmentedButton("color-scheme-buttons",
>+                               colorSchemeOptions, colorScheme,
>+                               this._setColorScheme.bind(this));

nit: OK leave this on one line

>+    let fontTitle = gStrings.GetStringFromName("aboutReader.fontTitle");
>+    this._setupStepControl("font-size-control", fontTitle, this._onFontSizeChange.bind(this));
>+    this._fontSize = 0;
>+    this._setFontSize(Services.prefs.getIntPref("reader.font_size"));
>+
>+    let marginTitle = gStrings.GetStringFromName("aboutReader.marginTitle");
>+    this._setupStepControl("margin-size-control", marginTitle, this._onMarginSizeChange.bind(this));
>+    this._marginSize = 0;
>+    this._setMarginSize(Services.prefs.getIntPref("reader.margin_size"));

We might want to move the "step control" into XBL to make it a first class widget. Could be done later.

>   uninit: function Reader_uninit() {

>     delete this._titleElement;
>     delete this._contentElement;
>+    delete this._toolbarElement;

These all seem to be real DOM elements. Why do we need to delete them?

>+  _onMarginSizeChange: function Reader_onMarginSizeChange(operation) {
>+    if (operation == this._STEP_INCREMENT) {
>+      this._setMarginSize(this._marginSize + 5);
>+    } else {
>+      this._setMarginSize(this._marginSize - 5);
>+    }

nit: You don't need to use {} on one-line blocks. I notice that you mix styles in the file.

>+  _setMarginSize: function Reader_setMarginSize(newMarginSize) {

>+    this._marginSize = Math.max(5, Math.min(25, newMarginSize));
>+    document.body.style.margin = "20px " + this._marginSize + "%";

Hard coding the top/bottom "20px" seems fragile. Can we just use style.marginLeft and marginRight ?

r+, but check in with the real assets
Attachment #634012 - Flags: review?(mark.finkle) → review+
Attachment #634367 - Flags: review?(mark.finkle)
Attachment #634012 - Attachment is obsolete: true
Comment on attachment 634367 [details] [diff] [review]
Implement style toolbar for Reader Mode

Is this patch different?
I had to make quite a few changes in the css in order to apply the assets from Ian. I thought it would be worth having a second review on it.
Attachment #634370 - Flags: review?(mark.finkle)
Attachment #634367 - Attachment is obsolete: true
Attachment #634367 - Flags: review?(mark.finkle)
Comment on attachment 634370 [details] [diff] [review]
Implement style toolbar for Reader Mode

Patch is OK, but, again, I'd like to reduce the amount and size of the images. Can we:
* Only use one size of image for all DPIs?
* Run the images through a pngcrush tool to make them as small as possible?
* Make the background images smaller, so they aren't as big?

I am OK with followup bugs for any of these solutions
Attachment #634370 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #9)
> Comment on attachment 634370 [details] [diff] [review]
> Implement style toolbar for Reader Mode
> 
> Patch is OK, but, again, I'd like to reduce the amount and size of the
> images. Can we:
> * Only use one size of image for all DPIs?
> * Run the images through a pngcrush tool to make them as small as possible?
> * Make the background images smaller, so they aren't as big?
> 
> I am OK with followup bugs for any of these solutions

I filed bug 766164 to track this.
https://hg.mozilla.org/mozilla-central/rev/09d55edf687e
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Depends on: 766935
Depends on: 767121
Closing bug as verified fixed on:

Firefox for Android
Version: 21.0a1 (2013-01-29)
Device: Galaxy R
OS: Android 2.3.4
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: