Closed Bug 954561 Opened 10 years ago Closed 10 years ago

Contact list CSS inheritance cleanup

Categories

(Instantbird Graveyard :: Contacts window, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 3 obsolete files)

*** Original post on bio 1128 at 2011-10-30 22:05:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1128 as attmnt 959 at 2011-10-30 22:05:00 UTC ***

As requested.

These changes produce a bug which I couldn't find the cause of: on selecting a contact, the statusIcon overlay disappears.
Attachment #8352701 - Flags: review?(florian)
Comment on attachment 8352701 [details] [diff] [review]
Patch

*** Original change on bio 1128 attmnt 959 at 2011-10-30 22:57:14 UTC ***

First, thanks for deciding to work on this! :)

>diff -u /chrome/instantbird/content/instantbird/buddytooltip.xml ./buddytooltip.xml
>--- /chrome/instantbird/content/instantbird/buddytooltip.xml	2010-01-01 00:00:00.000000000 +0100
>+++ ./buddytooltip.xml	2011-10-30 21:56:47.000000000 +0100
>@@ -52,8 +52,8 @@
>       <xul:vbox flex="1">
>         <xul:hbox crop="end" align="center" flex="1">
>           <xul:stack class="prplTooltipBuddyIcon">
>-            <xul:image xbl:inherits="src=iconPrpl" class="protoIcon"/>
>-            <xul:image class="statusIcon"/>
>+            <xul:image class="protoIcon" xbl:inherits="src=iconPrpl, status"/>

I haven't seen any code in Mozilla using ", " rather than just "," as the separator for inherited attributes, so I'm not sure how well that works (and anyway, I would prefer that we keep the same style as usual).

>diff -u /chrome/instantbird/content/instantbird/conv.xml ./conv.xml

>         <xul:label crop="end" flex="1" mousethrough="always"
>                    anonid="displayname" class="convDisplayName"
>-                   xbl:inherits="value=displayname"/>
>-        <xul:label crop="end" flex="1000000" mousethrough="always"
>+                   xbl:inherits="value=displayname, status"/>
>+        <xul:label crop="end" flex="100000" mousethrough="always"

You are reverting https://hg.instantbird.org/instantbird/rev/b985510492b2 here.

