Closed Bug 1158281 Opened 5 years ago Closed 5 years ago

Match Pocket's Reader View Sepia Theme

Categories

(Firefox :: General, defect)

39 Branch
defect
Not set
Points:
1

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.3 - 11 May
Tracking Status
firefox38 --- unaffected
firefox38.0.5 --- verified
firefox39 --- fixed
firefox40 --- verified

People

(Reporter: mmaslaney, Assigned: bwinton)

References

Details

Attachments

(1 file, 2 obsolete files)

FROM POCKET:

The values for sepia:
Background: #F4ECD8
Text color: #5b4636
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?
Flags: needinfo?(alam)
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!
Flags: needinfo?(alam)
Attached patch The first cut at the patch. (obsolete) — Splinter Review
Attachment #8598170 - Flags: ui-review?(mmaslaney)
Attachment #8598170 - Flags: review?(gijskruitbosch+bugs)
Status: NEW → ASSIGNED
Iteration: --- → 40.2 - 27 Apr
Flags: qe-verify?
Flags: firefox-backlog+
Points: --- → 1
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 ?
(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 ?
Flags: needinfo?(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.  :)
Flags: needinfo?(bwinton)
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. :-)
Attachment #8598170 - Flags: review?(gijskruitbosch+bugs) → review?(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.
Attachment #8598170 - Flags: review?(margaret.leibovic) → review+
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
Target Milestone: Firefox 38 → Firefox 40
Attachment #8598170 - Flags: ui-review?(mmaslaney) → ui-review+
Attached patch The second version. (obsolete) — Splinter Review
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.  ;)
Attachment #8598170 - Attachment is obsolete: true
Attachment #8598827 - Flags: ui-review+
Attachment #8598827 - Flags: review?(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.
Attachment #8598827 - Flags: review?(margaret.leibovic) → review-
Attached patch bug1158281.diffSplinter Review
Reverting back to the previous patch (with updated commit message for ease of checkin), and carrying forward the r+ and ui-r+ from it.
Attachment #8598827 - Attachment is obsolete: true
Attachment #8598902 - Flags: ui-review+
Attachment #8598902 - Flags: review+
Keywords: checkin-needed
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.
Attachment #8598902 - Flags: approval-mozilla-beta?
Attachment #8598902 - Flags: approval-mozilla-aurora?
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
Attachment #8598902 - Flags: approval-mozilla-beta?
Attachment #8598902 - Flags: approval-mozilla-beta+
Attachment #8598902 - Flags: approval-mozilla-aurora?
Attachment #8598902 - Flags: approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
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.
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.
Removing qe-verify flag as verification Firefox 38.0.5 and 40 should suffice.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.