Closed Bug 1472957 Opened 5 years ago Closed 5 years ago
"Auto" option from Reading Mode is not responsive
59 bytes, text/x-review-board-request
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
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
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?
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 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.
Attachment #8990505 - Flags: review?(florian) → review+
Adding :rnewman just as a precaution as I would like to uplift this if we can also. Thanks Florian
Jonathan maybe it's enough to land it and then ask QA to verify on nightly? Or, is there someone else who can review?
Landing as advised, as mentioned it should be fine :). Thanks
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/46d48232c540 Disable auto reader mode for mobile as device light doens't work. r=florian
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 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
Attachment #8990505 - Flags: approval-mozilla-beta?
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.
Attachment #8990505 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed ob build 62 Beta 11. Devices: Nexus 5(Android 6.0.1) and Samsung Galaxy Note8 (Android 8.0)
You need to log in before you can comment on or make changes to this bug.