Closed
Bug 954561
Opened 10 years ago
Closed 10 years ago
Contact list CSS inheritance cleanup
Categories
(Instantbird Graveyard :: Contacts window, defect)
Instantbird Graveyard
Contacts window
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: aleth, Assigned: aleth)
References
Details
Attachments
(1 file, 3 obsolete files)
11.80 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 1128 at 2011-10-30 22:05:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***
Assignee | ||
Comment 1•10 years ago
|
||
*** 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 2•10 years ago
|
||
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-
Assignee | ||
Comment 3•10 years ago
|
||
*** 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)
Assignee | ||
Comment 4•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•10 years ago
|
||
*** 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
Comment 6•10 years ago
|
||
*** 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 :).
Assignee | ||
Comment 7•10 years ago
|
||
*** 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 8•10 years ago
|
||
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-
Assignee | ||
Comment 9•10 years ago
|
||
*** 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)
Assignee | ||
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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-
Assignee | ||
Comment 12•10 years ago
|
||
*** 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)
Assignee | ||
Comment 13•10 years ago
|
||
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
Assignee | ||
Comment 14•10 years ago
|
||
*** 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.
Comment 15•10 years ago
|
||
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+
Comment 16•10 years ago
|
||
*** 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
You need to log in
before you can comment on or make changes to this bug.
Description
•