1222414 - Convert hardcoded colors in add-on manager to CSS variables

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Tim Nguyen :ntim]

Many colors here: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/extensions/extensions.inc.css could use CSS variables from https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css instead of some hardcoded colors.

Comment 1

[Dillon Brock]

Attachment: New extensions.inc.css with color variables

Here is a new CSS file with most hardcoded colors replaced with color variables from common.inc.css.

Comment 2

[Waqar Arshad]

Hi Tim,

My name is Waqar and I am currently studying computer science degree. I would like to take this as my first bug as part of my opensource coursework and i was wondering if you could point me towards the right directions as in how to go about fixing this bug?

Thank You

Comment 3

[Tim Nguyen :ntim]

(In reply to Waqar Arshad from comment #2)
> Hi Tim,
> 
> My name is Waqar and I am currently studying computer science degree. I
> would like to take this as my first bug as part of my opensource coursework
> and i was wondering if you could point me towards the right directions as in
> how to go about fixing this bug?
> 
> Thank You
Hi Waqar, thank you for your interest in this bug, the first comment in this bug should a good description of what to do, further more, it seems that Dillon has already done most the work, except it needs to be under the form of a patch.

To generate a patch, you'll need to set up your development environment first. http://codefirefox.com is a good site with video tutorials.

Feel free to use the needinfo? flag if you need help.

Comment 4

[Waqar Arshad]

(In reply to Tim Nguyen [:ntim] from comment #3)
> (In reply to Waqar Arshad from comment #2)
> > Hi Tim,
> > 
> > My name is Waqar and I am currently studying computer science degree. I
> > would like to take this as my first bug as part of my opensource coursework
> > and i was wondering if you could point me towards the right directions as in
> > how to go about fixing this bug?
> > 
> > Thank You
> Hi Waqar, thank you for your interest in this bug, the first comment in this
> bug should a good description of what to do, further more, it seems that
> Dillon has already done most the work, except it needs to be under the form
> of a patch.
> 
> To generate a patch, you'll need to set up your development environment
> first. http://codefirefox.com is a good site with video tutorials.
> 
> Feel free to use the needinfo? flag if you need help.

Hi Tim,

Could you please explain to me how i can run and check what effect the css file has and whether the changes that have been made are correct?

Comment 5

[Tim Nguyen :ntim]

(In reply to Waqar Arshad from comment #4)
> Could you please explain to me how i can run and check what effect the css
> file has and whether the changes that have been made are correct?
You should verify that about:addons doesn't change visually.

Comment 6

[Amal Oueslati]

Attachment: The extensions css file now uses color variables in the linked css file

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8690245 [details] [diff] [review]
The extensions css file now uses color variables in the linked css file

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

You should choose the variable appropriate to the context.

::: toolkit/themes/shared/extensions/extensions.inc.css
@@ +156,5 @@
>    padding: 2px 8px;
>    margin: 6px 0;
>    -moz-margin-start: 6px;
>    border-radius: 100%;
> +  color: var(--in-content-selected-text);

This should stay as #fff

@@ +220,5 @@
>  
>  .view-header {
>    margin: 0;
>    -moz-margin-end: 48px;
> +  border-bottom: 1px solid var(--in-content-category-text);

This is a border, so it should probably use the box-border-color variable

@@ +227,5 @@
>  #header-utils-btn {
>    height: 30px;
>    line-height: 20px;
> +  border-color: var(--in-content-category-text);
> +  background-color: var(--in-content-page-background);

This is a button, so it should use box-background

@@ +738,5 @@
>    font-weight: bold;
>  }
>  
>  #detail-contrib-btn {
> +  color: var(--in-content-selected-text);

This should stay as #fff

@@ +743,2 @@
>    text-shadow: none;
> +  border: 1px solid var(--in-content-link-color);

This should use 1px solid transparent

@@ +745,2 @@
>    list-style-image: url("chrome://mozapps/skin/extensions/heart.png");
> +  background-color: var(--in-content-link-color);

This should be using in-content-primary-button-background as it's a primary button

@@ +923,5 @@
>  }
>  
>  .download-progress .status {
> +  color: var(--in-content-text-color);
> +  text-shadow: var(--in-content-selected-text) 0 0 2px;

Please keep this as #fff

@@ +970,5 @@
>    -moz-appearance: none;
>    background: transparent;
>    border: none;
>    box-shadow: none;
> +  color: var(--in-content-primary-button-background);

This should be --in-content-link-color

Comment 8

[ousman bah]

Hi, I want to fix this bug. This is my first bug. Can you guide me please

Comment 9

[Akshay]

Attachment: bug1222414_convertcolours.diff

Comment 10

[Caius Sabau]

Hi, 

Can I have this bug assigned to me?

Thanks

Comment 11

[Seema Shad]

Attachment: bug1222414.patch

Hi Tim,

Find my patch attached.

Kind regards,
Seema Shad

Comment 12

[Caius Sabau]

Attachment: bug1222414.diff

Please find my attachment. I have followed your latest guidance. Forgot to publish it last night as I was waiting to get the bug assigned.

Thanks

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8696945 [details] [diff] [review]
bug1222414.patch

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

::: toolkit/themes/shared/extensions/extensions.inc.css
@@ +51,5 @@
>    }
>  
>    .global-warning .warning-icon {
> +    background-color: var(--in-content-selected-text);
> +    box-shadow: 0 0 2px 5px var(--in-content-selected-text);

Please keep these 2 property values as #fff.

@@ +227,5 @@
>  
>  #header-utils-btn {
>    height: 30px;
>    line-height: 20px;
> +  border-color: var(--in-content-category-text);

This should be var(--in-content-box-border-color)

@@ +744,2 @@
>    text-shadow: none;
> +  border: 1px solid transparent var(--in-content-link-color);

You should remove var(--in-content-link-color) since the border-color is already set to transparent.

@@ +752,5 @@
>  }
>  
>  #detail-contrib-btn:not(:active):hover {
> +  border-color: var(--in-content-primary-button-background-hover);
> +  background-color: var(--in-content-primary-button-background-hover);

You can omit the border-color here, since transparent already takes care of duplicating the border-color from the background.

@@ +757,5 @@
>  }
>  
>  #detail-contrib-btn:active:hover {
> +  background-color: var(--in-content-primary-button-background-active);
> +  border-color: var(--in-content-primary-button-background-active);

Same here

@@ +853,5 @@
>    margin: 2px 4px;
>  }
>  
>  .download-progress[mode="undetermined"] {
> +  border-color: var(--in-content-category-border-focus);

I would use var(--in-content-border-focus) in this context

Comment 14

[Tim Nguyen :ntim]

(In reply to Caius Sabau from comment #12)
> Created attachment 8696950 [details] [diff] [review]
> bug1222414.diff
> 
> Please find my attachment. I have followed your latest guidance. Forgot to
> publish it last night as I was waiting to get the bug assigned.
> 
> Thanks

See the comments I've made on Seema's patch.

Also, Seema is the first person to request review from me, so I'll assign the bug to him. The other patches slipped off my radar (please use the needinfo box next time), please email me if you'd like me to suggest other bugs to work on.

Comment 15

[Caius Sabau]

Attachment: bug1222414_v2.diff

Although Seema is assigned to this bug, I just wanted to know if this version of my fix is correct.

Thanks

Comment 16

[Tim Nguyen :ntim]

(In reply to Caius Sabau from comment #15)
> Created attachment 8697271 [details] [diff] [review]
> bug1222414_v2.diff
> 
> Although Seema is assigned to this bug, I just wanted to know if this
> version of my fix is correct.
> 
> Thanks

Can you merge that patch with your previous patch ?

Comment 17

[Caius Sabau]

Sorry, it makes sense. I should have just updated the patch. I don't know how to merge them now, unless I start all over again? 
Is there another way?

Comment 18

[Tim Nguyen :ntim]

(In reply to Caius Sabau from comment #17)
> Sorry, it makes sense. I should have just updated the patch. I don't know
> how to merge them now, unless I start all over again? 
> Is there another way?

Do `hg qpop` until only your first patch is applied.
Then do: 
hg qfold name-of-your-second-patch

Comment 19

[Caius Sabau]

Attachment: bug1222414.diff

Awesome, think I made it. Can you please have a look at this patch now?

Thank you!

Comment 20

[Tim Nguyen :ntim]

Seema, What's the status of this bug ?

Comment 21

[Caius Sabau]

Tim, 
I would like to know what do you think about my patch. Can you review it please?

Thanks

Comment 22

[Tim Nguyen :ntim]

Seems like Seema is no longer working on this bug.

Comment 23

[Tim Nguyen :ntim]

Comment on attachment 8697318 [details] [diff] [review]
bug1222414.diff

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

Almost there, thanks for the patch !
Also, can you change the commit message to :
Bug 1222414 - Convert hardcoded colors in add-on manager to CSS variables. r=dao,ntim

::: toolkit/themes/shared/extensions/extensions.inc.css
@@ +48,5 @@
>    }
>  
>    .global-warning .warning-icon {
>      background-color: #fff;
> +   box-shadow: 0 0 2px 5px #fff;

Please restore the indentation here.

@@ +858,5 @@
>    margin: 2px 4px;
>  }
>  
>  .download-progress[mode="undetermined"] {
> +  border-color:var(--in-content-border-focus);

Please add a space after ":"

@@ +866,5 @@
>  .download-progress[complete] .end-cap,
>  .download-progress[mode="undetermined"] .end-cap,
>  .download-progress .progress .progress-bar {
>    -moz-appearance: none;
> +  background-color: ar(--in-content-border-focus);

Should be var not ar

Comment 24

[Pooja Arora]

Hi, please let me know if I can work on this bug?

Comment 25

[Tim Nguyen :ntim]

(In reply to Pooja Arora from comment #24)
> Hi, please let me know if I can work on this bug?

Assigned it to you, you can look at the current patch and its review feedback for guidance.

Comment 26

[Pooja Arora]

(In reply to Tim Nguyen :ntim from comment #25)
> (In reply to Pooja Arora from comment #24)
> > Hi, please let me know if I can work on this bug?
> 
> Assigned it to you, you can look at the current patch and its review
> feedback for guidance.

Sure. Thank you!

Comment 27

[Rayann Tedds]

Hiya, I'm interested in developing this bug. Can I be assigned to it please? Thank you :)

Comment 28

[Matthew N. [:MattN]]

Hey Tim, it looks like most, if not all, of this was done in bug 1169679. Should we dupe it there?

Comment 29

[Tim Nguyen :ntim]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #28)
> Hey Tim, it looks like most, if not all, of this was done in bug 1169679.
> Should we dupe it there?

I don't think bug 1169679 did anything related to this bug.

However, bug 1388761 did take care parts of this bug.

There's still some left: https://dxr.mozilla.org/mozilla-central/rev/9fe69ff0762da191fbd2b9fc0718e96f1ab4137e/toolkit/themes/shared/extensions/extensions.inc.css#118,184,189,266,300,374,739,742,748,757,975,995-1010,1036

Comment 30

[Raymond]

Hello, I'd like to work on this as my good first bug!

Comment 31

[Raymond]

Attachment: Fix for bug 1222414

Patch for extensions.inc.css that adds color variables

Comment 32

[Tim Nguyen :ntim]

Comment on attachment 8957381 [details] [diff] [review]
Fix for bug 1222414

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

Looks mostly fine, thanks for looking into this.

I have a few comments, can you please address them so we can land this patch?

::: toolkit/themes/shared/extensions/extensions.inc.css
@@ +181,4 @@
>  }
>  
>  .category-badge {
> +  background-color: var(--in-content-page-color);

Please leave this as #55D4FF :) The .category-badge uses special colors unique the add-on manager.

@@ +185,5 @@
>    padding: 2px 8px;
>    margin: 6px 0;
>    margin-inline-start: 6px;
>    border-radius: 100%;
> +  color: var(--in-content-selected-text);

Please leave this as #FFF for the same reason.

@@ +736,4 @@
>  
>  #detail-contributions {
>    border-radius: 2px;
> +  border: 1px solid var(--in-content-box-border-colour);

I think you mean border-color not border-colour :)

@@ +1033,4 @@
>  
>  .addon[active="true"] .experiment-bullet,
>  #detail-view[active="true"] #detail-experiment-bullet {
> +  fill: var(--in-content-box-background-active);

Could you leave this as `fill: rgb(106, 201, 20)` ?

Comment 33

[Raymond]

Attachment: 1222414.patch

Fix for bug 1222414 with suggested changes

Comment 34

[Tim Nguyen :ntim]

Comment on attachment 8957568 [details] [diff] [review]
1222414.patch

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

Looks good, thanks!

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e620f50bbdcf
Convert hardcoded colors in add-on manager to CSS variables. r=jaws, ntim

Comment 36

[Cosmin Sabou [:CosminS]]

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

Comment 37

[Tim Nguyen :ntim]

Congrats Raymond! Your patch is now in the latest Nightly :)

Comment 38

[Raymond]

Awesome, thanks!