Use different text highlight color in dark theme

VERIFIED FIXED in Firefox 38

Status

()

defect
P3
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Margaret, Assigned: divjot94, Mentored)

Tracking

Trunk
mozilla40
Points:
---

Firefox Tracking Flags

(firefox38 verified, firefox39 fixed, firefox40 verified)

Details

(Whiteboard: [lang=css])

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

4 years ago
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)
(Assignee)

Comment 2

4 years ago
Posted 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)
(Reporter)

Comment 3

4 years ago
(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]
(Assignee)

Comment 4

4 years ago
Posted patch Initial Patch (obsolete) — Splinter Review
(Assignee)

Comment 5

4 years ago
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
(Assignee)

Comment 7

4 years ago
Posted 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+
(Assignee)

Comment 9

4 years ago
Posted patch Inverted text colors (obsolete) — Splinter Review
(Assignee)

Updated

4 years ago
Attachment #8573895 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
(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)
(Assignee)

Comment 11

4 years ago
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)
(Reporter)

Comment 13

4 years ago
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)
(Assignee)

Comment 14

4 years ago
(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; 
> }
(Assignee)

Comment 15

4 years ago
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
(Assignee)

Comment 16

4 years ago
So, can we mark it as resolved and push it to central? Or are there other changes required to be made before pushing it?
(Reporter)

Comment 17

4 years ago
(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!
(Assignee)

Comment 18

4 years ago
Posted patch Final Patch (obsolete) — Splinter Review
(Assignee)

Updated

4 years ago
Attachment #8574565 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8576289 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 19

4 years ago
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)
(Assignee)

Comment 20

4 years ago
(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)
(Assignee)

Comment 22

4 years ago
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)
(Assignee)

Comment 24

4 years ago
(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)
(Reporter)

Comment 25

4 years ago
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+
(Reporter)

Updated

4 years ago
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 26

4 years ago
Awesome !
What are the next steps for me ?
(Reporter)

Comment 27

4 years ago
(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.
(Assignee)

Comment 28

4 years ago
Attachment #8576289 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
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
(Assignee)

Comment 30

4 years ago
Is this an error ?

Comment 31

4 years ago
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 ?

Comment 32

4 years ago
(In reply to bogas04 from comment #30)
> Is this an error ?

You need to pull the latest code and rebase the patch.
(Assignee)

Comment 33

4 years ago
(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.

Comment 34

4 years ago
(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.).
(Assignee)

Updated

4 years ago
Attachment #8583832 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8586834 - Attachment is obsolete: true
(Assignee)

Comment 37

4 years ago
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 38

4 years ago
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 ?
(Assignee)

Comment 39

4 years ago
(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.

Comment 40

4 years ago
(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 :)
(Reporter)

Comment 41

4 years ago
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)
(Reporter)

Comment 42

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Reporter)

Comment 45

4 years ago
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.