Closed Bug 511107 Opened 11 years ago Closed 11 years ago

Need a centralized way to assign lightweight themes to XUL windows

Categories

(Toolkit :: XUL Widgets, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: dev-doc-complete, verified1.9.2)

Attachments

(2 files, 9 obsolete files)

No description provided.
Attached patch wip (obsolete) — Splinter Review
The styling bits are tested on Linux and likely broken on Windows and OS X.
Attached patch wip 2 (obsolete) — Splinter Review
Attachment #395063 - Attachment is obsolete: true
This looks more like it belongs in the Personas project than in of browser/ and|or toolkit/
Attached patch wip 3 (obsolete) — Splinter Review
Attachment #395075 - Attachment is obsolete: true
Attached patch wip 4 (obsolete) — Splinter Review
Attachment #395298 - Attachment is obsolete: true
Attached patch wip 5 (obsolete) — Splinter Review
Attachment #395438 - Attachment is obsolete: true
Depends on: 511108
Attached patch wip 6 (obsolete) — Splinter Review
Attachment #395655 - Attachment is obsolete: true
Blocks: 508761
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #395698 - Attachment is obsolete: true
Attachment #397073 - Flags: review?(mconnor)
Attachment #397073 - Flags: review?(rflint)
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!
Attachment #397073 - Flags: review?(rflint) → review+
Summary: Need a way to assign arbitrary header and footer images to XUL windows → Need a centralized way to assign lightweight themes to XUL windows
Depends on: 513461
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #397073 - Attachment is obsolete: true
Attachment #397533 - Flags: review?(mconnor)
Attachment #397073 - Flags: review?(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
Attachment #397533 - Flags: review?(mconnor) → review+
(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 on attachment 397533 [details] [diff] [review]
patch v2

Enn, could you please review (at least) the xul.css changes?
Attachment #397533 - Flags: review?(enndeakin)
Blocks: 511771
Attachment #397533 - Flags: review?(enndeakin) → review+
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.
Attached patch for check-in (obsolete) — Splinter Review
Attachment #397533 - Attachment is obsolete: true
Component: Toolbars and Toolbar Customization → XUL Widgets
QA Contact: toolbars → xul.widgets
http://hg.mozilla.org/mozilla-central/rev/e75c771c6ab8
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Attachment #398393 - Flags: approval1.9.2?
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
So, while the attribute in css is "lwtheme" I see the attribute in browser.xul is "lightweightthemes", any reason for that?
Status: REOPENED → ASSIGNED
(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.
Attachment #398393 - Flags: approval1.9.2?
Attached patch for check-inSplinter Review
stupid typo :(
Attachment #398393 - Attachment is obsolete: true
Attachment #398632 - Flags: approval1.9.2?
http://hg.mozilla.org/mozilla-central/rev/a15c41a408de
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Depends on: 514782
Attached patch 1.9.2 patchSplinter Review
this applies cleanly on 1.9.2 and incorporates the fix for bug 514782
Attachment #398834 - Flags: approval1.9.2?
Attachment #398632 - Flags: approval1.9.2?
Keywords: dev-doc-needed
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.2+
Attachment #398834 - Flags: approval1.9.2?
No, test_LightweightThemeManager.js doesn't cover this...
Depends on: 516701
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.
Whiteboard: [doc-waiting-info]
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)
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.
(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.
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.
Whiteboard: [doc-waiting-info]
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
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
You need to log in before you can comment on or make changes to this bug.