Closed Bug 864562 Opened 11 years ago Closed 7 years ago

Use CSS Variables for lightweight theme styling

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: MattN, Assigned: jaws, Mentored)

References

()

Details

(Whiteboard: [lang=js][lang=css])

Attachments

(6 files, 3 obsolete files)

59 bytes, text/x-review-board-request
MattN
: review+
Details
59 bytes, text/x-review-board-request
MattN
: review+
Details
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 813786 added code to manipulate a LWT-specific stylesheet on lightweight-theme-styling-update and could be done in a cleaner fashion with CSS variables.  The CSS variable would need to be changed instead of manipulating CSS properties on a specific stylesheet.
Depends on: 813786
No longer depends on: 851009
For someone who has used CSS variables or CSSOM this shouldn't be hard to do.
Mentor: MattN+bmo
Whiteboard: [lang=js][lang=css]
Hi there 

could you assign me to this bug?
Hi Rishi, we typically assign bugs to people once they have uploaded a patch that shows they have made progress towards fixing the bug. Please comment here or ask on irc.mozilla.org if you have questions about getting started.
Hi Matthew,

I am ready to work on this bug, any further suggestions?

Cheers,
Prasad

CC: wajekarprasad@gmail.com
Hello and thanks for your interest!

The idea of this bug is to replace the dynamically modified CSS rules with static rules referencing CSS variables and then just update the variable value via CSSOM.

[1] is what modifies the CSS rules of chrome://browser/skin/browser-lightweightTheme.css[2] which has a variation for each platform. You will see comments in the CSS file like: `/*, lwtHeader;*/` which is showing that we will dynamically append the lightweight theme header image at that position. That should be replace with a variable like `var(--lwt-header-image)`. See other examples of CSS variables used with images at [3]. CSS variable docs are at [4]. The code at [1] would be changed to do something like `document.documentElement.style.setProperty("--lwt-header-image", …)` with no enumeration of the CSS rules in the stylesheet.

If this approach works out (which I'm not 100% sure of since I haven't used CSS variables very much) then we would eventually move the stylesheet rules back into the regular theme stylesheets but we can wait to do that until after.

Let me know if you have any questions. Please use the "need more information" from "mentor" option below the comment box.

[1] https://dxr.mozilla.org/mozilla-central/rev/a69583d2dbc6fdc18f63761a89cf539c356668be/browser/base/content/browser-addons.js#721-754
[2] https://dxr.mozilla.org/mozilla-central/search?q=path%3Abrowser-lightweightTheme.css+-path%3Aobj&redirect=false
[3] https://dxr.mozilla.org/mozilla-central/search?q=%22var(%22+background-image&redirect=false
[4] https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_variables
Flags: needinfo?(wajekarprasad)
Hi Matthew,
           I would like to work on this bug,Can you please assign it to me ?? I've done all the changes as you suggested.I attached the patch file of my changes.Please review it and suggest me if anything is wrong..Thank you..!!!


Best Regards,
    Raj
Comment on attachment 8825391 [details] [diff] [review]
please Review it..!!Thank you..!!

(Please make sure to set the review flag to '?' and put in :MattN as the reviewer when submitting patches)
Flags: needinfo?(wajekarprasad)
Attachment #8825391 - Flags: review?(MattN+bmo)
Assignee: nobody → meghpararajkumar
Status: NEW → ASSIGNED
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Comment on attachment 8825391 [details] [diff] [review]
> please Review it..!!Thank you..!!
> 
> (Please make sure to set the review flag to '?' and put in :MattN as the
> reviewer when submitting patches)

Ohk,Sir..Next time i'll make sure..thank you..!!
Comment on attachment 8825391 [details] [diff] [review]
please Review it..!!Thank you..!!

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

Thanks Raj, I have some comments on your patch.

::: browser/base/content/browser-addons.js
@@ +731,4 @@
>    updateStyleSheet(headerImage) {
>      if (!this.styleSheet)
>        return;
> +    document.documentElement.style.setProperty(`(--lwt-header-image)`, headerImage);

You have extra braces inside the backticks here. I would also just use regular double quotes.

@@ +737,3 @@
>    },
>  
> + 

