1443217 - Merge duplicated CSS variables from common.inc.css

Status: NEW

(Toolkit :: Themes, enhancement, P3)

Description

[Jared Wein [:jaws] (please needinfo? me)]

/toolkit/themes/shared/in-content/common.inc.css defines some variables that have similar names and are defined with the same value. These variables are never used with different values, so they can be merged.

--in-content-page-color
--in-content-text-color
│
└─ Replace *-text-color with *-page-color to match *-page-background

--in-content-border-color
--in-content-box-border-color
│
└─ Replace *-box-border-color with *-border-color to match *-border-focus/highlight

Comment 1

[Apratim]

This is the merged file: https://pastebin.com/MfyqH5rW

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Please submit your patch through MozReview and request review from me by putting "r?jaws" at the end of the commit message. You can follow the steps here to learn how to push to MozReview: https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#submitting-commits-for-review

Comment 3

[Apratim]

Attachment: Bug 1443217 - Merge duplicated CSS variables from common.inc.css

Review commit: https://reviewboard.mozilla.org/r/236554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236554/

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8967853 [details]
Bug 1443217 - Merge duplicated CSS variables from common.inc.css

https://reviewboard.mozilla.org/r/236554/#review242810

::: browser/config/mozconfig:10
(Diff revision 1)
>  # This file specifies the build flags for Firefox.  You can use it by adding:
>  #  . $topsrcdir/browser/config/mozconfig
>  # to the top of your mozconfig file.
>  
>  ac_add_options --enable-application=browser
> + # Automatically download and use compiled C++ components:

Please revert this change.

::: toolkit/themes/shared/in-content/common.inc.css:10
(Diff revision 1)
>  %endif
>  @namespace html "http://www.w3.org/1999/xhtml";
>  @namespace xul "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>  
>  *|*:root {
> -  --in-content-page-color: #0c0c0d;
> +  --in-content-page-text-category-text-active-color: #0c0c0d; 

nit, please remove any trailing white space at the end of these lines.

::: toolkit/themes/shared/in-content/common.inc.css:18
(Diff revision 1)
>    --in-content-box-background-odd: #f3f6fa;
>    --in-content-box-background-hover: #ebebeb;
>    --in-content-box-background-active: #dadada;
> -  --in-content-box-border-color: #d7d7db;
> +  --in-content-box-border-color: #d7d7db;    
>    --in-content-item-hover: rgba(0,149,221,0.25);
> -  --in-content-item-selected: #0a84ff;
> +  --in-content-item-selected-border-highlight-focus-text-selected-header-button-background: #0a84ff;

This doesn't seem right. Did you try doing a find-and-replace multiple times without checking the output?

Comment 5

[Apratim]

Comment on attachment 8967853 [details]
Bug 1443217 - Merge duplicated CSS variables from common.inc.css

https://reviewboard.mozilla.org/r/236554/#review242810

> This doesn't seem right. Did you try doing a find-and-replace multiple times without checking the output?

Yes, I tried find and replace tool multiple times, couldn't check the output.I actually didn't know how to check the output, any specified tool out there?
Thanks

Comment 6

[Apratim]

Attachment: Bug 1443217 - Merge duplicated CSS variables from common.inc.css.

Review commit: https://reviewboard.mozilla.org/r/251668/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251668/

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8986295 [details]
Bug 1443217 - Merge duplicated CSS variables from common.inc.css.

https://reviewboard.mozilla.org/r/251668/#review258062

It looks like this commit is based on top of your previous commit. Can you fold them together? You can run `hg histedit` then choose the `fold` option on the newest commit to fold it in to the previous commit. Then you can repush your patch for review.

Comment 8

[Apratim]

Attachment: Bug 1443217 - Merge duplicated CSS variables from common.inc.css

Review commit: https://reviewboard.mozilla.org/r/251694/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251694/

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8986352 [details]
Bug 1443217 - Merge duplicated CSS variables from common.inc.css

https://reviewboard.mozilla.org/r/251694/#review258218

Please fold all your changesets together.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Apratim from comment #5)
> Comment on attachment 8967853 [details]
> Bug 1443217 - Merge duplicated CSS variables from common.inc.css
> 
> https://reviewboard.mozilla.org/r/236554/#review242810
> 
> > This doesn't seem right. Did you try doing a find-and-replace multiple times without checking the output?
> 
> Yes, I tried find and replace tool multiple times, couldn't check the
> output.I actually didn't know how to check the output, any specified tool
> out there?
> Thanks

Well, in this case you could just look at the file and see the output of the find-and-replace. You can also use `hg diff` to look at the changes and verify that they make sense. You can also build Firefox, and look at the various parts of the user interface that use these variables and make sure that their appearance hasn't changed from a build that doesn't have your changes.

Comment 11

[Apratim]

Comment on attachment 8986352 [details]
Bug 1443217 - Merge duplicated CSS variables from common.inc.css

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251694/diff/1-2/

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8986352 [details]
Bug 1443217 - Merge duplicated CSS variables from common.inc.css

https://reviewboard.mozilla.org/r/251694/#review258360

::: toolkit/themes/shared/in-content/common.inc.css:18
(Diff revision 2)
>    --in-content-box-background-odd: #f3f6fa;
>    --in-content-box-background-hover: #ebebeb;
>    --in-content-box-background-active: #dadada;
>    --in-content-box-border-color: #d7d7db;
>    --in-content-item-hover: rgba(0,149,221,0.25);
> -  --in-content-item-selected: #0a84ff;
> +  --in-content-item-selected-border-highlight-focus-text-selected-header-button-background: #0a84ff;

This variable name doesn't make any sense. You've got your commits folded together now, but the issues I commented on in my first review haven't been addressed.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

This webpage has guides to help you build Firefox,
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build#Building_Firefox_for_the_Desktop

I would recommend using an Artifact build as it will build much faster since you don't need to make any C++ changes,
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds