Add a 'Night Mode' to Reader Mode

RESOLVED FIXED in Firefox 24

Status

()

Firefox for Android
Reader View
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: lucasr, Assigned: Margaret)

Tracking

({feature})

unspecified
Firefox 24
All
Android
feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 24+)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
* Automatic background/foreground changing based on ambient light
* Maybe have 4 levels: Current light (white background), light gray background, dark gray background and Current dark (black background)
(Reporter)

Updated

5 years ago
Keywords: uiwanted

Comment 1

5 years ago
Hi Lucas, this would be an awesome feature to have!
I will try to start implement this~
You should be able to use the HTML5 Ambient light spec to do this (the spec is already implemented in Fennec).

http://www.w3.org/TR/ambient-light/

Updated

5 years ago
Blocks: 866766
Created attachment 747101 [details]
Reference mockup of different coloured backgrounds

Let's give it a whirl. I fear it might be a little to "clever" to flip the UI around automatically like this, but let's see how it feels in practice
(Assignee)

Comment 5

5 years ago
I can take this.
Assignee: nobody → margaret.leibovic
Keywords: uiwanted
(Assignee)

Comment 6

5 years ago
Created attachment 747557 [details] [diff] [review]
WIP

I still want to try tweaking some of the lux value thresholds, but this seems to be working better than some of my earlier attempts. I also wonder how consistent sensors are on different devices.

I found that it was too difficult to use 4 modes and avoid flickering between them, so I decided to just use 3 for now. Also, using "dim"/"normal"/"bright" aligns with the lightlevel event spec, which we may want to use if it's ever implemented.

I also need to write a follow-up patch to adjust to adjust the text settings popup, since adding an "Auto" button makes some of the alignment look off.

Here's an APK in case anyone wants to test:
https://dl.dropboxusercontent.com/u/3358452/nightmode.apk
Attachment #747557 - Flags: feedback?(lucasr.at.mozilla)
I'm worried about the text:background contrast ratio of the gray states, especially the #CCCCCC background. Seems like that could be hard to read, especially if the phone's brightness isn't high.
(Reporter)

Comment 8

5 years ago
Comment on attachment 747557 [details] [diff] [review]
WIP

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

Nice.

::: mobile/android/app/mobile.js
@@ +663,5 @@
>  pref("reader.margin_size", 5);
>  
> +// The default color scheme in reader (light, dark, sepia, auto)
> +// auto = color automatically adjusts according to ambient light level
> +pref("reader.color_scheme", "auto");

I agree but confirm with ibarlow.

::: mobile/android/chrome/content/aboutReader.js
@@ +54,5 @@
>    win.addEventListener("unload", this, false);
>    win.addEventListener("scroll", this, false);
>    win.addEventListener("popstate", this, false);
>    win.addEventListener("resize", this, false);
> +  win.addEventListener("devicelight", this, false);

You should probably only listen to this if the color scheme is set to 'auto'.

@@ +340,5 @@
> +      this._lightlevel = "normal";
> +    else
> +      this._lightlevel = "bright";
> +
> +    this._doc.body.setAttribute("lightlevel", this._lightlevel);

Is this lightlevel attribute part of the standard? Otherwise you should probably set a custom data attribute instead.

::: mobile/android/themes/core/aboutReader.css
@@ +9,5 @@
>  body {
>    margin-top: 20px;
>    margin-bottom: 20px;
> +  -moz-transition-property: background-color, color;
> +  -moz-transition-duration: 0.7s;

Uh, fancy background and font color transitions? Nice :-)

@@ +18,5 @@
>    background-color: #ffffff;
>    color: #222222;
>  }
>  
> +.auto[lightlevel="normal"] {

Better get feedback from ibarlow about the intermediary color scheme(s). Given that's adding only one intermediary color scheme instead of two.
Attachment #747557 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(In reply to Brian Nicholson (:bnicholson) from comment #7)
> I'm worried about the text:background contrast ratio of the gray states,
> especially the #CCCCCC background. Seems like that could be hard to read,
> especially if the phone's brightness isn't high.

I tend to agree -- I mentioned this in IRC yesterday, that changing the screen to grey is sort of unattractive. Looked ok in my mockup, not as much in practice :)

