857987 - Add a 'Night Mode' to Reader Mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Lucas Rocha (:lucasr)]

* 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)

Comment 1

[Bruce Wu]

Hi Lucas, this would be an awesome feature to have!
I will try to start implement this~

Comment 2

[Wesley Johnston (:wesj)]

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/

Comment 3

[Lucas Rocha (:lucasr)]

Also, have a look at:
https://hacks.mozilla.org/2013/04/ambient-light-events-and-javascript-detection/

Comment 4

[Ian Barlow (:ibarlow)]

Attachment: 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

Comment 5

[:Margaret Leibovic]

I can take this.

Comment 6

[:Margaret Leibovic]

Attachment: 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

Comment 7

[Brian Nicholson (:bnicholson)]

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.

Comment 8

[Lucas Rocha (:lucasr)]

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.

Comment 9

[Ian Barlow (:ibarlow)]

(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.

Comment 10

[Ian Barlow (:ibarlow)]

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 :)

Comment 11

[:Margaret Leibovic]

Attachment: 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

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 13

[:Margaret Leibovic]

Attachment: (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.

Comment 14

[:Margaret Leibovic]

(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 :)

Comment 15

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8f738b7da5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0344d91c8bf

Comment 16

[Caspy7]

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.)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8f738b7da5c
https://hg.mozilla.org/mozilla-central/rev/d0344d91c8bf

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We've unprefixed transitions long ago, no?

Comment 19

[:Margaret Leibovic]

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

You are correct. I filed bug 874205.

Comment 20

[Alex Keybl [:akeybl]]

Adding the feature keyword to be included in the new Release Tracking page.