Sepia option for Reader View is not showing correctly

VERIFIED FIXED in Firefox 68

Status

()

defect
P5
normal
VERIFIED FIXED
10 months ago
25 days ago

People

(Reporter: yekit, Assigned: sonali18317)

Tracking

Firefox 62
Firefox 68
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 wontfix, firefox67 wontfix, firefox68 verified)

Details

(Whiteboard: [good first bug])

Attachments

(3 attachments)

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
The middle option used to be an automatic mode, so maybe the change to "Sepia" wasn't properly implemented?
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → Android
Hardware: Unspecified → All
Likely needs the styles from desktop to be correctly referenced.
Blocks: 1472957

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.

Will be fixed?

Priority: -- → P5
Whiteboard: [good first bug]

Hi,

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

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?

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

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

Flags: needinfo?(jh+bugzilla)

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.

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.

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

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.

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.

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.

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.

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.

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

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.

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.

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.

Assignee: nobody → sonali18317
Keywords: checkin-needed

Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17b693747dbd
Added color definitions for sepia mode in mobile aboutReader.css r=JanH

Keywords: checkin-needed
Flags: needinfo?(jh+bugzilla)
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Duplicate of this bug: 1560107

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

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.