511107 - Need a centralized way to assign lightweight themes to XUL windows

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, enhancement)

Comment 1

[Dão Gottwald [:dao]]

Attachment: wip

The styling bits are tested on Linux and likely broken on Windows and OS X.

Comment 2

[Dão Gottwald [:dao]]

Attachment: wip 2

Comment 3

[Philip Chee]

This looks more like it belongs in the Personas project than in of browser/ and|or toolkit/

Comment 4

[Dão Gottwald [:dao]]

Attachment: wip 3

Comment 5

[Dão Gottwald [:dao]]

Attachment: wip 4

Comment 6

[Dão Gottwald [:dao]]

Attachment: wip 5

Comment 7

[Dão Gottwald [:dao]]

Attachment: wip 6

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch v1

Comment 9

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Comment on attachment 397073 [details] [diff] [review]
patch v1

It'd be cool to eventually get a pseudo-class for lightweighttheme/darktextcolor so we can eliminate a bunch of the descendant selectors - otherwise the theme portions look good!

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch v2

In order to use pseudo classes for this (bug 513461), it needs to be enforced that the header image is set on the root element. This patch does that.

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 397533 [details] [diff] [review]
patch v2

>diff --git a/toolkit/content/LightweightThemeConsumer.jsm b/toolkit/content/LightweightThemeConsumer.jsm

>+function _setImage(aElement, aActive, aURL) {
>+  aElement.style.backgroundImage =
>+    (aActive && aURL) ? 'url("' + aURL.replace('"', '\\"', "g") + '")' : "";
>+}
>+
>+function _parseRGB(aColorString) {
>+  var rgb = aColorString.match(/^rgba?\((\d+), (\d+), (\d+)/);
>+  rgb.shift();
>+  return rgb.map(function (x) parseInt(x));
>+}

any particular reason these are outside of the object?

>diff --git a/toolkit/content/xul.css b/toolkit/content/xul.css
>--- a/toolkit/content/xul.css
>+++ b/toolkit/content/xul.css
>@@ -14,16 +14,17 @@
>   -moz-user-focus: ignore;
>   -moz-user-select: -moz-none;
>   display: -moz-box;
>   -moz-box-sizing: border-box;
> }
> 
> :root {
>   text-rendering: optimizeLegibility;
>+  -moz-binding: url("chrome://global/content/bindings/general.xml#root-element");
> }

why are we applying this to every root element in XUL? why not just add it conditionally like this:

:root[lightweightthemes="true"]

