Reader Mode: Implement reader style toolbar

VERIFIED FIXED in Firefox 21

Status

()

VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: lucasr, Unassigned)

Tracking

({uiwanted})

unspecified
Firefox 16
All
Android
uiwanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox21 verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Updated

6 years ago
Blocks: 696921
Created attachment 632493 [details]
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.
(Reporter)

Comment 2

6 years ago
Created attachment 634012 [details] [diff] [review]
Implement style toolbar for Reader Mode
Attachment #634012 - Flags: review?(mark.finkle)
(Reporter)

Comment 3

6 years ago
Created attachment 634013 [details]
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+
(Reporter)

Comment 5

6 years ago
Created attachment 634367 [details] [diff] [review]
Implement style toolbar for Reader Mode
Attachment #634367 - Flags: review?(mark.finkle)
(Reporter)

Updated

6 years ago
Attachment #634012 - Attachment is obsolete: true
Comment on attachment 634367 [details] [diff] [review]
Implement style toolbar for Reader Mode

Is this patch different?
(Reporter)

Comment 7

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

Comment 8

6 years ago
Created attachment 634370 [details] [diff] [review]
Implement style toolbar for Reader Mode
Attachment #634370 - Flags: review?(mark.finkle)
(Reporter)

Updated

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

Comment 10

6 years ago
(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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16

Updated

6 years ago
Depends on: 766935

Updated

6 years ago
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
status-firefox21: --- → verified
You need to log in before you can comment on or make changes to this bug.