1158281 - Match Pocket's Reader View Sepia Theme

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Michael Maslaney [:mmaslaney] ([e-mail])]

FROM POCKET:

The values for sepia:
Background: #F4ECD8
Text color: #5b4636

Comment 1

[Blake Winton (:bwinton) (:☕️)]

Here's how it looks: https://www.dropbox.com/s/6xst2wu10e14jo4/Screenshot%202015-04-24%2014.17.07.png?dl=0

Anthony, did we want the new colours on Android, too, for consistency?

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Currently, we have three settings right now on Mobile - Dark, Auto, and Light. 

If we were to add a fourth, it seems too cramped and I think this will need more thought/co-ordination on the mobile side. Will co-ordinate with Darrin on our end and we'll work on it in another bug so we don't block you guys here. :)

Thanks for keeping us in the loop!

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Attachment: The first cut at the patch.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8598170 [details] [diff] [review]
The first cut at the patch.

Review of attachment 8598170 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/themes/shared/aboutReader.css
@@ +99,2 @@
>  body.sepia > .container > .header > .domain {
> +  border-bottom-color: #5b4636 !important;

Why do all these styles need !important ?

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #4)
> Comment on attachment 8598170 [details] [diff] [review]
> The first cut at the patch.
> 
> Review of attachment 8598170 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/themes/shared/aboutReader.css
> @@ +99,2 @@
> >  body.sepia > .container > .header > .domain {
> > +  border-bottom-color: #5b4636 !important;
> 
> Why do all these styles need !important ?

Comment 6

[Blake Winton (:bwinton) (:☕️)]

I bet they don't, but I just copied that from the previous code (which I might have gotten from fx-team?).  I'll test it without tomorrow, unless someone tells me why they're necessary.  :)

Comment 7

[:Gijs (he/him)]

Comment on attachment 8598170 [details] [diff] [review]
The first cut at the patch.

(In reply to Blake Winton (:bwinton) from comment #6)
> I bet they don't, but I just copied that from the previous code (which I
> might have gotten from fx-team?).  I'll test it without tomorrow, unless
> someone tells me why they're necessary.  :)

Yeah, I don't know... maybe margaret does. :-)

Comment 8

[:Margaret Leibovic]

(In reply to :Gijs Kruitbosch from comment #7)
> Comment on attachment 8598170 [details] [diff] [review]
> The first cut at the patch.
> 
> (In reply to Blake Winton (:bwinton) from comment #6)
> > I bet they don't, but I just copied that from the previous code (which I
> > might have gotten from fx-team?).  I'll test it without tomorrow, unless
> > someone tells me why they're necessary.  :)
> 
> Yeah, I don't know... maybe margaret does. :-)

That's new from bug 1154028. I found that I needed these to avoid these styles from being overridden by the default styles in the other CSS files.

But maybe they're not needed for *all* of these rules? I would definitely double check that things work properly with all color schemes before removing these.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Attachment: The second version.

So, I've removed the !importants, and took the opportunity to lowercase all the colours to match the rest of both the files, thus I'm going to re-request review.  ;)

Comment 10

[:Margaret Leibovic]

Comment on attachment 8598827 [details] [diff] [review]
The second version.

Review of attachment 8598827 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/themes/shared/aboutReader.css
@@ +92,5 @@
>  
>  /* Override some controls and content styles based on color scheme */
>  
> +body.light > .container > .header > .domain {
> +  border-bottom-color: #333333;

Removing these !important declaration breaks the color of the domain element at the top of reader view, so I think you should add these back.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Attachment: bug1158281.diff

Reverting back to the previous patch (with updated commit message for ease of checkin), and carrying forward the r+ and ui-r+ from it.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8598902 [details] [diff] [review]
bug1158281.diff

Approval Request Comment
[Feature/regressing bug #]: desktop reader mode
[User impact if declined]: slightly different reading mode colours for pocket sepia users.
[Describe test coverage new/current, TreeHerder]: Manual
[Risks and why]: Low, due to css colour-only change.
[String/UUID change made/needed]: None.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e7f4e5602c67

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e7f4e5602c67

Comment 15

[Andrei Vaida [:avaida]]

Confirmed fixed as of Nightly 40.0a1 (2015-04-03), using Windows 7 (x64), Mac OS X 10.9.5 and Ubuntu 14.04 (x86). The background and text color match the requirements posted in Comment 0.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/44698a75f0a6

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/810e81a9bced

Comment 18

[aleth [:aleth]]

I guess it's a matter of taste, but I don't think this change was an improvement :-/ 

Apart from looking less crisp, the new colour doesn't fit as well with the reader mode sidebar and URL toolbar bordering it.

Comment 19

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using Firefox 38.0.5 Beta 1 (buildID: 20150511143336): the background and text color match the requirements posted in Comment 0.

Comment 20

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Removing qe-verify flag as verification Firefox 38.0.5 and 40 should suffice.