747415 - Make our css more efficient.

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Blake Winton (:bwinton) (:☕️)]

While showing mconley the Efficient CSS page in MDN, I thought of a quick regex we could run to find places where we qualify ID rules with tag names or classes (which is the first thing listed as inefficient).

The command I ran was:
ack -ai "^[^\"\/]*[a-z0-9]#" -G "\.css" --ignore-dir=mozilla --ignore-dir=suite

The output it gave was:
mail/themes/gnomestripe/editor/EditorDialog.css
162:#alignTypeSelect,label#alignLabel {

mail/themes/gnomestripe/mail/mailWindow1.css
459:tabpanels#tabpanelcontainer {

mail/themes/pinstripe/editor/EditorDialog.css
162:#alignTypeSelect,label#alignLabel {

mail/themes/qute/editor/EditorDialog.css
162:#alignTypeSelect,label#alignLabel {

mail/themes/qute/mail/mailWindow1-aero.css
264:  #messengerWindow[sizemode=normal] tabpanels#tabpanelcontainer {

mail/themes/qute/mail/mailWindow1.css
417:tabpanels#tabpanelcontainer {

These all seem like things we may want to fix, but I haven't done any digging yet.

('ack -ai "^[^\"\/]*[a-z0-9]\..*{" -G "\.css" --ignore-dir=mozilla --ignore-dir=suite' is another interesting command we might want to run…)

Comment 1

[Richard Marti (:Paenglab)]

Attachment: Make it efficient

Changed the slow selectors.
I removed the label#alignLabel entirely because it isn't used. The #alignLabel isn't a label, it's a caption.

I've found in EditorDialog.css a tree.list and leaved it because I wasn't sure if it is still used. If you can say me it^s no more used, I remove this also.

Your second regex didn't work well in mxr. Where where too many hits to use it ( every 0.5 etc. was shown). If you can run this on your machine and post the result here, I'll update the patch.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 617225 [details] [diff] [review]
Make it efficient

The results for the second regex are at http://pastebin.mozilla.org/1591549  They're trying to find the cases that violate "Don’t qualify class rules with tag names", but it's not exact.  They'll also require a little more interpretation than just simple replacement, since I think there are a few places that we can't fix without some larger changes.

http://pastebin.mozilla.org/1591594 contains most of the obvious places that we use the descendant or child selector, if you felt like fixing those, too.  ;)

Now on to the review:

>+++ b/mail/themes/gnomestripe/editor/EditorDialog.css
>@@ -159,7 +159,7 @@
>   min-height:     50px;
> }
> 
>-#alignTypeSelect,label#alignLabel {
>+#alignTypeSelect {

It looks like we're missing the "#alignLabel" here now.  Was it unused?

>+++ b/mail/themes/pinstripe/editor/EditorDialog.css
>@@ -159,7 +159,7 @@
>-#alignTypeSelect,label#alignLabel {
>+#alignTypeSelect {

Ditto.

>+++ b/mail/themes/qute/editor/EditorDialog.css
>@@ -159,7 +159,7 @@
>-#alignTypeSelect,label#alignLabel {
>+#alignTypeSelect {

Ditto.

Aside from that, awesome, and thank you!  :)  r=me with those fixed.

Later,
Blake.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Oh, I completely failed at reading your comment.  Nevermind about fixing those, although adding the rule for #alignLabel is probably what the original author intended.  :)

Thanks,
Blake.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: Make it efficient v2

Patch expanded to classes. Reference: http://pastebin.mozilla.org/1591549 (no more online).

- I haven't touched testpilot. I can't check the results because I've never seen a survey :(
- I leaved also everything in test. I think it's better it works like it is than I optimize it to not correctly working tests. It isn't used at the end user install and doesn't harm him.

Comment 5

[Richard Marti (:Paenglab)]

I add Florian and Philipp to the CC list to inform them this patch changes also CSS in IM and Calendar.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 619384 [details] [diff] [review]
Make it efficient v2

>+++ b/calendar/base/themes/common/calendar-views.css

I'm not a calendar reviewer, so I'm going to leave this for them.

>+++ b/mail/base/content/featureConfigurators/subpage.css
>@@ -28,9 +28,7 @@
>-.column.right {
>+.right {

So, this isn't really doing what you think it is, but I think it works out fine in this case, because we don't use a class of right anywhere else in the featureConfigurator pages, but I don't think we want to change it.

>+++ b/mail/base/content/glodaFacetView.css
>@@ -124,7 +124,7 @@
>-.popup-menuitem.top {
>+.popup-menu .top {

I think this is actually the opposite of what the recommendation is.  Specifically, you're adding a descendant selector, when the suggestion is "Avoid the descendant selector"!  So we should probably leave all of these the way they were.

>@@ -810,19 +810,22 @@
>-span.loading, span.empty {
>+#showLoading > .loading,
>+#showEmpty > .empty {

Same here.  You're adding a child selector, and while that's better than a descendant selector, it's still not as good as the previous rule.

>+++ b/mail/components/im/messages/main.css
>@@ -69,24 +69,24 @@
>-div.bubble>div.indicator {
>+.bubble > .indicator {

The only .indicator elements seem to be in .bubble, so you can probably make these:
.indicator {
instead.

>-div.bubble p.pseudo {
>+.bubble .pseudo {

Ditto.

>-div.bubble p.pseudo>span>span.time {
>+.bubble .pseudo > span > .time {

I haven't checked this one, but I bet you can reduce it to ".time {".

>+++ b/mail/components/im/themes/imAccountWizard.css
>@@ -44,21 +44,21 @@
>-groupbox.collapsable {
>+.collapsable {
>   -moz-user-focus: normal;
> }
> 
> %ifdef XP_WIN
>-groupbox.collapsable .caption-text {
>+.collapsable .caption-text {

I could only find one thing with a class of "caption-text"…

>@@ -68,7 +68,7 @@
>-groupbox.collapsable caption .caption-icon {
>+.collapsable caption .caption-icon {

Ditto.

>+++ b/mail/components/im/themes/imMenulist.css
>@@ -37,10 +37,10 @@
> /* Fix the icons in the menulist by undoing things from menu.css */
>-menulist > menupopup > menuitem.menuitem-iconic > .menu-iconic-left {
>+menulist > menupopup > .menuitem-iconic > .menu-iconic-left {

I wonder if we can just use .menu-iconic-left here?

>+++ b/mail/themes/gnomestripe/editor/EditorDialog.css
>@@ -159,7 +159,7 @@
>   min-height:     50px;
> }
> 
>-#alignTypeSelect,label#alignLabel {
>+#alignTypeSelect {

Is the #alignLabel really not needed?

>+++ b/mail/themes/gnomestripe/mail/smileys.css
>@@ -46,124 +46,122 @@
> /* ::::: we also represent smilies inside of spans ::::: */
>-span.moz-smiley-s1,
>+.moz-smiley-s1,

If we're going to change the selector here, we should probably change the comment.

Other than that it looked good, but based on those problems, I'm going to have to say r-.

Thanks,
Blake.

Comment 7

[Florian Quèze [:florian]]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #6)

> >+++ b/mail/components/im/themes/imMenulist.css
> >@@ -37,10 +37,10 @@
> > /* Fix the icons in the menulist by undoing things from menu.css */
> >-menulist > menupopup > menuitem.menuitem-iconic > .menu-iconic-left {
> >+menulist > menupopup > .menuitem-iconic > .menu-iconic-left {
> 
> I wonder if we can just use .menu-iconic-left here?

The purpose of this rule is to undo the display: none when the menuitem has the class .menuitem-iconic, not to force the icons to be visible on all menuitems.

So I think we need at least this: .menuitem-iconic > .menu-iconic-left

But for code clarity, it may be better to keep a selector that is as specific as the one it overrides; the original selector is:
http://hg.mozilla.org/mozilla-central/annotate/21da3f655b30/toolkit/themes/gnomestripe/global/menu.css#l188
http://hg.mozilla.org/mozilla-central/annotate/21da3f655b30/toolkit/themes/winstripe/global/menu.css#l223

Comment 8

[Richard Marti (:Paenglab)]

Attachment: Make it efficient v3

I've added Philipp to r? for the calendar part.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #6)
> Comment on attachment 619384 [details] [diff] [review]
> Make it efficient v2
> 
> >+++ b/mail/base/content/glodaFacetView.css

I'm leaving glodaFacetView.css as it is.

> >+++ b/mail/components/im/messages/main.css
> >@@ -69,24 +69,24 @@
> >-div.bubble>div.indicator {
> >+.bubble > .indicator {
> 
> >-div.bubble p.pseudo {
> >+.bubble .pseudo {
> 
> >-div.bubble p.pseudo>span>span.time {
> >+.bubble .pseudo > span > .time {

Shortened to single class selector and is still working.

> >+++ b/mail/components/im/themes/imAccountWizard.css
> >@@ -44,21 +44,21 @@
> >-groupbox.collapsable {
> >+.collapsable {
> 
> > %ifdef XP_WIN
> >-groupbox.collapsable .caption-text {
> >+.collapsable .caption-text {
>
> >-groupbox.collapsable caption .caption-icon {
> >+.collapsable caption .caption-icon {

Shortened

> >+++ b/mail/components/im/themes/imMenulist.css
> >@@ -37,10 +37,10 @@
> > /* Fix the icons in the menulist by undoing things from menu.css */
> >-menulist > menupopup > menuitem.menuitem-iconic > .menu-iconic-left {
> >+menulist > menupopup > .menuitem-iconic > .menu-iconic-left {
> 
> I wonder if we can just use .menu-iconic-left here?

Leave it this specific to override the global selector.

> >+++ b/mail/themes/gnomestripe/editor/EditorDialog.css
> >@@ -159,7 +159,7 @@
> >   min-height:     50px;
> > }
> > 
> >-#alignTypeSelect,label#alignLabel {
> >+#alignTypeSelect {
> 
> Is the #alignLabel really not needed?

Yes, also when it's name is #alignLabel it's a caption and not a label :). An I would say it's better to use the global caption margin of 6px instead this 5px which ends in one slightly misaligned of three captions in this dialog.

> >+++ b/mail/themes/gnomestripe/mail/smileys.css
> >@@ -46,124 +46,122 @@
> > /* ::::: we also represent smilies inside of spans ::::: */
> >-span.moz-smiley-s1,
> >+.moz-smiley-s1,

Done, is this text okay?

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 619981 [details] [diff] [review]
Make it efficient v3

>+++ b/mail/themes/qute/mail/quickFilterBar-aero.css
>@@ -209,7 +209,7 @@
>-#quick-filter-bar-collapsible-buttons toolbarbutton label.toolbarbutton-text {
>+#quick-filter-bar-collapsible-buttons toolbarbutton .toolbarbutton-text {

Can we switch this to "#quick-filter-bar-collapsible-buttons > toolbarbutton > .toolbarbutton-text"?

>+++ b/mail/themes/qute/mail/smileys.css
>@@ -44,126 +44,124 @@
>-/* ::::: we also represent smilies inside of spans ::::: */
>+/* ::::: we represent smilies instead of text in spans ::::: */
> 
>-span.moz-smiley-s1,
>+.moz-smiley-s1,

Sorry, I wasn't really clear there.

It looks to me like we're representing smilies everywhere now, not just inside of spans, so I thought we might want to change that to "we also represent smilies inside of elements", or "we also represent smilies".


Other than that, I like it, so r=me.

And as a side note, if you're going to do more css cleanup (which I really do appreciate you doing!), _please_ do it as a new patch, cause reviewing this much css again and again makes my head hurt.  ;)

Thanks,
Blake.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 619981 [details] [diff] [review]
Make it efficient v3

The only thing that makes me uneasy here is that some selectors are turning out to be more general. What if an extension or other part of the app uses a similar class name? For example, IM uses .event, which is - at least judging by the name - a likely class name for calendar things too. This might be a more general problem though.

If you want me to give a thorough review/testing on this, then you'll have to wait until I'm back. Otherwise r=philipp to push things forward, assuming you have tested this with Lightning.

Comment 11

[Florian Quèze [:florian]]

(In reply to Philipp Kewisch [:Fallen] from comment #10)
> For example, IM uses .event, which is - at least judging
> by the name - a likely class name for calendar things too.

If it can make you feel better for that specific example, I can assure you that mail/components/im/messages/main.css is going to be used only for the HTML document in which we display chat messages, it's not going to be applied to any XUL document.

I agree that for css files included more broadly, it would be easy to cause regressions with such seemingly trivial changes though.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/deae93f5e12e

Comment 13

[Philip Chee]

>>>+++ b/mail/components/im/themes/imMenulist.css
>>>@@ -37,10 +37,10 @@
>>> /* Fix the icons in the menulist by undoing things from menu.css */
>>>-menulist > menupopup > menuitem.menuitem-iconic > .menu-iconic-left {
>>>+menulist > menupopup > .menuitem-iconic > .menu-iconic-left {
>> 
>> I wonder if we can just use .menu-iconic-left here?
> 
> The purpose of this rule is to undo the display: none when the menuitem has the class
> .menuitem-iconic, not to force the icons to be visible on all menuitems.

Can't you use the .menuitem-with-favicon class which exists for this purpose?

Comment 14

[Florian Quèze [:florian]]

(In reply to Philip Chee from comment #13)

> > The purpose of this rule is to undo the display: none when the menuitem has the class
> > .menuitem-iconic, not to force the icons to be visible on all menuitems.
> 
> Can't you use the .menuitem-with-favicon class which exists for this purpose?

This class seems to exist only ifdef MOZ_WIDGET_GTK2 (the only reference I've found in a CSS file is http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/xul.css#373).