Closed
Bug 750686
Opened 13 years ago
Closed 12 years ago
Reader Mode: Implement reader style toolbar
Categories
(Firefox for Android Graveyard :: General, defect)
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)
Updated toolbar UI
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
|
||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
Attachment #634012 -
Flags: review?(mark.finkle)
|
|
Reporter | |
Comment 3•12 years ago
|
||
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 4•12 years ago
|
||
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•12 years ago
|
||
Attachment #634367 -
Flags: review?(mark.finkle)
|
|
Reporter | |
Updated•12 years ago
|
Attachment #634012 -
Attachment is obsolete: true
|
|
||
Comment 6•12 years ago
|
||
Comment on attachment 634367 [details] [diff] [review]
Implement style toolbar for Reader Mode
Is this patch different?
|
|
Reporter | |
Comment 7•12 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•12 years ago
|
||
Attachment #634370 -
Flags: review?(mark.finkle)
|
|
Reporter | |
Updated•12 years ago
|
Attachment #634367 -
Attachment is obsolete: true
Attachment #634367 -
Flags: review?(mark.finkle)
|
|
||
Comment 9•12 years ago
|
||
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•12 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.
|
|
Reporter | |
Comment 11•12 years ago
|
||
|
||
Comment 12•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
|
||
Comment 13•12 years ago
|
||
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
|
Assignee | |
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•