Closed Bug 513461 Opened 15 years ago Closed 15 years ago

Need pseudo classes for lightweight theme handling

Categories

(Core :: CSS Parsing and Computation, defect, P1)

defect

Tracking

()

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

People

(Reporter: dao, Assigned: Natch)

References

Details

(Keywords: dev-doc-complete, perf)

Attachments

(2 files, 9 obsolete files)

I'm currently using many selectors like ':root[lwtheme="true"] toolbar' in bug 511107, but we'd like to avoid descendant selectors. To achieve that, I'm proposing the following pseudo classes:

:-moz-lwtheme
Matches in chrome documents when the root element's lwtheme attribute is "true".

:-moz-lwtheme-darktext
Matches when :-moz-lwtheme matches and the root element's lwthemetextcolor attribute is "dark".

:-moz-lwtheme-brighttext
Matches when :-moz-lwtheme matches and the root element's lwthemetextcolor attribute is "bright".
Attached patch wip 1 (obsolete) — Splinter Review
Based on the patch in bug 478416 from Neil Deakin, this does the same thing for :-moz-lwtheme and :-moz-lwtheme-is(bright|dark).

This needs some tests. Also, in nsCSSRuleProcessor I'm not sure of a better way to assign the result aside for using 1 and 2, should I make a local enum that duplicates what nsXULDocument has?

This also doesn't work well with getComputedStyle, the style returned is the style of the pseudo selector, without specifying it. Specifying the pseudo selector does absolutely nothing afaict.
Assignee: nobody → highmind63
Status: NEW → ASSIGNED
Oh, and currently -moz-lwtheme only matches if 1) lwtheme="true" and 2) there's a valid value in lwthemetextcolor (either dark or bright) on the root element. I figured that's the intention, correct me if I'm wrong.
Just FYI, this will probably conflict with my patch in bug 508482.
Nochum, would you also like to update the stylesheets that use the attributes or should I prepare that patch?
Comment on attachment 398578 [details] [diff] [review]
wip 1

It would be good if you added a basic parsability test to layout/style/test/test_selectors.html (which will also test serialization and cloning a little bit) in addition to other tests you need.

>+   * Returns 0 if the document lightweight theme is uninitialized, 1 for no
>+   * theme, 2 for a dark theme and 3 for a light theme. This is used to
>+   * determine the state of the pseudoclasses :-moz-lwtheme and :-moz-lwtheme-is.

This sounds like what we use enums for.  I think you should move the enum you declare in nsXULDocument to nsIDocument so that this comment can use names rather than numbers.

>+        // if the lwtheme changed, make sure to restyle appropriately
>+        if ((aName == nsGkAtoms::lwtheme ||
>+             aName == nsGkAtoms::lwthemetextcolor) &&
>+            document && document->GetRootContent() == this) {
>+            nsCOMPtr<nsIXULDocument> xuldoc = do_QueryInterface(document);
>+            if (xuldoc) {
>+                xuldoc->ResetDocumentLWTheme();
>+            }
>+        }

It's possible we might want to use the document state mechanism that mstange is adding in bug 508482.  I think it has less direct benefit here, but there is value in using similar code.  I guess it's probably ok as it is now, though, since it's similar to the localedir code.

However, the comment here is wrong:  this code isn't doing any restyling:  it's just clearing the XUL document's cache.  The restyling is being done in nsCSSRuleProcessor::HasAttributeDependentStyle.

