Closed
Bug 1158281
Opened 9 years ago
Closed 9 years ago
Match Pocket's Reader View Sepia Theme
Categories
(Firefox :: General, defect)
Tracking
()
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)
2.34 KB,
patch
|
bwinton
:
review+
bwinton
:
ui-review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
FROM POCKET: The values for sepia: Background: #F4ECD8 Text color: #5b4636
Assignee | ||
Comment 1•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8598170 -
Flags: ui-review?(mmaslaney)
Attachment #8598170 -
Flags: review?(gijskruitbosch+bugs)
Updated•9 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 40.2 - 27 Apr
Flags: qe-verify?
Flags: firefox-backlog+
Assignee | ||
Updated•9 years ago
|
Points: --- → 1
Comment 4•9 years ago
|
||
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•9 years ago
|
||
(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)
Assignee | ||
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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)
Comment 8•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #8598170 -
Flags: review?(margaret.leibovic) → review+
Updated•9 years ago
|
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
Assignee | ||
Updated•9 years ago
|
Target Milestone: Firefox 38 → Firefox 40
Reporter | ||
Updated•9 years ago
|
Attachment #8598170 -
Flags: ui-review?(mmaslaney) → ui-review+
Assignee | ||
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
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-
Assignee | ||
Comment 11•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 12•9 years ago
|
||
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?
Updated•9 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
Updated•9 years ago
|
status-firefox38:
--- → unaffected
status-firefox38.0.5:
--- → affected
status-firefox39:
--- → affected
status-firefox40:
--- → affected
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e7f4e5602c67
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e7f4e5602c67
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Attachment #8598902 -
Flags: approval-mozilla-beta?
Attachment #8598902 -
Flags: approval-mozilla-beta+
Attachment #8598902 -
Flags: approval-mozilla-aurora?
Attachment #8598902 -
Flags: approval-mozilla-aurora+
Comment 15•9 years ago
|
||
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
Updated•9 years ago
|
Comment 18•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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.
Description
•