Nit: extra whitespace

::: browser/themes/linux/browser-lightweightTheme.css
@@ +7,5 @@
>  
> +
> +#tabbrowser-tabs{
> + --lwt-header-image: @fgTabTextureLWT@ ;
> +}

I don't think you need a rule like this here though I haven't used variables much. If you do need a default value for the variable, put it in :root otherwise this more specific rule will override the variable value set by the code in browser-addons.js. A reasonable default value would be `none` probably.

@@ +19,4 @@
>  #tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab > .tab-stack > .tab-background > .tab-background-end[selected=true]:-moz-lwtheme::before {
>    background-attachment: scroll, fixed;
>    background-color: transparent;
> +  background-image: var(--lwt-header-image);/*, lwtHeader;*/

Throughout the CSS files you made the same mistake:
The comments 
> ;/*, lwtHeader;*/
are what should be replaced with: 
> , var(--lwt-header-image);

You should do a build and test your patch with a lightweight theme applied (install from https://addons.mozilla.org/firefox/themes/) and check what happens when you drag a tab horizontally on the tabstrip.
Attachment #8825391 - Flags: review?(MattN+bmo) → feedback+
Attached patch improved patch.. (obsolete) — Splinter Review
Attachment #8825901 - Flags: review?(MattN+bmo)
Comment on attachment 8825901 [details] [diff] [review]
improved patch..

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

::: browser/themes/osx/browser-lightweightTheme.css
@@ +14,5 @@
>  #tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab > .tab-stack > .tab-background > .tab-background-start[selected=true]:-moz-lwtheme::before,
>  #tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab > .tab-stack > .tab-background > .tab-background-end[selected=true]:-moz-lwtheme::before {
>    background-attachment: scroll, fixed;
>    background-color: transparent;
> +  background-image: var(--lwt-header-image);/*,var(--lwt-header-image);*/

Hi Raj, you shouldn't be changing the @fgTabTextureLWT@ portion, only replacing the comment with a variable
Attachment #8825901 - Flags: review?(MattN+bmo) → feedback+
Attached patch Changes.patch (obsolete) — Splinter Review
Attachment #8826288 - Flags: review?(MattN+bmo)
Comment on attachment 8826288 [details] [diff] [review]
Changes.patch

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

Please test your patches before uploading Raj.

After you get this working, in a separate commit we will then want to move these styles into the regular browser.css files for each theme as it no longer needs to be in its own file, that was only for more efficient iteration of rules in browser-addons.js.

::: browser/base/content/browser-addons.js
@@ +731,4 @@
>    updateStyleSheet(headerImage) {
>      if (!this.styleSheet)
>        return;
> +    document.documentElement.style.setProperty("lwt-header-image", headerImage);

You're missing the "--" at the beginning of the property name so I guess you didn't test the patch again? Please apply a lightweight theme and compare what happens when you drag a tab horizontally before and after the patch. You will see it's not working.

::: browser/themes/linux/browser-lightweightTheme.css
@@ -5,1 @@
>  %include linuxShared.inc

Please revert the whitespace changes above

@@ +10,5 @@
>   */
>  
>  /* Lightweight theme on tabs */
> +:root{
> + --lwt-header-image: null;

Move these to the regular browser.css files

@@ +30,3 @@
>    background-position: 0 0, 0 0, right top;
>    background-repeat: repeat-x, repeat-x, no-repeat;
> +}
\ No newline at end of file

Please revert this whitespace change

::: browser/themes/osx/browser-lightweightTheme.css
@@ -12,1 @@
>  /* Lightweight theme on tabs */

Please revert the whitespace changes above

@@ +24,4 @@
>    background-attachment: scroll, scroll, fixed;
>    background-color: transparent;
>    background-image: url(chrome://browser/skin/tabbrowser/tab-active-middle.png),
> +                   @fgTabTextureLWT@,var(--lwt-header-image);

Can you put the indentation back to how it was and keep/move the var to it's own line.

::: browser/themes/windows/browser-lightweightTheme.css
@@ -13,1 @@
>  /* Lightweight theme on tabs */

Please revert the whitespace changes above
Attachment #8826288 - Flags: review?(MattN+bmo) → feedback+
Hi Raj, I've finished up your patch and addressed Matt's feedback. In my commit message on the patch I gave you credit for the original patch.
Assignee: meghpararajkumar → jaws
Comment on attachment 8838789 [details]
Bug 864562 - Use CSS variables for lightweight theme styling.

https://reviewboard.mozilla.org/r/113608/#review115178

As I mentioned in a previous comment, the lwt CSS files should be folded into the platform's browser.css (putting the rules near other related rules) in a separate commit or bug. Then we can remove even more code from browser-addons

::: browser/base/content/browser-addons.js:800
(Diff revision 1)
>      XPCOMUtils.defineLazyGetter(this, "styleSheet", function() {
>        for (let i = document.styleSheets.length - 1; i >= 0; i--) {
>          let sheet = document.styleSheets[i];
>          if (sheet.href == "chrome://browser/skin/browser-lightweightTheme.css")
>            return sheet;
>        }
>        return undefined;
>      });

I don't think any of the this.styleSheet references are actually needed anymore so you could delete this getter and the references too.

::: browser/themes/osx/browser-lightweightTheme.css:7
(Diff revision 1)
>  /*
>   * LightweightThemeListener will append the current lightweight theme's header
>   * image to the background-image for each of the following rulesets.
>   */

Can you update this comment for all three OSs to say that the variable gets set there rather than being appended?
Attachment #8838789 - Flags: review?(MattN+bmo) → review+
Attachment #8826288 - Attachment is obsolete: true
Attachment #8825901 - Attachment is obsolete: true
Attachment #8825391 - Attachment is obsolete: true
Comment on attachment 8839543 [details]
Bug 864562 - Declare lwtheme CSS in tabs.inc.css since we don't need to iterate rules anymore.

https://reviewboard.mozilla.org/r/114156/#review115706

::: browser/themes/linux/browser.css:1317
(Diff revision 1)
> +/*
> + * LightweightThemeListener will set the current lightweight theme's header
> + * image to the lwt-header-image variable, used in each of the following rulesets.
> + */
> +
> +/* Lightweight theme on tabs */
> +#tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab > .tab-stack > .tab-background > .tab-background-start[selected=true]:-moz-lwtheme::before,
> +#tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab > .tab-stack > .tab-background > .tab-background-end[selected=true]:-moz-lwtheme::before {
> +  background-attachment: scroll, fixed;
> +  background-color: transparent;
> +  background-image: @fgTabTextureLWT@, var(--lwt-header-image);
> +  background-position: 0 0, right top;
> +  background-repeat: repeat-x, no-repeat;
> +}
> +
> +#tabbrowser-tabs:not([movingtab]) > .tabbrowser-tab > .tab-stack > .tab-background > .tab-background-middle[selected=true]:-moz-lwtheme {
> +  background-attachment: scroll, scroll, fixed;
> +  background-color: transparent;
> +  background-image: url(chrome://browser/skin/tabbrowser/tab-active-middle.png),
> +                    @fgTabTextureLWT@,
> +                    var(--lwt-header-image);
> +  background-position: 0 0, 0 0, right top;
> +  background-repeat: repeat-x, repeat-x, no-repeat;
> +}

It turns out that the rules outside of `@media (min-resolution: …dppx)` in the browser-lightweightTheme.css files are indentical so could you please move them to tabs.inc.css above `/* End selected tab */` and move the >1dppx rules inside the existing `@media (min-resolution: 1.1dppx) {` block inside that file above `.tab-throbber[busy]`.

I checked that the 2x images exist on all three platforms.
Attachment #8839543 - Flags: review?(MattN+bmo)
Comment on attachment 8839543 [details]
Bug 864562 - Declare lwtheme CSS in tabs.inc.css since we don't need to iterate rules anymore.

https://reviewboard.mozilla.org/r/114156/#review116284

Thanks
Attachment #8839543 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8840094 [details]
Bug 864562 - CustomizeMode should use CSS variables for setting LWT properties.

https://reviewboard.mozilla.org/r/114630/#review116290

::: browser/themes/windows/browser.css:2374
(Diff revision 1)
>    /* The image will get set from CustomizeMode.jsm */
> -  background-image: none;
> +  background-image: var(--lwt-header-image);

Drive-by: I think you're missing the change for linux: https://dxr.mozilla.org/mozilla-central/rev/32dcdde1fc64fc39a9065dc4218265dbc727673f/browser/themes/linux/browser.css#1556-1557
Comment on attachment 8840073 [details]
Bug 864562 - Move remaining LWT inline styles to CSS variables.

https://reviewboard.mozilla.org/r/114608/#review116354

::: browser/themes/linux/browser.css:45
(Diff revision 2)
>    --urlbar-separator-color: ThreeDShadow;
>  
>    --lwt-header-image: none;
>  }
>  
> +:root {

I'm thinking: why not put all these new lwtheme things in browser.inc? I mean, they're the same on all platforms and will be from now on. Plus, we'll be adding more and more of these variables to :root as we continue with the API.
Attachment #8840073 - Flags: review?(mdeboer)
Comment on attachment 8840094 [details]
Bug 864562 - CustomizeMode should use CSS variables for setting LWT properties.

https://reviewboard.mozilla.org/r/114630/#review116356

r=me with Matt's issue fixed.
Attachment #8840094 - Flags: review?(mdeboer) → review+
Comment on attachment 8840073 [details]
Bug 864562 - Move remaining LWT inline styles to CSS variables.

https://reviewboard.mozilla.org/r/114608/#review116354

> I'm thinking: why not put all these new lwtheme things in browser.inc? I mean, they're the same on all platforms and will be from now on. Plus, we'll be adding more and more of these variables to :root as we continue with the API.

Mainly because browser.inc only defines filters right now. It could make sense to add variable definitions, but this patch isn't doing that. This patch is applying styles to various nodes (:root and #browser-bottombox:-moz-lwtheme).

Maybe we re-create browser-lightweightThemes.css (or under a related name), but it would be confusing to move these rule definitions to browser.inc but not move other rules there such as the "Lightweight theme on tabs" section of tabs.inc.css.
Comment on attachment 8840073 [details]
Bug 864562 - Move remaining LWT inline styles to CSS variables.

https://reviewboard.mozilla.org/r/114608/#review116426

As per discussion on IRC: r=me. Thanks!

Also interesting to keep an eye on Talos regressions here - we're adding stuff to :root after all, even though they're no-ops by default.
Attachment #8840073 - Flags: review?(mdeboer) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2dc596ad4240
Use CSS variables for lightweight theme styling. r=MattN
https://hg.mozilla.org/integration/autoland/rev/2308f6c6fddf
Declare lwtheme CSS in tabs.inc.css since we don't need to iterate rules anymore. r=MattN
https://hg.mozilla.org/integration/autoland/rev/a99f7690feea
Move remaining LWT inline styles to CSS variables. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/028a63d5008c
CustomizeMode should use CSS variables for setting LWT properties. r=mikedeboer
Backed out for failing browser_ext_themes_chromeparity.js:

https://hg.mozilla.org/integration/autoland/rev/29f6ae6d3d0b5045e91dca18eb4400e8b434b63f
https://hg.mozilla.org/integration/autoland/rev/010d89bb2adbe319c7ddff7e3301846d5c67f22a
https://hg.mozilla.org/integration/autoland/rev/64df634e26a435ba4243e86967dadd948a5c00a0
https://hg.mozilla.org/integration/autoland/rev/b4a3f056216c271ba4955c575cbe655d469ed941

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=028a63d5008ce088fd9728f82c9d1f8704577d90&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=79700417&repo=autoland

[task 2017-02-23T18:09:27.440968Z] 18:09:27     INFO - TEST-START | browser/components/extensions/test/browser/browser_ext_themes_chromeparity.js
[task 2017-02-23T18:09:27.908545Z] 18:09:27     INFO - TEST-INFO | started process screentopng
[task 2017-02-23T18:09:29.227849Z] 18:09:29     INFO - TEST-INFO | screentopng: exit 0
[task 2017-02-23T18:09:29.232151Z] 18:09:29     INFO - Buffered messages logged at 18:09:27
[task 2017-02-23T18:09:29.232430Z] 18:09:29     INFO - Entering test bound setup
[task 2017-02-23T18:09:29.232673Z] 18:09:29     INFO - Leaving test bound setup
[task 2017-02-23T18:09:29.235353Z] 18:09:29     INFO - Entering test bound test_support_theme_frame
[task 2017-02-23T18:09:29.237344Z] 18:09:29     INFO - Extension loaded
[task 2017-02-23T18:09:29.239761Z] 18:09:29     INFO - Console message: Warning: attempting to write 14241 bytes to preference extensions.webextensions.uuids. This is bad for general performance and memory usage. Such an amount of data should rather be written to an external file. This preference will not be sent to any content processes.
[task 2017-02-23T18:09:29.247416Z] 18:09:29     INFO - Console message: parsing color=accentcolor
[task 2017-02-23T18:09:29.248312Z] 18:09:29     INFO - Console message: parsing color=frame
[task 2017-02-23T18:09:29.251251Z] 18:09:29     INFO - Console message: parsing color=tab_text
[task 2017-02-23T18:09:29.255631Z] 18:09:29     INFO - Console message: parsing color=textcolor
[task 2017-02-23T18:09:29.258849Z] 18:09:29     INFO - Console message: Security Error: Content at chrome://browser/skin/browser.css may not load or link to moz-extension://6c6e84a8-8095-48f3-a975-d2ccdcdc10fc/face.png.
[task 2017-02-23T18:09:29.261004Z] 18:09:29     INFO - Console message: Security Error: Content at chrome://browser/skin/browser.css may not load or link to moz-extension://6c6e84a8-8095-48f3-a975-d2ccdcdc10fc/face.png.
[task 2017-02-23T18:09:29.267376Z] 18:09:29     INFO - Console message: Security Error: Content at chrome://browser/skin/browser.css may not load or link to moz-extension://6c6e84a8-8095-48f3-a975-d2ccdcdc10fc/face.png.
[task 2017-02-23T18:09:29.269586Z] 18:09:29     INFO - Console message: Security Error: Content at chrome://browser/skin/browser.css may not load or link to moz-extension://6c6e84a8-8095-48f3-a975-d2ccdcdc10fc/face.png.
[task 2017-02-23T18:09:29.271687Z] 18:09:29     INFO - Console message: Security Error: Content at chrome://browser/skin/browser.css may not load or link to moz-extension://6c6e84a8-8095-48f3-a975-d2ccdcdc10fc/face.png.
[task 2017-02-23T18:09:29.273688Z] 18:09:29     INFO - Console message: Security Error: Content at chrome://browser/skin/browser.css may not load or link to moz-extension://6c6e84a8-8095-48f3-a975-d2ccdcdc10fc/face.png.
[task 2017-02-23T18:09:29.275824Z] 18:09:29     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_themes_chromeparity.js | LWT attribute should be set - true == true - 
[task 2017-02-23T18:09:29.278280Z] 18:09:29     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_themes_chromeparity.js | LWT text color attribute should be set - "bright" == "bright" - 
[task 2017-02-23T18:09:29.281653Z] 18:09:29     INFO - Buffered messages finished
[task 2017-02-23T18:09:29.284311Z] 18:09:29     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_themes_chromeparity.js | The backgroundImage should use face.png. Actual value is: none - false == true - JS frame :: chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_themes_chromeparity.js :: test_support_theme_frame :: line 45
[task 2017-02-23T18:09:29.289926Z] 18:09:29     INFO - Stack trace:
[task 2017-02-23T18:09:29.295410Z] 18:09:29     INFO -     chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_themes_chromeparity.js:test_support_theme_frame:45
[task 2017-02-23T18:09:29.296313Z] 18:09:29     INFO -     process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
[task 2017-02-23T18:09:29.297260Z] 18:09:29     INFO -     walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
[task 2017-02-23T18:09:29.298637Z] 18:09:29     INFO -     Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
[task 2017-02-23T18:09:29.302587Z] 18:09:29     INFO -     schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
[task 2017-02-23T18:09:29.308383Z] 18:09:29     INFO -     completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
[task 2017-02-23T18:09:29.310226Z] 18:09:29     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:324:15
[task 2017-02-23T18:09:29.315176Z] 18:09:29     INFO -     promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
[task 2017-02-23T18:09:29.317027Z] 18:09:29     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-02-23T18:09:29.318737Z] 18:09:29     INFO -     TaskImpl@resource://gre/modules/Task.jsm:277:3
[task 2017-02-23T18:09:29.320548Z] 18:09:29     INFO -     asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-02-23T18:09:29.322221Z] 18:09:29     INFO -     Task_spawn@resource://gre/modules/Task.jsm:166:12
[task 2017-02-23T18:09:29.323943Z] 18:09:29     INFO -     TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:389:16
[task 2017-02-23T18:09:29.325864Z] 18:09:29     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:327:15
[task 2017-02-23T18:09:29.328462Z] 18:09:29     INFO -     TaskImpl@resource://gre/modules/Task.jsm:277:3
[task 2017-02-23T18:09:29.330278Z] 18:09:29     INFO -     asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-02-23T18:09:29.332215Z] 18:09:29     INFO -     Task_spawn@resource://gre/modules/Task.jsm:166:12
[task 2017-02-23T18:09:29.334159Z] 18:09:29     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
[task 2017-02-23T18:09:29.336582Z] 18:09:29     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
[task 2017-02-23T18:09:29.338582Z] 18:09:29     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59
Flags: needinfo?(jaws)
Hey Mike, are you able to take a look at my WIP patch here? I've got a test failure in browser_ext_themes_chromeparity.js because the background-image is being returned as "none". However, I can't figure out why the style system isn't updating the display with the correct background image.