>diff -u /chrome/instantbird/skin/classic/instantbird/blist.css ./blist.css
>--- /chrome/instantbird/skin/classic/instantbird/blist.css  2010-01-01 00:00:00.000000000 +0100
>+++ ./blist.css 2011-10-30 22:52:10.000000000 +0100
>@@ -216,9 +216,12 @@
>   max-height: 16px;
> }
>
>-.protoIcon, .statusIcon,
>-.hideGroupButton, .startChatBubble,
>-.expander-up, .expander-down {
>+.protoIcon,
>+.statusIcon,
>+.hideGroupButton,
>+.startChatBubble,
>+.expander-up,
>+.expander-down {
>   width: 16px;
>   height: 16px;
>   min-height: 16px;

What's the point of this change?

>-:-moz-any(conv, contact)[status="offline"][selected="true"] .protoIcon {
>+.protoIcon[status="offline"][selected],
>+.protoIcon[status="unknown"][selected] {
>   opacity: 0.7;
> }

Have you verified that changing [selected="true"] to [selected] doesn't change the behavior?

>+.contactDisplayName[status="idle"],
>+.buddyDisplayName[status="idle"],
>+.convDisplayName[status="idle"] {
>+  color: GrayText;
>+}
>+

Would you like to add a displayName class to all these 3 elements, to further simplify the selectors?


>-contact[state="fading"] :-moz-any(.contactDisplayName, .buddyDisplayName) {
>+.contactDisplayName[state="fading"],
>+.buddyDisplayName[state="fading"] {
>   color: GrayText;
>   font-style: italic;
> }

This unfortunately changes the behavior, as the state attribute from a contact element can't be inherited inside the buddy element (where .buddyDisplayName is).
So I'm afraid inheriting the state attribute isn't a very good idea :-/.


>diff -u /chrome/instantbird/skin/classic/instantbird/buddytooltip.css ./buddytooltip.css

>-tooltip[status="unknown"] .protoIcon {
>+tooltip.protoIcon[status="unknown"] {
>   opacity: 0.7;
> }

Is there a reason for keeping the tag name here?
Attachment #8352701 - Flags: review?(florian) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1128 as attmnt 960 at 2011-10-31 01:16:00 UTC ***

Thanks for your comments.

>>-.protoIcon, .statusIcon,
>>-.hideGroupButton, .startChatBubble,
>>-.expander-up, .expander-down {
>>+.protoIcon,
>>+.statusIcon,
>>+.hideGroupButton,
>>+.startChatBubble,
>>+.expander-up,
>>+.expander-down {
>>   width: 16px;
>>   height: 16px;
>>   min-height: 16px;
>
>What's the point of this change?

Consistency with the rest of the file; better legibility.

>>-:-moz-any(conv, contact)[status="offline"][selected="true"] .protoIcon {
>>+.protoIcon[status="offline"][selected],
>>+.protoIcon[status="unknown"][selected] {
>>   opacity: 0.7;
>> }
>
>Have you verified that changing [selected="true"] to [selected] doesn't change
>the behavior?

Yes. Actually and unfortunately, any setting here seems to be ignored (I tried e.g. "display: none !important"). 

>Would you like to add a displayName class to all these 3 elements, to further
>simplify the selectors?

Called it aDisplayName as .displayName is asking for confusion with #displayName.

>This unfortunately changes the behavior, as the state attribute from a contact
>element can't be inherited inside the buddy element (where .buddyDisplayName
>is).

Oh right. And the buddy element carries its own state attribute already. 

>>+tooltip.protoIcon[status="unknown"] {
>>   opacity: 0.7;
>> }
>
>Is there a reason for keeping the tag name here?

I was not certain that the buddytooltip.css would, when loaded by the tooltip XBL, only modify that rule for the tooltip. But it seems XBL is that clever.

The problem with the selected elements persists :(
Attachment #8352702 - Flags: review?(florian)
Comment on attachment 8352701 [details] [diff] [review]
Patch

*** Original change on bio 1128 attmnt 959 at 2011-10-31 01:16:53 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352701 - Attachment is obsolete: true
Assignee: nobody → aleth
Status: NEW → ASSIGNED
*** Original post on bio 1128 at 2011-10-31 02:15:47 UTC ***

Hmm, in fact what you said about "state" above might apply similarly for "status" (ie the rules also need to trigger when the parent status is xyz). In which case I don't know if this rewrite is possible.
Status: ASSIGNED → NEW
*** Original post on bio 1128 at 2011-10-31 09:09:12 UTC ***

(In reply to comment #3)
> what you said about "state" above might apply similarly for "status"

Technically it applies too to the status attribute.

> (ie the rules also need to trigger when the parent status is xyz).

I think this was a bug of the previous implementation, and the behavior change is actually desired in this case :).
*** Original post on bio 1128 at 2011-10-31 13:22:56 UTC ***

(In reply to comment #4)
> I think this was a bug of the previous implementation, and the behavior change
> is actually desired in this case :).

Thanks, that's what I wanted to know, since reproducing the previous behaviour exactly seems impossible :)
Comment on attachment 8352702 [details] [diff] [review]
Patch

*** Original change on bio 1128 attmnt 960 at 2011-10-31 14:43:21 UTC ***

>diff -u /chrome/instantbird/skin/classic/instantbird/blist.css ./blist.css

>+.statusIcon[status="away"] ,

No space before that comma please :).

(In reply to comment #2)

> >>-.protoIcon, .statusIcon,
> >>-.hideGroupButton, .startChatBubble,
> >>-.expander-up, .expander-down {
> >>+.protoIcon,
> >>+.statusIcon,
> >>+.hideGroupButton,
> >>+.startChatBubble,
> >>+.expander-up,
> >>+.expander-down {
> >>   width: 16px;
> >>   height: 16px;
> >>   min-height: 16px;
> >
> >What's the point of this change?
> 
> Consistency with the rest of the file; better legibility.

I'm not really convinced, as these pairs somehow had a logic but ok... It doesn't matter much either way.

> >>-:-moz-any(conv, contact)[status="offline"][selected="true"] .protoIcon {
> >>+.protoIcon[status="offline"][selected],
> >>+.protoIcon[status="unknown"][selected] {
> >>   opacity: 0.7;
> >> }
> >
> >Have you verified that changing [selected="true"] to [selected] doesn't change
> >the behavior?
> 
> Yes. Actually and unfortunately, any setting here seems to be ignored (I tried
> e.g. "display: none !important").

Is this the issue you mentioned in comment 0 already? Can't you figure out with DOM inspector which rules get applied?

> >Would you like to add a displayName class to all these 3 elements, to further
> >simplify the selectors?
> 
> Called it aDisplayName as .displayName is asking for confusion with
> #displayName.

What does the "a" mean here? Maybe .blistDisplayName would avoid confusion?
Attachment #8352702 - Flags: review?(florian) → review-
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1128 as attmnt 963 at 2011-10-31 21:24:00 UTC ***

Progress! Selection issue fixed by not overlooking contact.xml#contact-big ;)

>I'm not really convinced, as these pairs somehow had a logic but ok... It
>doesn't matter much either way.

I don't mind either but it's nicer if it's consistent, with e.g. http://lxr.instantbird.org/instantbird/source/instantbird/themes/blist.css#170 and following.

>What does the "a" mean here? Maybe .blistDisplayName would avoid confusion?

The 'a' is the English indefinite article (a convention I have seen used elsewhere for generic types). But I changed it as your suggestion is clearer.
Attachment #8352705 - Flags: review?(florian)
Comment on attachment 8352702 [details] [diff] [review]
Patch

*** Original change on bio 1128 attmnt 960 at 2011-10-31 21:24:29 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352702 - Attachment is obsolete: true
Comment on attachment 8352705 [details] [diff] [review]
Patch

*** Original change on bio 1128 attmnt 963 at 2011-11-09 23:29:09 UTC ***

I like this patch, but unfortunately it breaks the status icon in conversation tab tooltips, as blist.css isn't included in that window.

Possible solution: create a new status.css file (don't forget to add it in jar.mn) and @import it in both blist.css and buddytooltip.css.
Attachment #8352705 - Flags: review?(florian) → review-
Attached patch PatchSplinter Review
*** Original post on bio 1128 as attmnt 985 at 2011-11-10 14:49:00 UTC ***

Didn't realise the same tooltips were used elsewhere - thanks!
Attachment #8352726 - Flags: review?(florian)
Comment on attachment 8352705 [details] [diff] [review]
Patch

*** Original change on bio 1128 attmnt 963 at 2011-11-10 14:49:26 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352705 - Attachment is obsolete: true
*** Original post on bio 1128 at 2011-11-10 15:53:18 UTC ***

Just noticed jar.mn (unlike the rest of IB) uses tabs instead of spaces, sorry.
Blocks: 954506
Comment on attachment 8352726 [details] [diff] [review]
Patch

*** Original change on bio 1128 attmnt 985 at 2011-12-13 20:58:00 UTC ***

Sorry for the delay in getting back to this patch which was already almost finished in its previous iteration.
This looks good except 3 whitespace details I'm going to change before committing:

>diff -u -N /chrome/instantbird/skin/classic/instantbird/status.css ./status.css
>--- /chrome/instantbird/skin/classic/instantbird/status.css	1970-01-01 01:00:00.000000000 +0100
>+++ ./status.css	2011-11-10 15:27:11.000000000 +0100
>@@ -0,0 +1,70 @@
>+/* ***** BEGIN LICENSE BLOCK *****

>+ * ***** END LICENSE BLOCK ***** */
>+
>+
>+.statusIcon[status="away"],

A single empty line would be enough here.

>+}
>+

An empty line at the end of the file is superfluous (the file should finish with one \n, not two).

>--- /instantbird/themes/jar.mn	2011-11-10 15:45:26.000000000 +0100
>+++ /instantbird/themes/jar.mn	2011-11-10 15:46:46.000000000 +0100
>@@ -39,6 +39,7 @@
> *	skin/classic/instantbird/engineManager.css
> *	skin/classic/instantbird/instantbird.css
> 	skin/classic/instantbird/menus.css
>+  skin/classic/instantbird/status.css

As you noted in comment 10, this should be a tab.
Attachment #8352726 - Flags: review?(florian) → review+
*** Original post on bio 1128 at 2011-12-13 23:23:27 UTC ***

https://hg.instantbird.org/instantbird/rev/1a03a3b11095
Severity: normal → trivial
Status: NEW → RESOLVED
Closed: 10 years ago
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → 1.2
Depends on: 954644
You need to log in before you can comment on or make changes to this bug.