1472957 - "Auto" option from Reading Mode is not responsive

Status: VERIFIED FIXED

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

Description

[Sorina Florean [:sflorean]]

Environment: 
Device: Samsung Galaxy Note8 (Android 8.0.0);
Build: Nightly 63.0a1 (2018-07-02);

Steps to reproduce:
1. Go to Wikipedia -> English;
2. Tap on the Reader Mode icon on URL Bar;
3. Tap on "Aa" option;
4. Select "Dark";
5. Change to "Auto" option.

Expected result:
Light theme is applied.

Actual result:
Dark theme is displayed, "auto" option is not responsive.

Notes:
See the video: https://drive.google.com/open?id=1_QIjt2dBflIwiDC089I4WbtbH_07Jrs7

Regression window:
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b996cabc7ef54bbe050d647494bf00d668ec52e6&tochange=0f8f71b0b9d84e7732c07f841e395de516b31b66

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

This relied on the ambient sensor API. See bug 1462308 and bug 1359076 comment 11.

Comment 2

[Mike Taylor [:miketaylr]]

jkt, do you know if we ever filed a follow up bug (or came up with a plan) for reader mode? https://bugzilla.mozilla.org/show_bug.cgi?id=1359076#c30

thanks

Comment 3

[Jonathan Kingston [:jkt] he/him]

Ah I'm sorry I dunno how this got missed.

The original idea was to either drop the feature or allow it only for this page/chrome context.

The video does make it a little worse than it actually is. The UI is still responsive after the user presses that button.

The code in https://searchfox.org/mozilla-central/rev/fd5c37f1dd9a0d1e327a6c6b4d81ea92f52c4330/dom/system/nsDeviceSensors.cpp#593 should be able to handle if the API is called from a priv context.

This will impact stable on the next release (62), if we think it's urgent enough perhaps we should uplift removing that button?

Comment 4

[Jonathan Kingston [:jkt] he/him]

The simple uplift would be commenting out the pref in:

mobile/android/app/mobile.js
  from pref("reader.color_scheme.values", "[\"dark\",\"auto\",\"light\"]");

to use
  modules/libpref/init/all.js
  value pref("reader.color_scheme.values", "[\"light\",\"dark\",\"sepia\"]");

This would keep the interface looking the same and be pretty risk free until we decide on a better plan here.

Comment 5

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1472957 - Disable auto reader mode for mobile as device light doens't work.

Review commit: https://reviewboard.mozilla.org/r/255578/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255578/

Comment 6

[Florian Quèze [:florian] (on leave until October 9)]

Comment on attachment 8990505 [details]
Bug 1472957 - Disable auto reader mode for mobile as device light doens't work.

https://reviewboard.mozilla.org/r/255578/#review262472

I'm not a peer for mobile, but this is so simple that I can rs it if this is what you were looking for.

Comment 7

[Jonathan Kingston [:jkt] he/him]

Adding :rnewman just as a precaution as I would like to uplift this if we can also. Thanks Florian

Comment 8

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Jonathan maybe it's enough to land it and then ask QA to verify on nightly? Or, is there someone else who can review?

Comment 9

[Jonathan Kingston [:jkt] he/him]

Landing as advised, as mentioned it should be fine :). Thanks

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46d48232c540
Disable auto reader mode for mobile as device light doens't work. r=florian

Comment 11

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/46d48232c540

Comment 12

[Sorina Florean [:sflorean]]

Tested with Nokia 6(Android 7.1.1) so verified as fixed on the latest Nightly build (07-19).
We have now dark, light, sepia options and changing from dark to sepia makes the light theme to be applied.

Comment 13

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8990505 [details]
Bug 1472957 - Disable auto reader mode for mobile as device light doens't work.

Approval Request Comment
[Feature/Bug causing the regression]: 1359076
[User impact if declined]: Broken button on fenec reader mode
[Is this code covered by automated tests?]: N/A
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a pref change
[String changes made/needed]: N/A

Comment 14

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8990505 [details]
Bug 1472957 - Disable auto reader mode for mobile as device light doens't work.

Simple fix to set the theme for reader mode. Let's take this for beta 11.

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a284daff752a

Comment 16

[Sorina Florean [:sflorean]]

Verified as fixed ob build 62 Beta 11.
Devices: Nexus 5(Android 6.0.1) and Samsung Galaxy Note8 (Android 8.0)