Remove <resources> from the common XML files

RESOLVED FIXED in Thunderbird 63.0

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

unspecified
Thunderbird 63.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Bug 1474069 will remove support for XBL <resources>, meaning that all XBL
stylesheets should become regular stylesheets, similarly to the dependencies of
bug 1470830.

This should be the last <resources> removal bug for TB.

Frank-Rainer, I added you to this bud because this needs also changes in suite (add stylesheets imports somewhere to load them without using <resources>).
Posted patch common-resources.patch (obsolete) — Splinter Review
I tested all places I know that use this bindings, like Birthday field in AB card, the toolbars and fields with spinbuttons in the prefs, and naturally the prefs itself.

I moved the stylesheet imports to the bindings.css because this is the place the bindings are loaded and through this the stylesheets also automatically imported. Like this, extensions that use already bindings.css should need to do nothing special.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8990579 - Flags: review?(jorgk)
Comment on attachment 8990579 [details] [diff] [review]
common-resources.patch

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

Look good to me.

::: mail/base/jar.mn
@@ +41,1 @@
>  *   content/messenger/textbox.xml                   (../../common/bindings/textbox.xml)

As far as I can see, textbox.xml doesn't need pre-processing any more. Not your fault.
Attachment #8990579 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+2) from comment #2)
> As far as I can see, textbox.xml doesn't need pre-processing any more. Not
> your fault.

https://dxr.mozilla.org/comm-central/source/common/bindings/textbox.xml#240 shows it's still needed. Although the preprocessor would fail when no preprocessing is needed.
Keywords: checkin-needed
Grrr, I looked for 'ifdef' :-(
Please try the compose window. Looks like this patch breaks the recipient boxes, they have lost their borders.
Flags: needinfo?(richard.marti)
Keywords: checkin-needed
Attachment #8990579 - Flags: review+
This was not an issue of my patch, it came from bug 1473832 and bug 1471544 which landed in m-c.
Attachment #8990579 - Attachment is obsolete: true
Flags: needinfo?(richard.marti)
Attachment #8990757 - Flags: review?(jorgk)
Please explain: Are we porting bug 1473832 and bug 1471544 here? Does the addressing widget not work in a current Daily due to those bugs? Or did the patch need further changes after those bugs landed? But landings of those two bugs pre-date the first version of the patch here. I'm confused.
With this new patch I addressed both bugs where I saw issues. It's not a porting of them, it's a increasing of the specificity of some of our rules after the m-c bug loads the stylesheets as document stylesheet which makes them more specific (for an example where also m-c is affected, see bug 1474193).

There can be now some visual glitches that we haven't encountered until now. Also is m-c on landing more of this stylesheets as document stylesheets what can add more glitches.
Comment on attachment 8990757 [details] [diff] [review]
common-resources.patch

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

::: mail/themes/windows/mail/compose/messengercompose.css
@@ +678,5 @@
>      border-color: #a9b7c9 !important;
>    }
>  
> +  menulist,
> +  menulist[disabled="true"] {

Please teach me some CSS. Doesn't 'menulist' include all possible attribute values, so why is the ' menulist[disabled="true"]' necessary?
menulist is the base selector and menulist[disabled="true"] is for the disabled menulists. The menulist.css has this menulist[disabled="true"] which is now loaded as document stylesheet like our stylesheets and this makes the menulist.css selector more specific than our selector with only "menulist". Adding menulist[disabled="true"] to our stylesheet makes our selector same specific but because our stylesheet is loaded later, our rules are used.

menulist has the base specificity
menulist[disabled="true"] is something like specificity + 1
menulist[disabled="true"]:hover is something like specificity  + 2
etc.

menulist is a element with base specificity 
.something is a class with specificity + 1
#something is a ID with specificity  + 2

Combinations can be augment the specificity. This are only simple examples, adding multiple attributes augment the specificity too.
I don't think I follow. In M-C there is a definition got menulist, followed by menulist[disabled="true"]:
toolkit/themes/windows/global/menulist.css

Our style sheet loads later and and refines all of menulist, no? So why do we need the menulist[disabled="true"]?
I took it out
-  menulist,
-  menulist[disabled="true"] {
+  menulist {
and it still works, well, in the addressing widget.

What am I missing?
With default theme it works. Try the dark theme, then you see what looks not so good in HTML composer window.
I see no difference with the dark theme.
Posted image compose-menulist.png
This is how today dark Daily looks with the disabled menulists. They should be dark and not white.
Comment on attachment 8990757 [details] [diff] [review]
common-resources.patch

I'm sorry, you are right. Thanks for the patience :-)
Attachment #8990757 - Flags: review?(jorgk) → review+
No problem, there is going much in parallel which is interfering.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/5205ff465bd3cdc64681e39728281bc4d3fb09d5
Remove <resources> from XML files in common/. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 63.0
You need to log in before you can comment on or make changes to this bug.