I wonder if we only want to shift colours between white on black, to black on white, and rely on Android's auto-brightness setting to do the rest of the in between work for us, if users have that turned on.
As an aside, walking from a bright hallway into a dark room and having my reader go from white to black was kind of a magical experience :)
(Assignee)

Comment 11

5 years ago
Created attachment 748146 [details] [diff] [review]
patch

This patch uses two modes, and I like it. I realized that we use the .light/.dark classes to style a bunch of things, so it's better to just use those two existing themes.

Updated APK available here:
https://dl.dropboxusercontent.com/u/3358452/nightmode.apk
Attachment #747557 - Attachment is obsolete: true
Attachment #748146 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 748146 [details] [diff] [review]
patch

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

>+  _handleDeviceLight: function Reader_handleDeviceLight(luxValue) {
>+    // Ignore changes that are within a certain threshold of previous lux values.
>+    if ((this._colorScheme === "dark" && luxValue < 50) ||
>+        (this._colorScheme === "light" && luxValue > 25))
>+      return;
>+
>+    if (luxValue < 30)

Maybe we should move these to named constants

One thing to watch, with 3 buttons across we seem to be messing up the "centering" of the popup menu a bit. It seems to be pushed off the left of the screen a little on my Galaxy Nexus. Maybe play with the button padding/margins a little?
Attachment #748146 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 748194 [details] [diff] [review]
(Part 2) Use flex to size segmented buttons in reader mode menu

This fixes the menu layout.

I don't know that those lux values need to be named constants, since they're only used in this one place.
Attachment #748194 - Flags: review?(mark.finkle)
(Assignee)

Comment 14

5 years ago
(In reply to Mark Finkle (:mfinkle) from comment #12)
> Comment on attachment 748146 [details] [diff] [review]
> patch
> 
> >diff --git a/mobile/android/chrome/content/aboutReader.js b/mobile/android/chrome/content/aboutReader.js
> 
> >+  _handleDeviceLight: function Reader_handleDeviceLight(luxValue) {
> >+    // Ignore changes that are within a certain threshold of previous lux values.
> >+    if ((this._colorScheme === "dark" && luxValue < 50) ||
> >+        (this._colorScheme === "light" && luxValue > 25))
> >+      return;
> >+
> >+    if (luxValue < 30)
> 
> Maybe we should move these to named constants

I don't know if that's really necessary, since these are just used in this one place. I feel like it's easier to understand what's going on if you just see the numbers, but maybe I'm biased from playing around with these values :)
Attachment #748194 - Flags: review?(mark.finkle) → review+

Comment 16

5 years ago
It's possible I've missed something in this bug addressing this, but, based on the example screenshot I wanted to add my two cents.
I get frequent light sensitivity (because my brain hates me) and so I use dark (stylish) themes on all pages.  One thing I've found from my experiments, is that plain/bright white text on a jet black background irritates very quickly (sort of leaves flash bulb shadows in my vision).  So for the right most example I would prefer a shade of grey.  #3 is more tolerable because of less contrast.
(And I agree with the previous assessment that #2 does not have enough contrast.)
https://hg.mozilla.org/mozilla-central/rev/d8f738b7da5c
https://hg.mozilla.org/mozilla-central/rev/d0344d91c8bf
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
We've unprefixed transitions long ago, no?
(Assignee)

Comment 19

5 years ago
(In reply to :Ms2ger from comment #18)
> We've unprefixed transitions long ago, no?

You are correct. I filed bug 874205.

Updated

5 years ago
Depends on: 875852

Updated

5 years ago
relnote-firefox: --- → ?

Updated

5 years ago
relnote-firefox: ? → 24+
Adding the feature keyword to be included in the new Release Tracking page.
Keywords: feature
You need to log in before you can comment on or make changes to this bug.