>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp
>--- a/layout/style/nsCSSParser.cpp
>+++ b/layout/style/nsCSSParser.cpp
>@@ -3323,16 +3323,24 @@ CSSParserImpl::ParsePseudoClassWithIdent
>   // -moz-locale-dir can only have values of 'ltr' or 'rtl'.
>   if (aPseudo == nsCSSPseudoClasses::mozLocaleDir) {
>     if (!mToken.mIdent.EqualsLiteral("ltr") &&
>         !mToken.mIdent.EqualsLiteral("rtl")) {
>       return eSelectorParsingStatus_Error;
>     }
>   }
> 
>+  // -moz-lwtheme-is can only have values of 'dark' or 'bright'.
>+  if (aPseudo == nsCSSPseudoClasses::mozLWThemeIs) {
>+    if (!mToken.mIdent.EqualsLiteral("dark") &&
>+        !mToken.mIdent.EqualsLiteral("bright")) {
>+      return eSelectorParsingStatus_Error;

Hmmm.  These should be case-insensitive, and so should the :-moz-locale-dir() arguments.  And it looks like they're not, although maybe I'm missing something.

>+// -moz-lwtheme may be used to match a document that has a lightweight theme
>+CSS_PSEUDO_CLASS(mozLWTheme, ":-moz-lwtheme")
>+
>+// -moz-lwtheme-is(dark) and -moz-lwtheme-is(bright) may be used to match the
>+// lightweight theme being used on the document
>+CSS_PSEUDO_CLASS(mozLWThemeIs, ":-moz-lwtheme-is")

I wonder if it would be better to call these :-moz-has-lwtheme and :-moz-lwtheme(dark)
-moz-lwtheme-is(dark) is wrong. This is about the text color, hence the proposed :-moz-lwtheme-darktext and :-moz-lwtheme-brighttext.
I'll update this on Sunday (hopefully), as to the names, how about -moz-has-lwtheme and -moz-lwtheme-text(dark|bright)? I'm trying to avoid adding more pseudo-selectors if possible. Also, this is more extensible.

I'll update the stylesheets and add a test as well.

About the casing, I can make it case-insensitive, I was just following what -moz-locale-dir was doing, do you want me to update that too?
(In reply to comment #7)
> I'm trying to avoid adding more pseudo-selectors if possible.

To what end?
I'd like to have this easily typeable. I also find :-moz-locale-dir(rtl) instead of :-moz-rtl-locale unfortunate.

> Also, this is more extensible.

I don't foresee us adding more distinctions, other than maybe neither dark nor bright, which would be no change from the style system perspective.
Attached patch patch (obsolete) — Splinter Review
Patch ready for review, including the parsing test and changed the names in accordance with the comments. The stylesheets have been updated as well.
Attachment #398578 - Attachment is obsolete: true
Attachment #398980 - Flags: superreview?(dbaron)
Attachment #398980 - Flags: review?(dbaron)
Comment on attachment 398980 [details] [diff] [review]
patch

Dao, can you review the stylesheet changes?
Attachment #398980 - Flags: review?(dao)
Attached patch patch (obsolete) — Splinter Review
Sorry for the bugspam, patch was full of cruft from earlier attempts and some line-endings were changed unintentionally.
Attachment #398980 - Attachment is obsolete: true
Attachment #398982 - Flags: superreview?(dbaron)
Attachment #398982 - Flags: review?(dbaron)
Attachment #398980 - Flags: superreview?(dbaron)
Attachment #398980 - Flags: review?(dbaron)
Attachment #398980 - Flags: review?(dao)
Attachment #398982 - Flags: review?(dao)
Why is :-moz-has-lwtheme better than :-moz-lwtheme?
Comment on attachment 398982 [details] [diff] [review]
patch

>+#main-window:-moz-has-lwtheme:not([active="true"]) .tabbrowser-tab {

In preparation for bug 508482, this should be:
#main-window:not([active="true"]) .tabbrowser-tab:-moz-has-lwtheme

Looks good otherwise. I'm going to test all three themes, as I think some selectors now have less weighting, and maybe not enough in some cases.
I implemented :-moz-has-lwtheme based on dbaron's comment. It seems more self-explanatory that way too. The selectors may indeed have gone a bit awry, I noticed that bookmarks in the bookmark toolbar get a very thick bold text when the theme is set to bright, yet when you hover over them they return to their regular font weight. I wasn't sure if that was the intention so I figured I'd mention it here just in case it's a side-effect of losing specificity and therefore applicability.
(In reply to comment #14)
> I implemented :-moz-has-lwtheme based on dbaron's comment. It seems more
> self-explanatory that way too.

The element doesn't have the lightweight theme, though -- the document has. (Like so many other things, maybe this should be an @ rule.)
Comment on attachment 398982 [details] [diff] [review]
patch

>--- a/toolkit/themes/gnomestripe/global/global.css
>+++ b/toolkit/themes/gnomestripe/global/global.css

>+toolbar:-moz-has-lwtheme,

>+menubar:-moz-has-lwtheme > menu,
>+toolbarbutton:-moz-has-lwtheme {

>+menubar:-moz-has-lwtheme > menu:not([open="true"]),
>+toolbarbutton:-moz-has-lwtheme:not(:hover):not([checked="true"]):not([open="true"]):not([disabled="true"])

>--- a/toolkit/themes/pinstripe/global/global.css
>+++ b/toolkit/themes/pinstripe/global/global.css

>+toolbar:-moz-has-lwtheme,
>+findbar:-moz-has-lwtheme,

>+splitter:-moz-has-lwtheme {

>+.findbar-container:-moz-has-lwtheme > label,
>+.findbar-find-status:-moz-has-lwtheme {

>--- a/toolkit/themes/winstripe/global/global.css
>+++ b/toolkit/themes/winstripe/global/global.css

>+toolbox:moz-has-lwtheme,
>+toolbar:moz-has-lwtheme,

>+splitter:moz-has-lwtheme {

>+toolbarbutton:moz-has-lwtheme {

>+menubar:moz-has-lwtheme > menu {

>+toolbarbutton:moz-has-lwtheme:not(:hover):not([checked="true"]):not([open="true"]):not([disabled="true"]) {

now that they don't depend on the root element anymore, these should probably be moved to toolbar.css / menu.css / toolbarbutton.css / findBar.css / splitter.css.

>--- a/toolkit/themes/winstripe/global/global.css
>+++ b/toolkit/themes/winstripe/global/global.css

>+.tabbrowser-tabs:moz-has-lwtheme {

This actually belongs in winstripe's browser.css. (This was wrong in the original patch.)
Attached patch patch ver. 1.1 (obsolete) — Splinter Review
This patch addresses all of Dao's previous comments.
Attachment #398982 - Attachment is obsolete: true
Attachment #399264 - Flags: superreview?(dbaron)
Attachment #399264 - Flags: review?(dbaron)
Attachment #398982 - Flags: superreview?(dbaron)
Attachment #398982 - Flags: review?(dbaron)
Attachment #398982 - Flags: review?(dao)
Attachment #399264 - Flags: review?(dao)
Comment on attachment 399264 [details] [diff] [review]
patch ver. 1.1

I still think -moz-has-lwtheme is wrong. While it is more self-explanatory, it's also misleading. It makes sense for the root element the element with lwthemefooter="true", which have the lightweight theme's background images and text color, but not for the descendant elements.

>+.toolbar-focustarget {
>+  -moz-user-focus: ignore !important;
>+}

This doesn't belong in toolbar.css. (It doesn't belong in any of these stylesheets, but that's another bug.)
Comment on attachment 399264 [details] [diff] [review]
patch ver. 1.1

>-toolbar[mode="text"] .toolbarbutton-text {
>-  padding: 0 !important;
>-  margin: 3px 5px !important;
>-}

This shouldn't be moved either.

>--- a/toolkit/themes/winstripe/global/menu.css
>+++ b/toolkit/themes/winstripe/global/menu.css

>+menubar:-moz-has-lwtheme > menu {
>+  -moz-appearance: none;
>+  color: inherit;
>+}
> menubar > menu[_moz-menuactive="true"][open="true"] {
>   border-width: 3px 1px 1px 3px;
> }
> menubar > menu[_moz-menuactive="true"],
> menubar > menu[_moz-menuactive="true"][open="true"] {
>   color: -moz-menubarhovertext;
> }

menubar > menu[_moz-menuactive="true"] seems to override menubar:-moz-has-lwtheme > menu. Adding :not(:-moz-lwtheme) to the former would be one way to solve this.

>+menubar:-moz-has-lwtheme > menu:not([open="true"]) {
>+  color: inherit;
>+  text-shadow: inherit;
>+}

You seem to have moved this from gnomestripe to winstripe.
Attachment #399264 - Flags: review?(dao) → review-
Dao, can you explain to me what [_moz-menuactive=true] matches so that I can test this before uploading another patch? I think I've worked it out, just need to know what it's for.
[_moz-menuactive="true"] can be triggered by moving the mouse over the menu, for instance, but it's actually menubar > menu[_moz-menuactive="true"][open="true"] what overrides the other selector, so just open a menu.
Attached patch patch ver. 1.2 (obsolete) — Splinter Review
Attachment #399264 - Attachment is obsolete: true
Attachment #399362 - Flags: superreview?(dbaron)
Attachment #399362 - Flags: review?(dbaron)
Attachment #399264 - Flags: superreview?(dbaron)
Attachment #399264 - Flags: review?(dbaron)
Attachment #399362 - Flags: review?(dao)
Comment on attachment 399362 [details] [diff] [review]
patch ver. 1.2

>--- a/toolkit/themes/gnomestripe/global/toolbar.css
>+++ b/toolkit/themes/gnomestripe/global/toolbar.css

>+toolbar:-moz-has-lwtheme {
>+  -moz-appearance: none;
>+}
>+
> menubar, toolbar[type="menubar"] {
>   -moz-appearance: menubar;

toolbar[type="menubar"] overrides toolbar:-moz-has-lwtheme here.

I've also noticed a small glitch with checkboxes in gnomestripe, which is not caused by your changes though. We're changing the background and text color on hover, but still inherit the text shadow. Could be fixed by adding text-shadow:none in checkbox.css:

114 checkbox:not([disabled="true"]):hover {
115   color: -moz-buttonhovertext;
116 }

The same applies to radio.css. Both could be moved to a separate bug if you don't want to touch it.


Other than that it seems more self-explanatory, can you explain why you think has-lwtheme is actually correct?
Attachment #399362 - Flags: review?(dao) → review-
Comment on attachment 399362 [details] [diff] [review]
patch ver. 1.2

>--- a/toolkit/themes/winstripe/global/toolbar.css
>+++ b/toolkit/themes/winstripe/global/toolbar.css

>+toolbox:-moz-has-lwtheme,
>+toolbar:-moz-has-lwtheme {
>+  -moz-appearance: none;
>+  background: none;
>+  border-style: none;
>+}

toolbar:first-child overrides this, adding back a bottom border.
Comment on attachment 399362 [details] [diff] [review]
patch ver. 1.2

--- a/toolkit/themes/winstripe/global/global.css
+++ b/toolkit/themes/winstripe/global/global.css

-:root[lwtheme="true"] menubar > menu {
-  -moz-appearance: none;
-  color: inherit;
-}

You haven't moved this to menu.css.
Attached patch patch ver. 1.3 (obsolete) — Splinter Review
(In reply to comment #25)

Um, I did move it, at least the line you point to in that comment. winstripe/global/menu.css there's on edit, this.

Everything else was addressed.
Attachment #399362 - Attachment is obsolete: true
Attachment #400643 - Flags: superreview?(dbaron)
Attachment #400643 - Flags: review?(dbaron)
Attachment #399362 - Flags: superreview?(dbaron)
Attachment #399362 - Flags: review?(dbaron)
Attachment #400643 - Flags: review?(dao)
(In reply to comment #26)
> Everything else was addressed.

Except for the checkbox changes?
Comment on attachment 400643 [details] [diff] [review]
patch ver. 1.3

>--- a/toolkit/themes/winstripe/global/menu.css
>+++ b/toolkit/themes/winstripe/global/menu.css

> menubar > menu[_moz-menuactive="true"],
> menubar > menu[_moz-menuactive="true"][open="true"] {
>   color: -moz-menubarhovertext;
> }
>+menubar > menu:-moz-has-lwtheme {
>+  -moz-appearance: none;
>+  color: inherit;
>+}

menubar > menu[_moz-menuactive="true"][open="true"] still overrides this. Probably best to use !important for the color.
Comment on attachment 400643 [details] [diff] [review]
patch ver. 1.3

>--- a/browser/themes/pinstripe/browser/browser.css
>+++ b/browser/themes/pinstripe/browser/browser.css

>-#main-window[lwtheme="true"] .tabbrowser-tab,
>-#main-window[lwtheme="true"]:not([active="true"]) .tabbrowser-tab {
>+.tabbrowser-tab:-moz-has-lwtheme,
>+#main-window:-moz-has-lwtheme:not([active="true"]) .tabbrowser-tab {
>   color: inherit;
>   text-shadow: inherit;
> }

Both .tabbrowser-tab[selected="true"] and .tabbrowser-tab:hover override .tabbrowser-tab:-moz-has-lwtheme, so you should move this down.
(In reply to comment #27)
> (In reply to comment #26)
> > Everything else was addressed.
> 
> Except for the checkbox changes?

Yeah, meant to comment about that too. I'd like to do that in a different bug, my editor is overloaded with tabs atm :)

Filed bug 516701. I'll update the rest tonight.
Attachment #400643 - Flags: review?(dbaron)
Comment on attachment 400643 [details] [diff] [review]
patch ver. 1.3

oops...
Attachment #400643 - Flags: review?(dao) → review?(dbaron)
Attached patch patch ver. 1.4 (obsolete) — Splinter Review
Update to comments.
Attachment #400643 - Attachment is obsolete: true
Attachment #401125 - Flags: superreview?(dbaron)
Attachment #401125 - Flags: review?(dbaron)
Attachment #401125 - Flags: review?(dao)
Attachment #400643 - Flags: superreview?(dbaron)
Attachment #400643 - Flags: review?(dbaron)
Comment on attachment 401125 [details] [diff] [review]
patch ver. 1.4

r=me provided that we find a compelling argument for has-lwtheme (I think we don't have one yet), or use lwtheme, or find a better third option (I'm not crazy about "lwtheme" as an abbreviation, but "lightweighttheme" seems verbose).
Attachment #401125 - Flags: review?(dao) → review+
I think we need this for 1.9.2, since inefficient selectors can slow down the UI quite a bit.
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1
I'm ok with :-moz-lwtheme instead of :-moz-has-lwtheme.
Attached patch patch ver. 1.5 (obsolete) — Splinter Review
s/-moz-has-lwtheme/-moz-lwtheme/g

Everything else remains the same.
Attachment #401125 - Attachment is obsolete: true
Attachment #402219 - Flags: superreview?(dbaron)
Attachment #402219 - Flags: review?(dbaron)
Attachment #401125 - Flags: superreview?(dbaron)
Attachment #401125 - Flags: review?(dbaron)
Attached patch patch ver. 1.5 (obsolete) — Splinter Review
Whoops, wrong patch. Sorry for the bugspam.
Attachment #402219 - Attachment is obsolete: true
Attachment #402220 - Flags: superreview?(dbaron)
Attachment #402220 - Flags: review?(dbaron)
Attachment #402219 - Flags: superreview?(dbaron)
Attachment #402219 - Flags: review?(dbaron)
dbaron: can we get a quick review of the patch with the change you asked for?
Keywords: dev-doc-needed
Is this really a beta-blocker? If so, I won't be available starting tomorrow, October 2 ~500PM EST, to update the patch in time for the freeze, so if review can be done sometime today I'd be able to update the patch still. Otherwise if there are any minor nits, can someone fill them in for me and land this?

(In reply to comment #38)
> dbaron: can we get a quick review of the patch with the change you asked for?

Are there any pending comments/changes that are expected from the current patch? I'm not aware of any...
Comment on attachment 402220 [details] [diff] [review]
patch ver. 1.5

nsIDocument.h:

+  virtual int GetDocumentLWTheme() { return Doc_Theme_Uninitialized; }

Seems like you should return Doc_Theme_None from nsIDocument.
(nsXULDocument can still initialize to Uninitialized.)

+  /**
+   * Returns Doc_Theme_Uninitialized if the document lightweight theme is
+   * uninitialized, Doc_Theme_None if there is no lwtheme specified, Doc_Theme_Dark
+   * for a dark theme and Doc_Theme_Bright for a light theme. This is used to
+   * determine the state of the pseudoclasses :-moz-lwtheme and :-moz-lwtheme-text.
+   */

The comments shouldn't allow returning Uninitialized; this method
should always return one of the other values.


In nsCSSRuleProcessor.cpp:
+      nsIDocument* doc = data.mContent ? data.mContent->GetDocument() :
+                                         data.mPresContext->Document();

the GetDocument calls on the data.mContent should be GetOwnerDoc()
instead, and I don't think you should bother with getting the document
from the pres context otherwise.

+  // check for the lwtheme and lwthemetextcolor attribute on root XUL elements
+  if ((aData->mAttribute == nsGkAtoms::lwtheme ||
+       aData->mAttribute == nsGkAtoms::lwthemetextcolor) &&
+      aData->mNameSpaceID == kNameSpaceID_XUL &&
+      aData->mContent == aData->mContent->GetOwnerDoc()->GetRootContent())
+  {
+    data.change = nsReStyleHint(data.change | eReStyle_Self);
+  }

This should be merged with the localedir check right above it.

r=dbaron with those things fixed
Attachment #402220 - Flags: superreview?(dbaron)
Attachment #402220 - Flags: review?(dbaron)
Attachment #402220 - Flags: review+
Attached patch patch ver. 1.6Splinter Review
Thanks! I addresses all the comments.
Attachment #402220 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/345a62043ffc
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Thanks Dao.
Flags: in-testsuite+
(In reply to comment #45)
The attributes all of these css selectors depend on are:

lwtheme="true"
lwthemetextcolor=["bright"|"dark"]

Afaik, there isn't any "lightweightthemes" attribute.
(In reply to comment #46)
> The attributes all of these css selectors depend on are:
> 
> lwtheme="true"
> lwthemetextcolor=["bright"|"dark"]

This is an implementation detail and should not be documented.

> Afaik, there isn't any "lightweightthemes" attribute.

There is.

(In reply to comment #45)
> https://developer.mozilla.org/en/XUL/Attribute/lwthemetextcolor

Sheppy, could you please remove this page? I've already removed the references to it.
This bug's checkin introduced this build warning:
{
/mozilla/content/xul/document/src/nsXULDocument.h: In constructor ‘nsXULDocument::nsXULDocument()’:
/mozilla/content/xul/document/src/nsXULDocument.h:485: warning: ‘nsXULDocument::mResolutionPhase’ will be initialized after
/mozilla/content/xul/document/src/nsXULDocument.h:353: warning:   ‘nsIDocument::DocumentTheme nsXULDocument::mDocLWTheme’
/mozilla/content/xul/document/src/nsXULDocument.cpp:224: warning:   when initialized here
}

Attached patch fixes this.
Attachment #406124 - Attachment description: follow-up warning fix → follow-up: reorder initializers in constructor, to fix build warning
Attachment #406124 - Flags: review?(dbaron)
Attachment #406124 - Flags: approval1.9.2?
Attachment #406124 - Flags: approval1.9.2?
Comment on attachment 406124 [details] [diff] [review]
follow-up: reorder initializers in constructor, to fix build warning

r=dbaron
Attachment #406124 - Flags: review?(dbaron) → review+
No longer depends on: 524826
Depends on: 528792
Depends on: 547608
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: