Closed Bug 1139026 Opened 9 years ago Closed 9 years ago

Use different text highlight color in dark theme

Categories

(Toolkit :: Reader Mode, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla40
Tracking Status
firefox38 --- verified
firefox39 --- fixed
firefox40 --- verified

People

(Reporter: Margaret, Assigned: divjot94, Mentored)

References

Details

(Whiteboard: [lang=css])

Attachments

(2 files, 7 obsolete files)

See this tweet:
https://twitter.com/divjot94/status/572792968274305024

It could be nice polish to use a different text highlight color for different themes. This is also something that we could look into on Android, although our default orange highlight doesn't look as bad in the dark theme.

mmaslaney, what do you think?
Flags: needinfo?(mmaslaney)
We could use the same orange from the Type Controls highlight: #FF7000.
Flags: needinfo?(mmaslaney)
Attached image Using #FF7000 (obsolete) —
Code : https://gist.github.com/bogas04/b9bcd03070dd7ac836c6

Applies the -moz-selection pseudo class for p, h1, h2, h3, h4, h5, h6, span, time under .dark class.

Please help me in integrating the code to the reader mode. By the looks of it, I guess I just need to inject the rules into aboutReader.css
Flags: needinfo?(margaret.leibovic)
(In reply to bogas04 from comment #2)
> Created attachment 8572094 [details]
> Using #FF7000
> 
> Code : https://gist.github.com/bogas04/b9bcd03070dd7ac836c6
> 
> Applies the -moz-selection pseudo class for p, h1, h2, h3, h4, h5, h6, span,
> time under .dark class.
> 
> Please help me in integrating the code to the reader mode. By the looks of
> it, I guess I just need to inject the rules into aboutReader.css

Thanks for jumping in!

First of all, do you have a Firefox build environment set up? If not, you'll want to follow the instructions here:
https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

Then you'll want to edit the desktop CSS file, which is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/aboutReader.css

This MDN page has more details about generating a patch for review:
https://developer.mozilla.org/en-US/docs/Introduction

And as for the approach, I think you can make this apply to all text this page, so I think the .dark::-moz-selection selector would do the job.
Assignee: nobody → divjot94
Mentor: margaret.leibovic
Flags: needinfo?(margaret.leibovic)
Whiteboard: [lang=css]
Attached patch Initial Patch (obsolete) — Splinter Review
Apologies for the delay, after finally downloading the bundle & getting it built for the first time, I've written my first patch ever \o/

For some reason, .dark::-moz-selection doesn't work, 
so I went with .dark *::-moz-selection. 

After doing that I noticed how anchor tags with dark background & blue color didn't benefit from the orange highlight, hence I changed their background to white as of now. I'm not really good with colorschemes so it would be great if you could help me decide the right background-color for hyperlinks. 

Please review my patch.
Flags: needinfo?(margaret.leibovic)
P3 - bit of an a11y issue, and people also commonly hilight text on a page for various reasons.
Priority: -- → P3
Attached image Initial Patch Applied (obsolete) —
Is this okay to ship ?
Attachment #8572094 - Attachment is obsolete: true
Comment on attachment 8573895 [details] [diff] [review]
Initial Patch

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

Keeping white as the background-color for both selections, we should invert the text colors while keeping the same palette.

Non-link text would change to #0095dd and link text would change to #dd4800.

::: toolkit/themes/windows/global/aboutReader.css
@@ +214,5 @@
>  }
>  
> +/*
> + * Orange selection for dark mode 
> + * Bug : 1139026

We can remove this comment here, because this history will be stored within the revision control system.

@@ +217,5 @@
> + * Orange selection for dark mode 
> + * Bug : 1139026
> + */
> +.dark *::-moz-selection {
> +  background: #FF7000;

When we define the background-color we should also define the text color. Also, this should use `background-color` instead of the `background` shorthand.

@@ +219,5 @@
> + */
> +.dark *::-moz-selection {
> +  background: #FF7000;
> +}
> +.dark a::-moz-selection {

Please move this rule to be alongside the other rules that define the styling for links within .dark documents. It should go after the block at http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReader.css#173.
Attachment #8573895 - Flags: feedback+
Attached patch Inverted text colors (obsolete) — Splinter Review
Attachment #8573895 - Attachment is obsolete: true
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> @@ +219,5 @@
> > + */
> > +.dark *::-moz-selection {
> > +  background: #FF7000;
> > +}
> > +.dark a::-moz-selection {
> 
> Please move this rule to be alongside the other rules that define the
> styling for links within .dark documents. It should go after the block at
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/
> aboutReader.css#173.

These rules aren't present in Desktop version of reader mode, I've added the rules just after the adjustment made for blockquote in dark mode. Is there a better place than this?
Flags: needinfo?(jaws)
Attachment #8574400 - Attachment is obsolete: true
(In reply to bogas04 from comment #10)
> These rules aren't present in Desktop version of reader mode, I've added the
> rules just after the adjustment made for blockquote in dark mode. Is there a
> better place than this?

Oh, you're right. I was looking at the mobile CSS. Yes, the place you have them is fine.
Flags: needinfo?(jaws)
The most recent patch in this bug appears to only have a whitespace change. Do you have a more up-to-date patch?

Thanks, Jared, for stepping in to help review this.
Flags: needinfo?(margaret.leibovic)
(In reply to :Margaret Leibovic from comment #13)
> The most recent patch in this bug appears to only have a whitespace change.
> Do you have a more up-to-date patch?

Excuse the whitespace thingy, the patch also uses white background-color for all cases, and inverts colors for non links and link tags separately.

 
> .dark *::-moz-selection {
>   background-color: #FFFFFF;
>   color: #0095DD;
> }
> .dark a::-moz-selection {
>   color: #DD4800; 
> }
Still getting used to mercurial and bzexport, what I've done is that I created two different patches, which is messing up with the diff. My bad!

This was the initial patch : https://bug1139026.bugzilla.mozilla.org/attachment.cgi?id=8573895
this is the most recent patch : https://bug1139026.bugzilla.mozilla.org/attachment.cgi?id=8574565
So, can we mark it as resolved and push it to central? Or are there other changes required to be made before pushing it?
(In reply to bogas04 from comment #16)
> So, can we mark it as resolved and push it to central? Or are there other
> changes required to be made before pushing it?

Ah, sorry, I think there's some confusion in this bug. Before pushing anything to mozilla-central, we need a patch that applies that has an r+. Could you please upload a new version of your patch that combines all of your changes, and request review from me? Afterwards, when that gets an r+, we can flag this bug as checkin-needed. Thanks!
Attached patch Final Patch (obsolete) — Splinter Review
Attachment #8574565 - Attachment is obsolete: true
Attachment #8576289 - Flags: review?(margaret.leibovic)
Thanks for the updated patch! I just noticed that this doesn't actually match what mmaslaney (our UX designer) suggested in comment 1. Have you checked with him to see if he agrees with the colors you chose?

jaws, did you come up with these colors?
Flags: needinfo?(jaws)
(In reply to :Margaret Leibovic from comment #19)
> Thanks for the updated patch! I just noticed that this doesn't actually
> match what mmaslaney (our UX designer) suggested in comment 1. Have you
> checked with him to see if he agrees with the colors you chose?
> 
> jaws, did you come up with these colors?

Yeah, as recommended by jaws in his review, I went with following colors :

> Keeping white as the background-color for both selections, 
> we should invert the text colors while keeping the same palette.
> Non-link text would change to #0095dd and link text would change to #dd4800.
Oh, I didn't see comment #1. I got the colors from the Complimentary Colors section on http://www.color-hex.com/color/0095dd
Flags: needinfo?(jaws)
So main issue with #FF7000 as background is that it doesn't look good with links. 
Eg : http://i.imgur.com/gHU0NCe.png

To fix that, I went with white background for links. 
Eg : https://bug1139026.bugzilla.mozilla.org/attachment.cgi?id=8574400

And jaws recommended white bg with complementary colors.
Eg : https://bugzilla.mozilla.org/attachment.cgi?id=8574566

Can you tell me which one would be better & consistent to other platforms?
Flags: needinfo?(mmaslaney)
Let's move forward with Jaws' recommendation. I believe that version will be the most consistent with other platforms.
Flags: needinfo?(mmaslaney)
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #23)
> Let's move forward with Jaws' recommendation. I believe that version will be
> the most consistent with other platforms.

Cool!

:Margaret Leibovic could you please review the final patch. It follows Jaws' recommendation, as seen here : https://bug1139026.bugzilla.mozilla.org/attachment.cgi?id=8574566
Flags: needinfo?(margaret.leibovic)
Comment on attachment 8576289 [details] [diff] [review]
Final Patch

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

Thanks for your patience, this looks good!

::: toolkit/themes/windows/global/aboutReader.css
@@ +217,5 @@
> +  background-color: #FFFFFF;
> +  color: #0095DD;
> +}
> +.dark a::-moz-selection {
> +  color: #DD4800; 

Nit: Please remove the trailing whitespace here.
Attachment #8576289 - Flags: review?(margaret.leibovic) → review+
Flags: needinfo?(margaret.leibovic)
Awesome !
What are the next steps for me ?
(In reply to bogas04 from comment #26)
> Awesome !
> What are the next steps for me ?

Please attach a new patch that addresses my small review comment and has a properly formatted commit message. For more details on this, see:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions

Then we can add the checkin-needed keyword to this bug, and someone will check it in for you.

Usually we require a try push, but this is a small enough CSS change that I'm confident it won't break tests.
Attachment #8576289 - Attachment is obsolete: true
Keywords: checkin-needed
patching file toolkit/themes/windows/global/aboutReader.css
bad hunk #1 @@ -208,16 +208,24 @@ body {
 (17 16 24 24)
Keywords: checkin-needed
Is this an error ?
Comment on attachment 8583832 [details] [diff] [review]
Using white background-color & inverted color for selected area.

I'm surprised this patch is only for Windows. Aren't OSX and Linux affected as well ?
(In reply to bogas04 from comment #30)
> Is this an error ?

You need to pull the latest code and rebase the patch.
(In reply to Tim Nguyen [:ntim] from comment #31)
> Comment on attachment 8583832 [details] [diff] [review]
> Using white background-color & inverted color for selected area.
> 
> I'm surprised this patch is only for Windows. Aren't OSX and Linux affected
> as well ?

Yes it affects all platforms.

(In reply to Tim Nguyen [:ntim] from comment #32)
> (In reply to bogas04 from comment #30)
> > Is this an error ?
> 
> You need to pull the latest code and rebase the patch.

I'm having a tough time with mercurial, and it seems I've messed up a lot with the changes and patches etc. Is there a way to clean everything up and have a fresh copy of mozilla-central, without actually redownloading it ? Basically I want to discard all local changes (committed or uncommitted) and pull and merge with mozilla-central.
(In reply to bogas04 from comment #33)
> (In reply to Tim Nguyen [:ntim] from comment #31)
> > Comment on attachment 8583832 [details] [diff] [review]
> > Using white background-color & inverted color for selected area.
> > 
> > I'm surprised this patch is only for Windows. Aren't OSX and Linux affected
> > as well ?
> 
> Yes it affects all platforms.
> 
> (In reply to Tim Nguyen [:ntim] from comment #32)
> > (In reply to bogas04 from comment #30)
> > > Is this an error ?
> > 
> > You need to pull the latest code and rebase the patch.
> 
> I'm having a tough time with mercurial, and it seems I've messed up a lot
> with the changes and patches etc. Is there a way to clean everything up and
> have a fresh copy of mozilla-central, without actually redownloading it ?
> Basically I want to discard all local changes (committed or uncommitted) and
> pull and merge with mozilla-central.


To pop any mq-based patches:

hg qpop -a

To revert all your local changes:

hg revert --all


Then check:

hg out

and run:

hg strip


with the revisions that you want removed.

Once all that is done, just "hg pull -u" should pull and update your local copy to be current with whatever repository you downloaded from (mozilla-central, fx-team, etc.).
Attachment #8583832 - Attachment is obsolete: true
Attachment #8586834 - Attachment is obsolete: true
Comment on attachment 8586840 [details] [diff] [review]
Bug 1139026 - Using white background-color & inverted color for selected area. r=:Margaret Leibovic

Solved the whitespace issue. Please review it. It's embarrassing how something this small took so long ! But I believe subsequent patches won't take that long, now that I have gotten used to mq.
Flags: needinfo?(margaret.leibovic)
Attachment #8586840 - Flags: review?(margaret.leibovic)
Comment on attachment 8586840 [details] [diff] [review]
Bug 1139026 - Using white background-color & inverted color for selected area. r=:Margaret Leibovic

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

Can you please also apply the same changes to toolkit/themes/(linux|osx)/global/aboutReader.css ?
(In reply to Tim Nguyen [:ntim] from comment #38)
> Comment on attachment 8586840 [details] [diff] [review]
> Bug 1139026 - Using white background-color & inverted color for selected
> area. r=:Margaret Leibovic
> 
> Review of attachment 8586840 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you please also apply the same changes to
> toolkit/themes/(linux|osx)/global/aboutReader.css ?

http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/aboutReader.css
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/aboutReader.css

They lack aboutReader.css in global directory. It seems only windows folder has the file which is used by reader mode.
PS : I've tried the patch on my OSX system, and it works well.
(In reply to bogas04 from comment #39)
> (In reply to Tim Nguyen [:ntim] from comment #38)
> > Comment on attachment 8586840 [details] [diff] [review]
> > Bug 1139026 - Using white background-color & inverted color for selected
> > area. r=:Margaret Leibovic
> > 
> > Review of attachment 8586840 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Can you please also apply the same changes to
> > toolkit/themes/(linux|osx)/global/aboutReader.css ?
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> aboutReader.css
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/
> aboutReader.css
> 
> They lack aboutReader.css in global directory. It seems only windows folder
> has the file which is used by reader mode.
> PS : I've tried the patch on my OSX system, and it works well.

Ah, our code is pretty confusing, shared code is usually put in shared (not windows). Ignore what I said :)
Yes, there is only a file in the windows directory. In toolkit, linux inherits from windows, and for osx, we pull in the windows style as well:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/jar.mn#13

If we don't already have a bug on file, we should file one to move aboutReader.css to the shared directory.
Flags: needinfo?(margaret.leibovic)
Comment on attachment 8586840 [details] [diff] [review]
Bug 1139026 - Using white background-color & inverted color for selected area. r=:Margaret Leibovic

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

This looks good, I can land it for you. In the future, just remember to update the commit message in the patch to include the bug number, a description of what you changed, and the reviewer. It looks like you did this in the patch attachment name, but not in the actual patch :)
Attachment #8586840 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/a4500e6defba
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8586840 [details] [diff] [review]
Bug 1139026 - Using white background-color & inverted color for selected area. r=:Margaret Leibovic

Approval Request Comment
[Feature/regressing bug #]: reader view
[User impact if declined]: hard to see selected text in dark reader view theme
[Describe test coverage new/current, TreeHerder]: landed on nightly
[Risks and why]: low-risk small CSS change
[String/UUID change made/needed]: none
Attachment #8586840 - Flags: approval-mozilla-beta?
Attachment #8586840 - Flags: approval-mozilla-aurora?
Comment on attachment 8586840 [details] [diff] [review]
Bug 1139026 - Using white background-color & inverted color for selected area. r=:Margaret Leibovic

Should be in 38 beta 2
Attachment #8586840 - Flags: approval-mozilla-beta?
Attachment #8586840 - Flags: approval-mozilla-beta+
Attachment #8586840 - Flags: approval-mozilla-aurora?
Attachment #8586840 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
QA Contact: andrei.vaida
Verified fixed on Nightly 40.0a1 (2015-04-05), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
Verified fixed on Beta 38.0b2-build1 (20150406174117) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.
This was already verified on Firefox 38 and 40, so I don't think additional verification on Firefox 39 is needed.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: