1495344 - Sepia option for Reader View is not showing correctly

Status: VERIFIED FIXED

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

Description

[yekit]

Attachment: Screenshot_2018-09-30-09-59-54-321_org.mozilla.firefox.png

User Agent: Mozilla/5.0 (Android 8.0.0; Mobile; rv:62.0) Gecko/62.0 Firefox/62.0
Build ID: 20180920131237

Steps to reproduce:

Tap on Reader View, tap on Reader View option, select "Sepia" mode


Actual results:

It is showing "Light" mode 


Expected results:

It should show "Sepia" mode

Comment 1

[Jan Henning [:JanH]]

The middle option used to be an automatic mode, so maybe the change to "Sepia" wasn't properly implemented?

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

Likely needs the styles from desktop to be correctly referenced.

Comment 3

[u633275]

This is the bug I came to report. Here's an odd thing I noticed: Go to an article on chicagotribune.com/toggle between sepia and light. The only thing that changes is the color of the author's name.

Comment 4

[mrninko]

Will be fixed?

Comment 5

[rahul.rajesh.bhat]

Hi,

I am interested in starting to contribute to Mozilla
Could I work on this issue?

Comment 6

[sonali18317]

Hi,

I was using the Reader View on Android and found that Sepia did not work. I was about to create a bug just when I found that it already exists here. I was curious, so I started investigating the related code.

Here are some findings:
-Before introducing the "Sepia" option there was an "Auto" option which changed the theme automatically - to know more about this, look at Bug 1472957
-The commit for this change, did not add the relevant CSS attributes and hence the feature did not work.
-There is still a lot of unused code (earlier for auto mode) which is kept, so that auto mode could perhaps be introduced back.

The color codes for Sepia are not specified anywhere, I have chosen one of the color code from toolkit files. If they have to be changed, kindly let me know the updated colors and I will make the corresponding changes.
Also, who should I specify as a reviewer for this bug?

Comment 7

[sonali18317]

Attachment: Bug 1495344 Sepia option for Reader View android shows correctly

Comment 8

[Krzysztof]

(In reply to sonali18317 from comment #6)

-There is still a lot of unused code (earlier for auto mode) which is kept, so that auto mode could perhaps be introduced back.

Right, please don't remove this, some people are still using the auto mode.

Comment 9

[:Gijs (he/him)]

:JanH, could you or someone else on the android teams mentor this and/or review the patch? If not, can we remove the [good first bug] whiteboard tag?

Comment 10

[Jan Henning [:JanH]]

Yes, I can take care of this. Keeping the NI to remind me to fix the two nits and and land this myself before merge day if Sonali doesn't come back.

Comment 11

[sonali18317]

Gijs, thanks for finding a mentor for this bug :)
JanH, I'll fix them within 24 hrs positively. I have an exam today, sorry for the delay.

Comment 12

[sonali18317]

JanH, I am new to versioning. I tried using hg commit --amend to update the changes, but I get the error "abort: cannot amend public changesets". I had used hg update some time ago. Could you please tell me the best way to update this commit? I am using hg and arc.
Thanks

Comment 13

[Jan Henning [:JanH]]

Did you use ./mach vcs-setup to configure your Mercurial installation? Please do so if you haven't already and then paste the output of hg wip.

If you say you've used hg update at some point, you've probably changed away from the original commit you've uploaded to Phabricator, so you need to change back to it, make sure the files you want to change are correct and then you can use hg commit --amend to update your own commit and subsequently upload it to Phabricator again.

Comment 14

[sonali18317]

Hi Jan,
Umm nope, I hadn't used ./mach vcs-setup, thanks for helping me.
https://pastebin.com/e8DnJRDg is the output of hg wip after running vcs-setup.

Comment 15

[Jan Henning [:JanH]]

Ah, hmm. If I remember correctly, hg wip as installed by ./mach vcs-setup possibly hides changesets of your that haven't been touched in a few weeks, so that might explain why it doesn't appear in that paste.

In any case the "@" shows that your currently checked out commit is 464707:1735fe854369 nerli Merge inbound to mozilla-central. a=merge, which you obviously cannot modify, as it's already a public commit.

So what you need to do is find your old original commit, update to that, apply your changes, optionally rebase the commit to the current state of mozilla-central and the push it to Phabricator again.

Can you try hg log -r "author(sonali)" instead? Hopefully your original commit will appear there then. You can the use hg up [changeset ID] to change your working directory to that commit, and the update your patch using hg commit --amend. Finally you could also rebase it to the current state of mozilla-central by using hg rebase -s [changeset ID - you can also use a dot to reference the currently checked out commit if it's the one you want] -d central.

Comment 16

[sonali18317]

Hi Jan, I think I accidentally deleted my commit because I couldn't find it anywhere. hg log -r "author(sonali)" showed only the older commit which got merged. I could see all my changes in hg diff. Also, I checked the history and found I had used hg strip twice, so it's likely that I deleted it. :( Now that I have been reading about hg for quite a few hours, I found that it is not recommended to use hg strip.

Thanks for helping me out. I learned a lot :)
I made a new commit with the updated changes. Please review those, really sorry for the inconvenience.

Comment 17

[sonali18317]

Attachment: Bug 1495344 Sepia option for Reader View (Android) shows correctly

Comment 18

[Jan Henning [:JanH]]

Even if you had to create a completely fresh commit, I think it should have been possible to link this to the old Phabricator Review via the corresponding ID (D26417), though if you're using arc I can't help you directly there as I'm using something else. But it's not a big problem this time.

Edit: Also, if you already know who should be looking at your patch (in this case me), please also add them as reviewers in Phabricator, so your patch will be less likely to be overlooked.

Comment 19

[sonali18317]

I'll keep these things in mind. Thanks :)
What do you use if not arc? moz-phab?

Comment 20

[Jan Henning [:JanH]]

Neither - personally, I'm using the phabsend hg extension (respectively the unofficial fork that's currently maintained by :kwan), because
a) there was some delay between Phabricator supplanting Mozreview and moz-phab being available, and handling patch series with pure arc is rather painful from what I've heard, and
b) for the time being (bug 1471687), moz-phab still requires arc, which again is somewhat cumbersome to install, especially on Windows.

But if you already have arc set up, giving moz-phab a try might not be a bad idea, especially if you eventually want to start handling patch series with several commits, as moz-phab allows you to simple upload those to Phabricator in one go and also link them together automatically.

In any case, once your patch has been approved and there are no outstanding issues remaining, you can set the checking-needed keyword on the bug to get it checked in.
In theory for any non-trivial patch it's not a bad idea to run our automated tests on them before checking the changes in by submitting them to Try, but given that this isn't currently possible because of bug 1548973 and as your changes are straightforward enough, I think we can skip it this time.

Comment 21

[sonali18317]

Sure then I'll try moz-phab as well.
Will check how to run automated tests, it would be useful once that bug is fixed.

Thanks, I learned a lot :)
Let me know if I can help anywhere else.

Comment 22

[Jan Henning [:JanH]]

A little bit of reading about Try is available here: https://firefox-source-docs.mozilla.org/tools/try/index.html

With bug 1548973 over, I've pushed your changes to Try and there don't seem any unexpected errors in the relevant tests for Firefox on Android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c29383492edb22eac0ae6cc7f9beab8df86f13cd
(The one failure in the one orange tests isn't related to your changes).

So everything looks fine and like I said, to get your changes checked-in, you need to edit this bug (I've added you as assignee, so you should be able to edit it now) and add the "checkin-needed" keyword.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17b693747dbd
Added color definitions for sepia mode in mobile aboutReader.css r=JanH

Comment 24

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/17b693747dbd

Comment 26

[Stefan Deiac]

Verified as fixed on the latest beta 68.0b11 using Nexus 9 (Android 7.1.1) and OnePlus 5T (Android 9).