If I use the Browser Toolbox, I can see that the variable is set correctly and the correct style is applied, but the style doesn't get rendered. Not until I manually set "background-image: var(--lwt-header-image);" as an inline rule, and then after that I can delete the rule and the new CSS image from the previous will stick.
Flags: needinfo?(jaws) → needinfo?(mdeboer)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #40)
> Hey Mike, are you able to take a look at my WIP patch here? I've got a test
> failure in browser_ext_themes_chromeparity.js because the background-image
> is being returned as "none". However, I can't figure out why the style
> system isn't updating the display with the correct background image.

I've encountered this before and I think it's a gray corner of the CSS var implementation. Let's ask Cameron!
Flags: needinfo?(cam)
I'm away from today, but maybe Xidorn can take a look.
Flags: needinfo?(cam) → needinfo?(xidorn+moz)
As a workaround, you might be able to the value of the CSS var on the root element? Cuz what we mostly care about is that our unit of code works as expected here.
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #48)
> As a workaround, you might be able to the value of the CSS var on the root
> element? Cuz what we mostly care about is that our unit of code works as
> expected here.

It works just fine when the image is a data URI like we use in browser_ext_themes_lwtsupport.js, but in browser_ext_themes_chromeparity.js we use a local file and the image doesn't get displayed by that test.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #49)
> (In reply to Mike de Boer [:mikedeboer] from comment #48)
> > As a workaround, you might be able to the value of the CSS var on the root
> > element? Cuz what we mostly care about is that our unit of code works as
> > expected here.
> 
> It works just fine when the image is a data URI like we use in
> browser_ext_themes_lwtsupport.js, but in browser_ext_themes_chromeparity.js
> we use a local file and the image doesn't get displayed by that test.

The same thing happens when I load the extension manually through about:debugging so I don't think it's related to SpecialPowers.loadExtension.
Ah, I've got something here... in the browser console:
> Security Error: Content at chrome://browser/skin/browser.css may not load or link to moz-extension://72170bad-e43e-4922-a1ed-16031a6561d7/face.png.
Could you provide a minimum reproducible example for helping me understand where the problem is?
Flags: needinfo?(xidorn+moz) → needinfo?(mdeboer)
MattN helped me figure this out due to him encountering a similar issue years ago[1]. The load was failing because the resource is in a moz-extension:// URI and was being attempted to load from chrome://browser/skin, which is not in the system principle.

The load must be moved to chrome://browser/content. See https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/res/ExtensionProtocolHandler.cpp#32-46

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=928790#c12
Flags: needinfo?(mdeboer)
Comment on attachment 8842177 [details]
Bug 864562 - Rename lwt-accentcolor to lwt-accent-color and lwt-textcolor to lwt-text-color.

https://reviewboard.mozilla.org/r/116086/#review117868

::: browser/base/content/browser.css:15
(Diff revision 1)
>    --identity-popup-expander-width: 38px;
>    --panelui-subview-transition-duration: 150ms;
>  }
>  
>  :root:-moz-lwtheme {
> -  color: var(--lwt-textcolor) !important;
> +  color: var(--lwt-text-color) !important;

I think we need to make a bigger decision here: what naming scheme will we adopt for newer properties that we'll add for the theming API?
We'll need to think about the following:

1. Each property is already part of a category, which we can put in as a CSS var prefix. E.g. 'accent' --> '--lwt-color-accent' or 'header' --> '--lwt-image-header'
2. Is 'lwt' an appropriate and future-proof prefix to maintain? Shouldn't we simply say '--theme-image-header'?
Attachment #8842177 - Flags: review?(mdeboer) → review-
Comment on attachment 8841089 [details]
Bug 864562 - Fix the tests and move more logic from JS to CSS now that the values exist as CSS Variables.

https://reviewboard.mozilla.org/r/115430/#review117872

Deferring review until we've discussed the open questions.

::: browser/base/content/browser-addons.js
(Diff revision 2)
>  };
> -
> -/*
> - * Listen for Lightweight Theme styling changes and update the browser's theme accordingly.
> - */
> -var LightweightThemeListener = {

\o/ good riddance!

::: browser/base/content/browser.css:14
(Diff revision 2)
>  :root {
>    --identity-popup-expander-width: 38px;
>    --panelui-subview-transition-duration: 150ms;
>  }
>  
> +:root:-moz-lwtheme {

Yeah, I can see why you moved it here. (I seem to recall mentioning it some other place...)

However, Is this really going to be the place where we'll put all our CSS vars of the theming API?
Can it be argued that browser.inc.css is now _the_ place to be?

::: browser/base/content/browser.css:17
(Diff revision 2)
>  }
>  
> +:root:-moz-lwtheme {
> +  color: var(--lwt-textcolor) !important;
> +}
> +:root:-moz-lwtheme:not([customization-lwtheme]) {

nit: please add a newline here. The grouping is clear to me through the selector.
Attachment #8841089 - Flags: review?(mdeboer)
Comment on attachment 8842177 [details]
Bug 864562 - Rename lwt-accentcolor to lwt-accent-color and lwt-textcolor to lwt-text-color.

https://reviewboard.mozilla.org/r/116086/#review117868

> I think we need to make a bigger decision here: what naming scheme will we adopt for newer properties that we'll add for the theming API?
> We'll need to think about the following:
> 
> 1. Each property is already part of a category, which we can put in as a CSS var prefix. E.g. 'accent' --> '--lwt-color-accent' or 'header' --> '--lwt-image-header'
> 2. Is 'lwt' an appropriate and future-proof prefix to maintain? Shouldn't we simply say '--theme-image-header'?

This change was just to bring these variable names into consistency with lwt-header-image, where the value should be used for CSS image values. lwt-accent-color follows that convention.

I was hoping to have the larger change that you're asking for in (2) as part of whatever adds the new properties.
Comment on attachment 8841089 [details]
Bug 864562 - Fix the tests and move more logic from JS to CSS now that the values exist as CSS Variables.

https://reviewboard.mozilla.org/r/115430/#review117872

> Yeah, I can see why you moved it here. (I seem to recall mentioning it some other place...)
> 
> However, Is this really going to be the place where we'll put all our CSS vars of the theming API?
> Can it be argued that browser.inc.css is now _the_ place to be?

browser.inc.css is in browser/skin, so it's not a workable solution. As it stands right now, I'd prefer to keep them in this file, and then move them out to a new file at the point where it becomes too large.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #62)
> This change was just to bring these variable names into consistency with
> lwt-header-image, where the value should be used for CSS image values.
> lwt-accent-color follows that convention.

Hmmkay, I can see that. rs=me.

> I was hoping to have the larger change that you're asking for in (2) as part
> of whatever adds the new properties.

OK, I'm totally fine with that. As long as we both know when and where it's happening.
Comment on attachment 8842177 [details]
Bug 864562 - Rename lwt-accentcolor to lwt-accent-color and lwt-textcolor to lwt-text-color.

https://reviewboard.mozilla.org/r/116086/#review117886
Attachment #8842177 - Flags: review- → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #63)
> browser.inc.css is in browser/skin, so it's not a workable solution. As it
> stands right now, I'd prefer to keep them in this file, and then move them
> out to a new file at the point where it becomes too large.

But LTWs and our themes never really disable the default theme but augment it instead, no?
I'm looking for a clear and definitive answer where our new stuff, including LWT vars, should live.
(In reply to Mike de Boer [:mikedeboer] from comment #66)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #63)
> > browser.inc.css is in browser/skin, so it's not a workable solution. As it
> > stands right now, I'd prefer to keep them in this file, and then move them
> > out to a new file at the point where it becomes too large.
> 
> But LTWs and our themes never really disable the default theme but augment
> it instead, no?
> I'm looking for a clear and definitive answer where our new stuff, including
> LWT vars, should live.

It seems anything that touches images will need to live in /browser/content. I'd rather not make a split about colors being in /browser/skin and images being in /browser/content. MattN made a good argument that all should be in /browser/content since this is a feature being implemented and not necessarily rules for the theme.

We may want to create a new browser.inc.css (not the one you were referring to from bug 1309260), but one in /browser/base/content/. Can we do that in a follow-up or as part of the next set of work though?
webext-theme.inc.css or lightweighttheme.inc.css or theme-vars.inc.css (I think I like this last one the most)
Okidoke! (I like the last one the best as well, I think.
Comment on attachment 8841089 [details]
Bug 864562 - Fix the tests and move more logic from JS to CSS now that the values exist as CSS Variables.

https://reviewboard.mozilla.org/r/115430/#review117896
Attachment #8841089 - Flags: review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/85736c04bf96
Use CSS variables for lightweight theme styling. r=MattN
https://hg.mozilla.org/integration/autoland/rev/14640efe4084
Declare lwtheme CSS in tabs.inc.css since we don't need to iterate rules anymore. r=MattN
https://hg.mozilla.org/integration/autoland/rev/9c6075b44bd9
Move remaining LWT inline styles to CSS variables. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/7778cdded03a
CustomizeMode should use CSS variables for setting LWT properties. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/0a0a200dd138
Fix the tests and move more logic from JS to CSS now that the values exist as CSS Variables. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/d48b6ebcda5c
Rename lwt-accentcolor to lwt-accent-color and lwt-textcolor to lwt-text-color. r=mikedeboer
Blocks: 1344307
No longer depends on: 1344926
Depends on: 1344839
You need to log in before you can comment on or make changes to this bug.