this is mostly okay, r=me, subject to either fixes or convincing reasoning for the comments above.  either way, the xul.css change requires review from Enn

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> >+function _setImage(aElement, aActive, aURL) {
> >+  aElement.style.backgroundImage =
> >+    (aActive && aURL) ? 'url("' + aURL.replace('"', '\\"', "g") + '")' : "";
> >+}
> >+
> >+function _parseRGB(aColorString) {
> >+  var rgb = aColorString.match(/^rgba?\((\d+), (\d+), (\d+)/);
> >+  rgb.shift();
> >+  return rgb.map(function (x) parseInt(x));
> >+}
> 
> any particular reason these are outside of the object?

I've asked myself the opposite question before moving them outside: Any reason they should be inside? They don't act on the object, and they're not part of the LightweightThemeConsumer "API".

> > :root {
> >   text-rendering: optimizeLegibility;
> >+  -moz-binding: url("chrome://global/content/bindings/general.xml#root-element");
> > }
> 
> why are we applying this to every root element in XUL? why not just add it
> conditionally like this:
> 
> :root[lightweightthemes="true"]

Because existing bindings for root elements (dialog, wizard) need to extend chrome://global/content/bindings/general.xml#root-element anyway. This is also why I called the binding root-element rather than something like root-element-lwthemes.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 397533 [details] [diff] [review]
patch v2

Enn, could you please review (at least) the xul.css changes?

Comment 14

[Neil Deakin]

Comment on attachment 397533 [details] [diff] [review]
patch v2

>--- a/toolkit/content/widgets/dialog.xml
>+++ b/toolkit/content/widgets/dialog.xml
>@@ -6,17 +6,20 @@
>           xmlns:xbl="http://www.mozilla.org/xbl">
> 
>   <binding id="dialog-base">
>     <resources>
>       <stylesheet src="chrome://global/skin/dialog.css"/>
>     </resources>
>   </binding>

This is only used by the one binding now so it can just be incorporated into dialogheader.


>+  <binding id="root-element">
>+    <implementation>
>+      <field name="lightweightTheme">null</field>
>+      <constructor><![CDATA[
>+        if (this.hasAttribute("lightweightthemes")) {
>+          let temp = {};
>+          Components.utils.import("resource://gre/modules/LightweightThemeConsumer.jsm", temp);
>+          this.lightweightTheme = new temp.LightweightThemeConsumer(this.ownerDocument);

this._lightweightTheme please as there's no reason to use this outside the binding.


> /********** dialog **********/
> 
>-dialog {
>+dialog:root {
>   -moz-binding: url("chrome://global/content/bindings/dialog.xml#dialog");
>   -moz-box-orient: vertical;
> }

This change shouldn't be made.


>-wizard {
>+wizard:root {
>   -moz-binding: url("chrome://global/content/bindings/wizard.xml#wizard");
>   -moz-box-orient: vertical;
>   width: 40em;
>   height: 30em;
> }

Nor this one.

Comment 15

[Dão Gottwald [:dao]]

Attachment: for check-in

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/e75c771c6ab8

Comment 17

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Backed this out due to browser-chrome test failures: http://hg.mozilla.org/mozilla-central/rev/b5b3d45c1885

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1252022722.1252024653.6419.gz
TEST-INFO | chrome://mochikit/content/browser/toolkit/mozapps/plugins/tests/browser_bug435788.js | Console message: [JavaScript Error: "gPFS.document.documentElement.wizardPages is undefined" {file: "chrome://mochikit/content/browser/toolkit/mozapps/plugins/tests/browser_bug435788.js" line: 88}]
TEST-INFO | chrome://mochikit/content/browser/toolkit/mozapps/plugins/tests/browser_bug435788.js | Console message: [JavaScript Error: "document.getElementById("plugin-installer-wizard").advance is not a function" {file: "chrome://mozapps/content/plugins/pluginInstallerWizard.js" line: 330}]
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/mozapps/plugins/tests/browser_bug435788.js | Timed out

Comment 18

[Nochum Sossonko [:Natch]]

So, while the attribute in css is "lwtheme" I see the attribute in browser.xul is "lightweightthemes", any reason for that?

Comment 19

[Dão Gottwald [:dao]]

(In reply to comment #18)
> So, while the attribute in css is "lwtheme" I see the attribute in browser.xul
> is "lightweightthemes", any reason for that?

lightweightthemes activates the feature, while lwtheme indicates that a lightweight theme is actually in use.

Comment 20

[Dão Gottwald [:dao]]

Attachment: for check-in

stupid typo :(

Comment 21

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a15c41a408de

Comment 22

[Dão Gottwald [:dao]]

Attachment: 1.9.2 patch

this applies cleanly on 1.9.2 and incorporates the fix for bug 514782

Comment 23

[Tony Chung [:tchung]]

do we need another testcase for this?   or will http://hg.mozilla.org/mozilla-central/file/14cf3892b66f/toolkit/mozapps/extensions/test/unit/test_LightweightThemeManager.js suffice?

Comment 24

[Dão Gottwald [:dao]]

No, test_LightweightThemeManager.js doesn't cover this...

Comment 25

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d41f7d808a39

Comment 26

[Eric Shepherd [:sheppy]]

Do we have any existing documentation on how to create these themes? I'm presuming we have something somewhere, since Personas has been around for a while. If we do, we can borrow heavily from that (or copy it outright) for MDC.

Comment 27

[Dão Gottwald [:dao]]

http://getpersonas.com/en-US/demo_create

What needs to be documented specifically for this bug are these two attributes for XUL root elements:

lightweightthemes="true"
lightweightthemesfooter="some_id" (optional)

Comment 28

[Eric Shepherd [:sheppy]]

Looking at the code, it looks like lightweightthemes is true if a Persona-style theme is in use, but I'm not clear on what lightweightthemesfooter is for.

I tried looking at the material on getpersonas.com, but it keeps timing out.

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #28)
> Looking at the code, it looks like lightweightthemes is true if a Persona-style
> theme is in use

That attributes enables a window to use lightweight themes in general, but it doesn't mean that such a theme is actually being used.

> but I'm not clear on what lightweightthemesfooter is for.

It contains the id of the element that's going to pick up the footer image, e.g. the status bar in Firefox.

Comment 30

[Eric Shepherd [:sheppy]]

Added:

https://developer.mozilla.org/en/XUL/Attribute/lightweightthemes
https://developer.mozilla.org/en/XUL/Attribute/lightweightthemesfooter

Updated:

https://developer.mozilla.org/en/XUL/window

Added:

https://developer.mozilla.org/en/Themes/Lightweight_themes

Please feel free to apply corrections or make any suggested additions. For now, the main article just suggests reading the Personas site, because that content is shiny and pretty. At some point I expect we'll want to put that information directly onto MDC.

Comment 31

[Tony Chung [:tchung]]

Verified fix in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b2pre) Gecko/20091016 Namoroka/3.6b2pre 
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091016 Minefield/3.7a1pre