Closed Bug 1497041 Opened 6 years ago Closed 5 years ago

After editing the tag colour the treeitem could be not coloured

Categories

(Thunderbird :: Message Reader UI, defect)

Product:
Thunderbird
Component:
Message Reader UI
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird66 fixed, thunderbird67 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 67.0
Tracking Flags:
Tracking Status
thunderbird66 --- fixed
thunderbird67 --- fixed

People

(Reporter: Paenglab, Assigned: jorgk-bmo)

References

Details

(Keywords: regression)

Attachments

(3 files, 28 obsolete files)

1497041-tag-colors.patch [landed in comment #50]
19.77 KB, patch
jorgk-bmo
: review+
jorgk-bmo
: approval-comm-beta+
Details | Diff | Splinter Review
1497041-registry-import.patch [landed in comment #52, bustage-fix]
1.14 KB, patch
Details | Diff | Splinter Review
1497041-dynamic-css.patch (v6d)
135.02 KB, patch
aceman
: review+
darktrojan
: review+
Details | Diff | Splinter Review 
Since bug 1495808 it can happen that the treeitem and the tag button in message header isn't coloured.

This comes because with the new picker every colour can be chosen. In tagColors.css there are around 1000 colours defined and they match seldom to the now 16.7 million colours that can now be chosen.

Maybe we need now to define the styles dynamically on the treeitems and the tag buttons.

STR: Coose a colour in the new tag colour picker. Already the predefined colours in the Windows colour picker shows the issue when you then apply this tag on a message (best is only add this tag to not get the colour from a previous tag).
tagColors.css is madness ;-(
It is. If we could do this through JS or C++ it would be a lot better. Because now we'd need 16.7 million rules.
Attached patch 1497041-tag-colors.patch (obsolete) — — Splinter Review 
This fixes all JS parts like tag in the headers, in QFB and menus.

To test this, choose a new colour with the color picker. In almost every case you pick a colour which doesn't exist in tagColors.css. You see directly if it is such a colour when the treeitem in the threadPane doesn't get this colour. But in the tags in the message header you see the tag colour (with my patch applied).

If you find (what surely will happen) things which could be made nicer and smarter, please show it me.

What is missing, is to do the same to the treeitems in the threadPane. But this is c++ and I don't know how to do this. The change must be in nsMsgDBView.cpp. Please can you help dong this?
Assignee: nobody → richard.marti
Attachment #9020532 - Flags: feedback?(jorgk)
Attachment #9020532 - Flags: feedback?(acelists)
OK, this is all in nsMsgDBView::AppendKeywordProperties(). There it sets lc-black and lc-white.

I also takes the colour and prepends "lc-" (with a tricky replace):
color.Replace(0, 1, NS_LITERAL_CSTRING(LABEL_COLOR_STRING));
Note tat LABEL_COLOR_STRING is "lc-".

But the properties are a collection of class names, right? Like "read", "unread", "new", etc. I don't know how I can set a style for the row now. The interface is:

NS_IMETHODIMP
nsMsgDBView::GetRowProperties(int32_t index,
                              nsAString& properties)

NS_IMETHODIMP
nsMsgDBView::GetCellProperties(int32_t aRow,
                               nsTreeColumn *col,
                               nsAString& properties)

That's from
https://searchfox.org/comm-central/rev/146574df2b351a5c09676aceadb613e2f90c0112/mozilla/layout/xul/tree/nsITreeView.idl#34

So what property should be returned?
A class doesn't work because we then would set 16.4 Million selectors for every colour. It should be like in my patch for the tags in the menus and the message header tags a style like "style: --tag-color: #ffffff; --tag-contrast-color: black;". The --tag-contrast-color should be checked if the contrast is big enough with black as colour, if not use white. In my patch I used a function I copied from Windows8WindowFrameColor.jsm. The same should be done here.

I plan to use CSS variables instead of color and background-color. Like this I can exchange the two colours in CSS when the row is selected.
Comment on attachment 9020532 [details] [diff] [review]
1497041-tag-colors.patch

Maybe I wasn't clear enough. The nsITreeView.idl only provides so-called properties to the tree rows/cells. So the idea with setting a style won't work. There is no interface for it (unless I'm mistaken).

And as you rightly pointed out, 16.7 million properties don't work either.

So we should invest some work to restrict the tag colour chooser to return the "basic" HTML colours it returned before.
Attachment #9020532 - Flags: feedback?(jorgk)
The only thing we could do is set the actual tag name as a property, like tag-Work or so. Then generate some "dynamic" CSS to style based on that property/class. I don't know whether that can be done, perhaps with CSS variables which would have to be populated with the tag properties.

Can you do a selector with a variable? And how do you set CSS variables?

Like
.tag-class:([--tag1-name]) {
  background-color: --tag1-bcolor;
  color: --tag1-color;
}
repeat for 50(?) tags or so.
(In reply to Jorg K (GMT+2) from comment #7)
> The only thing we could do is set the actual tag name as a property, like
> tag-Work or so. Then generate some "dynamic" CSS to style based on that
> property/class. I don't know whether that can be done, perhaps with CSS
> variables which would have to be populated with the tag properties.
> 
> Can you do a selector with a variable? And how do you set CSS variables?
> 
> Like
> .tag-class:([--tag1-name]) {
>   background-color: --tag1-bcolor;
>   color: --tag1-color;
> }
> repeat for 50(?) tags or so.

"dynamic" CSS sounds as it could work. But unfortunately not with a CSS variable as name. The script should create for every tag two rules:

treechildren::-moz-tree-cell-text(tagName) {
  background-color: theTagBGColor;
  color: theTagTextColor;
}

treechildren::-moz-tree-cell-text(tagName, selected, focus) {
  background-color: theTagTextColor;
  color: theTagBGColor;
}

Normally there shouldn't be too much tags that consume much time to create the dynamic CSS.
Actually, the property we set could the the tag number, not the name, so we set tag1, tag2, tag3, etc. The CSS could then be:

treechildren::-moz-tree-cell-text(tag1) {
  background-color: --tag1-BGcolor;
  color: --tag1-color;
}
repeated from 1-50 or so. No?

We leave the lc-black and lc-white in the C++ code and just change this for now:
    color.Replace(0, 1, NS_LITERAL_CSTRING(LABEL_COLOR_STRING));
    properties.AppendASCII(color.get());
+   properties.Append(' ');
+   properties.Append(topKey);

I'd have to check what the value of topKey is, that might need some tweaking.
Yes, it doesn't matter it its a tag name or a tag number, when this works. When I create a tag with the name "test" in the prefs it is named mailnews.tags.test.color and mailnews.tags.test.tag and there is no number.
Attached patch cpp-experiment.patch (obsolete) — — Splinter Review 
OK, this is what we can to do in the C++ code. The resulting properties I saw were:
|none read offline lc-white $label1|
if the row was tagged with "1" (Important).

|none read offline lc-white huhu|
if the row was tagged with custom tag "huhu".

I don't know whether CSS can cope with "$label1" or whatever crazy tag the user may define, like §ÄøΔ, so we might have to escape the string.

More importantly we need to think about how to create this CSS dynamically based on the tags which are defined, so:
treechildren::-moz-tree-cell-text(huhu) {
  ...
Version: unspecified → 64
Attached patch 1497041-tag-colors.patch (obsolete) — — Splinter Review 
Patch updated to tip.

Maybe we could land this to show all tags correctly on all places except the tree.
Attachment #9020532 - Attachment is obsolete: true
Attachment #9020532 - Flags: feedback?(acelists)
Attachment #9034610 - Flags: feedback?(acelists)
So you workaround it with using style instead of class and setting a color directly.

But the message list (tree) still uses classes (tree cell properties), for which we need to generate the treechildren::-moz-tree-cell-text(tagName) css rules? Surely we can do that:
treechildren::-moz-tree-cell-text(encode(tag1)) {
  background-color: rgb(r,g,b); // from the mailnews.tags.tag1.color
  color: rgb(r,g,b);
}

But the rules need to be generated into an already loaded stylesheet. But where and how do we make it apply in all dialogs?
Could we generate the css into the user's profile and load it from there (changing the url to tagColors.css in our documents)? Why is there tagColors.css separate for each platform? Are there any differences?
(In reply to :aceman from comment #16)
> So you workaround it with using style instead of class and setting a color
> directly.

Yes

> But the message list (tree) still uses classes (tree cell properties), for
> which we need to generate the treechildren::-moz-tree-cell-text(tagName) css
> rules? Surely we can do that:
> treechildren::-moz-tree-cell-text(encode(tag1)) {
>   background-color: rgb(r,g,b); // from the mailnews.tags.tag1.color
>   color: rgb(r,g,b);
> }

That would be the best, then only the needed tags have styles. The only problem would be with the automatically generated feed tags when there are tousends of them.

> But the rules need to be generated into an already loaded stylesheet. But
> where and how do we make it apply in all dialogs?

Maybe through a override of tagColors.css? Or maybe through adding the stylesheet through JS? 

> Could we generate the css into the user's profile and load it from there
> (changing the url to tagColors.css in our documents)?

The user's profile would make sense.

> Why is there tagColors.css separate for each platform? Are there any
> differences?

Linux and Mac are the same. Windows is special because of the Explorer treechildren appearance. Actually are three different styles: for Classic and High Contrast the same as Linux and Mac, for Win8 and 10 a flat linear-gradient and for Win7 a real gradient.
(In reply to Jorg K (GMT+1) from comment #6)
> So we should invest some work to restrict the tag colour chooser to return
> the "basic" HTML colours it returned before.

Yes, we could drop the new color picker and just generate the 10x7 grid color chooser ourselves.
Should I whip up a patch?
(In reply to Richard Marti (:Paenglab) from comment #17)
> > But the rules need to be generated into an already loaded stylesheet. But
> > where and how do we make it apply in all dialogs?
> 
> Maybe through a override of tagColors.css? Or maybe through adding the
> stylesheet through JS? 

Maybe, but we would need to know into which documents to inject the stylesheet.
See https://searchfox.org/comm-central/search?q=tagColors.css into which different css files the tagColors.css is imported.
(In reply to :aceman from comment #18)
> Should I whip up a patch?
Well, I wouldn't mind to see all the mess in tagColors.css go.
(In reply to :aceman from comment #19)
> (In reply to Richard Marti (:Paenglab) from comment #17)
> > > But the rules need to be generated into an already loaded stylesheet. But
> > > where and how do we make it apply in all dialogs?
> > 
> > Maybe through a override of tagColors.css? Or maybe through adding the
> > stylesheet through JS? 
> 
> Maybe, but we would need to know into which documents to inject the
> stylesheet.
> See https://searchfox.org/comm-central/search?q=tagColors.css into which
> different css files the tagColors.css is imported.

If we still use my patch to directly apply the colours, we would need the stylesheet only for the main window with it's tree and the search window tree. If not, it would be the main window with the tree, the tag toolbarbutton, the message header, the quickfilter bar and the multimessage view. Then also the gloda results tab and the search window.
(In reply to Jorg K (GMT+1) from comment #20)
> (In reply to :aceman from comment #18)
> > Should I whip up a patch?
> Well, I wouldn't mind to see all the mess in tagColors.css go.

It would actually make tagColors.css needed again, but paenglab's patch wouldn't be needed.
Attached patch 1497041-tag-colors.patch (obsolete) — — Splinter Review

Rebased after import changes and some de-XBL in Gloda. Something still seems to be working, Richard, can you give it a quick whirl. I think we need to make a start here.

First step, find a home for function checkIsColorContrastEnough() which is added twice in the patch.

Geoff, you've seen more JS code in TB than anyone else. What would be a good spot? Or maybe there's more code JS needed to handle the creation of dynamic CSS and once we have that code, we move it onto a JS file.

Flags: needinfo?(geoff)
Attachment #9034610 - Attachment is obsolete: true
Attachment #9034610 - Flags: feedback?(acelists)

Let's assume we land the first part, can we please remove references to tagColors.css as per comment #21.

Keywords: leave-open

Do we have a solution for styling the tree?

Not yet, but thinking about it. Something with a dynamic style sheet. Let's fix this by stepwise approximation, that means that we get rid of all the unnecessary references to tagColors.css as well so we know where the new dynamic style sheet is needed exactly.

It should be needed for the two trees: in tread pane tree and the message search tree.

(In reply to Jorg K (GMT+1) from comment #26)

Not yet, but thinking about it. Something with a dynamic style sheet. Let's fix this by stepwise approximation, that means that we get rid of all the unnecessary references to tagColors.css as well so we know where the new dynamic style sheet is needed exactly.

But if we do not find a solution for the trees, we may need the other approach keeping the tagColors.css and restoring a limited color picker (albeit open-coded) and then anything landed 'stepwise' before it may be unneeded.

I'll risk a backout on this one ;-) - That said, part 1 will work, even if we restrict to a limited set of colours later. Furthermore, tagColors.css is on my personal hit list, and I'd prefer not to return to the stone-age limited set that we kicked out everywhere else.

Attached patch 1497041-tag-colors.patch (obsolete) — — Splinter Review

Updated the patch with removing the links to tagColors.css where it's no more needed.

I also moved the checkIsColorContrastEnough function from msgMail3PaneWindow.js to mailCore.js to make it work in the stand alone window too.

Attachment #9046739 - Attachment is obsolete: true
Attached patch 1497041-tag-colors.patch (obsolete) — — Splinter Review

Oops, forgot to change msgMail3PaneWindow.js to mailCore.js in multimessageview.xhtml.

Attachment #9046836 - Attachment is obsolete: true
Comment on attachment 9046845 [details] [diff] [review]
1497041-tag-colors.patch

Let's go with this for now. We'll find a better home for `checkIsColorContrastEnough()` later.
Attachment #9046845 - Flags: review+

For the record, as pointed out by Geoff, here's the Calendar answer to dynamic CSS:
https://searchfox.org/comm-central/rev/d2ace128d8a14a3da920dfc253b23f31c8952f00/calendar/base/content/calendar-views.js#482

We should bash this into place for the tags which are really no different as far as the colour is concerned.

I think the best place for getting the contrasting colour might be in nsIMsgTagService. You might also be interested in this less complicated version.

Flags: needinfo?(geoff)

Hmm, the calculation is taken from M-C:
https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/browser/modules/Windows8WindowFrameColor.jsm#30

Putting this in nsIMsgTagService.idl would put the implementation where exactly? nsMsgTagService.cpp :-(

Why's that bad?

It would make the contrasting colour available everywhere the tag colour is, for example I could update the WebExtension API to list both colours and then extensions don't need to implement their own version.

I don't know about bad, but certainly a little trick to do since it calls isContrastRatioAcceptable() from toolkit/modules/Color.jsm.

I think a JS implementation would be preferable here, that can then also do the dynamic CSS tricks we need.

Then I think it probably should be in a JSM in mail/base/modules. I was going to suggest utilityOverlay.js, but the Gloda view doesn't load that script.

I was thinking about TagColor.jsm.

Actually, we already have Windows8WindowFrameColor.jsm with most of the code in question in it. So it's just a matter of refactoring that.

Attached patch 1497041-tag-colors.patch (obsolete) — — Splinter Review

This passed linting and seems to work, I've only checked the multi-message view.

Attachment #9046845 - Attachment is obsolete: true
Attachment #9046981 - Flags: feedback?(richard.marti)
Comment on attachment 9046981 [details] [diff] [review]
1497041-tag-colors.patch

Works also on stand alone Window and gloda.
Attachment #9046981 - Flags: feedback?(richard.marti) → feedback+

Thanks. We use let windowFrameColor = new Color(Windows8WindowFrameColor.get()); in msgMail3PaneWindow.js. That code also still works? It's only for Windows 8 :-(

It doesn't work on Windows 8. The text is always white :-(

What doesn't work on Windows 8? The new code for the tags or the previous code for whatever purpose we have it there.

Attached patch 1497041-tag-colors.patch (obsolete) — — Splinter Review

msgMail3PaneWindow.js needed a small change to let Windows8WindowFrameColor work on Windows 8.

Attachment #9046981 - Attachment is obsolete: true

Another iteration simplifying the code in Windows8WindowFrameColor.jsm. It actually works better when using 217 of the frame base colour. "Better" means that it will switch the text/label to white "earlier", so not only for very dark backgrounds.

Attachment #9047162 - Attachment is obsolete: true
Attachment #9047162 - Flags: review+
Attachment #9047162 - Flags: review+
Attachment #9047215 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7a97f304ef19
Set the tag color directly instead of using tagColors.css. r=jorgk DONTBUILD

Hmm, the hacky import of that Window8 thing everywhere caused big bustage on non-Windows platforms :-( - Here's a quick fix. We should really split that JSM.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c862e0f0a0a6
Follow-up: Import WindowsRegistry.jsm only on Windows. rs=bustage-fix

Won't eslint warn that WindowsRegistry may not be declared in some cases (non-Window), but it is referenced later in the file (but maybe actually not really called on non-Windows)?

It doesn't.

Attached patch 1497041-dynamic-css.patch (obsolete) — — Splinter Review

I'm making a start here. This code will only run when adding a tag. Of course it needs to be run when editing a tag and when starting the application.

First problem I encountered: How do I get to the document whose stylesheet I need to modify.

Richard, any ideas? I'll continue after a break.

Attachment #9021545 - Attachment is obsolete: true

You would do this in an onload for a window:

// Register the style sheet to force styling.
let styleSheet = "chrome://totalmessage/skin/tmcompose.css",
let styleSheetURI = Services.io.newURI(styleSheet, null, null);
let ssSvc = Cc["@mozilla.org/content/style-sheet-service;1"]
              .getService(Ci.nsIStyleSheetService);
if (!ssSvc.sheetRegistered(styleSheetURI, ssSvc.AGENT_SHEET))
  ssSvc.loadAndRegisterSheet(styleSheetURI, ssSvc.AGENT_SHEET);

This should also be done in Bug 1490431.

Comment on attachment 9047969 [details] [diff] [review]
1497041-dynamic-css.patch

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

No real ideas for your questions. Only nits for the rules.

::: mail/components/preferences/display.js
@@ +303,5 @@
>  
> +function addTagtoSheet(aName, aColor, aSheet) {
> +  // Add rule to sheet (fom calendar-views.js - updateStyleSheetForViews()).
> +  let ruleString = "treechildren::-moz-tree-row(" + aName +
> +                   ", selected, focus) { background-color: #" + aColor + " !important;";

Bracket missing
 ", selected, focus) { background-color: #" + aColor + " !important; }";

There should also be a "color" rule for either black or white to get the correct contrast to the background colour.

There is also a rule missing for unselected treechildren like:

  ruleString += "treechildren::-moz-tree-row(" + aName + ") { 
                    color: #" + aColor + "; }";

or better add it before the selected rules.

Thanks for the comment. So instead of referencing tagColors.css the "usual" way, you would force it into all documents using the code you suggested, also see here:
https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/layout/base/nsIStyleSheetService.idl#39

Isn't this a bit drastic? tagColors.css is currently included in mailWindow1.css and searchDialog.css, the former being loaded into messenger.xul.

As for bug 1490431. That's long done. If you're not happy with the solution, please file (and fix?) a follow-up ;-)

(In reply to Richard Marti (:Paenglab) from comment #57)

No real ideas for your questions. Only nits for the rules.

Right. I was going to get something working and then pass it onto you. I can see that we currently have three rules like:

treechildren::-moz-tree-cell-text(lc-330033), .lc-330033:not([_moz-menuactive]) {
  color: #330033
}
treechildren::-moz-tree-row(lc-330033, selected, focus), .blc-330033 {
  background-color: #330033 !important;
}
.blc-330033 {
  border-color: #330033;
}

Surely where one rule can be added, three can be added. Also, we would need to scrap most (or all) of the current tagColors.css.

Do you plan to let the checked-in patches in the tree? If yes, then the .lc-* and .blc-* selectors are no more needed.

Yes, what is landed will stay, modulo the hacky Win8 stuff which I will rename/refactor. I got the rule added, stand by for a working version ;-)

Attached patch 1497041-dynamic-css.patch (v2) (obsolete) — — Splinter Review

Over to you, Richard. When adding tag "jiji" this rule is added:

treechildren::-moz-tree-row(jiji, selected, focus) { background-color: #a43585 !important; }

Wow, and a row with that tag will show that colour. Success!! Obviously it won't survive a restart yet.

You can mess with the the rules now, reduce tagColors.css to what is needed, I suggest to take out the "fixed" colours, and when you're done I'll see where to best put the JS code.

Deal?

Attachment #9047969 - Attachment is obsolete: true

As per comment #11, we will need to escape the tag name string. What identifiers can be used in CSS? Would percent encoding be OK? Or some other encoding? Base64? Surely Japanese users might use テスト.

Will you have tag name changes, or even 2 tags swapping names, while documents already referencing those tag colors are displayed and can't be recreated?

Maybe the tag index (position in the list) or similar could be used as the for the css rules, like "tag1".

Comment on attachment 9047975 [details] [diff] [review]
1497041-dynamic-css.patch (v2)

I see no coloured treechildren with this patch. 

Escaping should not be needed when you use UTF-8 encoding. Only ".", "#" or space needs an escaping with the "\" like \.hello\ you.

Sounds like you want to take over. I merely proved the point that it can be done :-)

There are already those ugly "names" for the standard tags, like $label2. We need to come up with a scheme that unites both.

I'm not sure what used to happen to tag name changes in the past, that's not a new problem. I could tag items "huhu" and then remove that tag. Or was there an integrity check?

(In reply to Richard Marti (:Paenglab) from comment #65)

I see no coloured treechildren with this patch.

What? As I said, it only works immediately after adding the tag in the preferences since the CSS is only added when adding a tag. That should be good enough for testing.

(In reply to Jorg K (GMT+1) from comment #67)

(In reply to Richard Marti (:Paenglab) from comment #65)

I see no coloured treechildren with this patch.

What? As I said, it only works immediately after adding the tag in the preferences since the CSS is only added when adding a tag. That should be good enough for testing.

I opened the prefs, added a new tag, closed the pref. Now I selected a message and assigned the new tag. The row in the tree is still light blue when selected. Only the text changed to white.

Maybe instead of adding the rule to mailWindow1.css we should create a style-sheet just for the tags, or include tagColors.css directly into messenger.xul. That way we might be able to scrap all rules added and then add them again when a tag is edited. Or else we'd have to find the rules corresponding to the tag and remove them.

I can see a lot organising being required to get this right for all cases.

Attached patch 1497041-dynamic-css.patch (v2a) (obsolete) — — Splinter Review

Oops, forgot hg qref. I wish you'd use a debug build and you'd have seen that there were two hashes in front of the colour. Also, tag names need to be lowercase since the view code makes the custom tags lowercase :-(

Attachment #9047975 - Attachment is obsolete: true
Attached image Screenshot (obsolete) —

Maybe with a new style-sheet. Then we can remove tagColors.css. When we start, check a pref like mail.tagColors.migrated. On first start it's not on true, so create all tags in the new style-sheet and set the pref to true. Then only the adding, deleting and editing needs to access the file.

Yes, something like that. Calendar uses some sort of cache:

https://searchfox.org/comm-central/rev/d2ace128d8a14a3da920dfc253b23f31c8952f00/calendar/base/content/calendar-views.js#500

There are quite a few issues to solve here:

  1. Load the rules at startup into all documents/tabs showing a folder tree. Or perhaps the all reference a common stylesheet.
  2. Do add/edit/delete properly.
  3. How to encode the tags if the contain non-ASCII characters.
  4. How to integrate the fixed names $labelN where N=1..5 (right?)
  5. Which style-sheet to use and how to include it. We could just empty out tagColors.css completely and include it directly.
Attached patch 1497041-dynamic-css.patch (v3) (obsolete) — — Splinter Review

This reshuffles the CSS the way I think it needs to be done.

For the tag encoding I tried "Ríchárd" and found that this is already encoded to "r&ao0-ch&aoe-rd". Looking at nsMsgTagService::AddTag() it's MUTF-7.

Aceman suggested not to encode the tags but to map them to tag1, tag2

Attachment #9047983 - Attachment is obsolete: true

Grrrrr, BMO truncated my comment :-( - Here is what was truncated:

Aceman suggested not to encode the tags but to map them to tag1, tag2, etc., but I think that won't fly since I don't appear to have the ordinal number in nsMsgDBView.cpp. Given that it's already encoded, we might as well use that.

So I did the conversion to MUTF-7 in the JS code, but sadly it didn't work. Apparently
treechildren::-moz-tree-row(r&ao0-ch&aoe-rd, selected, focus) { background-color: #46ca67 !important; }
is not a valid selector. So in the end, a whacked base64 on top of it and now it works.

So try tag Ríchárd now. As a selector that becomes ciZhbzAtY2gmYW9lLXJk.

Ríchárd, can you please fix the CSS in the JS code and then I'll take it from there again. And while you're there, please ass tagColor.css to the search dialogue.

Oops, "add" tagColor.css to the search dialogue.

BTW, the latest patch solves 3. and 5. fro comment #73. The rest is still open.

Attached patch 1497041-dynamic-css.patch (v3b) (obsolete) — — Splinter Review

Some refinements. Richard added a second rule. Sadly now we get green on green when the row is selected. I guess that can be fixed by refining the selector more with what we previously had, the .lc-330033:not([_moz-menuactive].

I noticed that base64 produces = at the end which also doesn't work in selectors, so that's fixed as well.

I had the idea two scrap the three tagColors.css files and use one in "shared". However, we never include anything from "shared" into XUL files, so maybe we leave the tree files. In any case, since they are now the same, they need to be added to allowed-dupes.mn.

Attachment #9048019 - Attachment is obsolete: true
Attached patch 1497041-dynamic-css.patch (v3c) (obsolete) — — Splinter Review

tagColors.css in shared but loaded at the old place during build.

(In reply to Jorg K (GMT+1) from comment #78)

Created attachment 9048026 [details] [diff] [review]
1497041-dynamic-css.patch (v3b)

Some refinements. Richard added a second rule. Sadly now we get green on green when the row is selected. I guess that can be fixed by refining the selector more with what we previously had, the .lc-330033:not([_moz-menuactive].

The .lc-330033:not([_moz-menuactive] was for the tag menu. Fixed by re-adding the lc-white and lc-black rules.

Attachment #9048026 - Attachment is obsolete: true

Thanks, Richard. I'd prefer not to do this:
rename from mail/themes/linux/mail/tagColors.css
rename to mail/themes/shared/mail/tagColors.css

Let's delete them all and then add a new one. Let's say it's "almost" empty ;-)

Tonight I will fix issue 1, so load the rules at start-up so we can play better :-)

Attached patch 1497041-dynamic-css.patch (v4) (obsolete) — — Splinter Review

OK, this load the tag colours back into the sheet on start-up. I also reshuffled the deletion of tagColors.css and its recreation in "shared".

It's still a bit hacky since I copied some code around which should be moved to a common place. So let's look at the list of issues:

  1. Load the rules at start-up into all documents/tabs showing a folder tree.
  2. Do add/edit/delete properly.
  3. How to encode the tags if the contain non-ASCII characters.
  4. How to integrate the fixed names $labelN where N=1..5 (right?)
  5. Which style-sheet to use and how to include it. We could just empty out tagColors.css completely and include it directly.

So 1. is mostly done, even if you open a folder in a new tab, the tag colours show, also for a new tag. Looks like all the documents use the same stylesheet.

  1. works only for adding. 3., 4. and 5. are done. The "fixed names" are actually the keys of the tags. And there is a service to get the keys, so we don't have to hand-roll the MUTF-7 stuff.

So here's a new to-do list:

2b. edit/delete.
6. Search dialogue not working.
7. refactoring of common code.

Richard, do you like it so far? What else needs doing? At least I achieve my aim to kill tagColors.css in its present form. That madness is gone now :-)

Attachment #9048028 - Attachment is obsolete: true
Attached patch 1497041-dynamic-css.patch (v5) (obsolete) — — Splinter Review

OK, this should do it. Edit working, we don't do anything for delete. Adding a tag via the "Tag" item on the main toolbar also works. The search windows works. And I stuffed all the duplicated code into TagServices.jsm.

So now we have the boring task of adding some jsdoc. There's also dump() left in the code, I'm open to suggestions: Pull it out, make it report an error. Whatever.

Aceman, Geoff, who'd like to review the final version?

Attachment #9048044 - Attachment is obsolete: true
Attachment #9048050 - Flags: feedback?(richard.marti)
Attachment #9048050 - Flags: feedback?(geoff)
Attachment #9048050 - Flags: feedback?(acelists)

Oh, found bug 1532167 in the process.

Comment on attachment 9048050 [details] [diff] [review]
1497041-dynamic-css.patch (v5)

Works very good for me.

The only issue I see is, when a light colour is chosen, like light yellow, and the row is selected, then the text is white. This isn't an issue of this patch, but it seems that lc-white and lc-black don't adapt to have a better contrast. That could be for a follow-up bug.
Attachment #9048050 - Flags: feedback?(richard.marti) → feedback+
Attached patch 1497041-cleanup-win8-stuff.patch (obsolete) — — Splinter Review

OK, I promised to clean up the Windows 8 mess I made, here it is.

Assignee: richard.marti → jorgk
Attachment #9048051 - Flags: review?(acelists)
Attachment #9048051 - Flags: feedback?(richard.marti)

(In reply to Richard Marti (:Paenglab) from comment #84)

Comment on attachment 9048050 [details] [diff] [review]
The only issue I see is, when a light colour is chosen, like light yellow,
and the row is selected, then the text is white. This isn't an issue of this
patch, but it seems that lc-white and lc-black don't adapt to have a better
contrast. That could be for a follow-up bug.

Exactly, it's this primitive code here:
https://searchfox.org/comm-central/rev/1c3790982365d9762765ed64ffbdde0bed6cc070/mailnews/base/src/nsMsgDBView.cpp#274
Only if the background colour is exactly white, we use black. None of the sophisticated contrast checking we do in JS.

Small tweak, thanks to Richard for testing on Windows 8 :-)

Attachment #9048051 - Attachment is obsolete: true
Attachment #9048051 - Flags: review?(acelists)
Attachment #9048051 - Flags: feedback?(richard.marti)
Attachment #9048055 - Flags: review?(acelists)
Attachment #9048055 - Flags: feedback?(richard.marti)
Comment on attachment 9048055 [details] [diff] [review]
1497041-cleanup-win8-stuff.patch (v1b)

Tested on Windows 8. Works.
Attachment #9048055 - Flags: feedback?(richard.marti) → feedback+
Attachment #9047215 - Attachment description: 1497041-tag-colors.patch → 1497041-tag-colors.patch [landed in comment #50]
Attachment #9047337 - Attachment description: 1497041-registry-import.patch → 1497041-registry-import.patch [landed in comment #52, bustage-fix]
Attachment #9047984 - Attachment is obsolete: true
Comment on attachment 9048055 [details] [diff] [review]
1497041-cleanup-win8-stuff.patch (v1b)

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

::: mail/base/modules/TagServices.jsm
@@ +10,5 @@
>  var TagServices = {
>    loadTagsIntoCSS,
>    addTagToDocumentSheet,
> +  isColorContrastEnough,
> +  getColor,

Interesting, usually we just implement the functions directly in the object.

::: mail/base/modules/Windows8WindowFrameColor.jsm
@@ -7,5 @@
>  this.EXPORTED_SYMBOLS = ["Windows8WindowFrameColor"];
>  
> -if ("@mozilla.org/windows-registry-key;1" in Cc) {
> -  var {WindowsRegistry} = ChromeUtils.import("resource://gre/modules/WindowsRegistry.jsm");
> -}

Nice, so we will load this only on Windows again, from msgMail3PaneWindow.js.

(In reply to :aceman from comment #89)

Interesting, usually we just implement the functions directly in the object.

Inspired by:
https://searchfox.org/comm-central/rev/1c3790982365d9762765ed64ffbdde0bed6cc070/mail/base/modules/DisplayNameUtils.jsm#11

Avoids this.function().

Attached patch 1497041-dynamic-css.patch (v5b) (obsolete) — — Splinter Review

Aceman pointed out that I needed to hit all windows when a new tag is created. Here we go.

Attachment #9048050 - Attachment is obsolete: true
Attachment #9048050 - Flags: feedback?(geoff)
Attachment #9048050 - Flags: feedback?(acelists)
Attachment #9048069 - Flags: feedback?(acelists)

Rebased.

Attachment #9048055 - Attachment is obsolete: true
Attachment #9048055 - Flags: review?(acelists)
Attachment #9048070 - Flags: review?(acelists)
Comment on attachment 9048050 [details] [diff] [review]
1497041-dynamic-css.patch (v5)

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

Thanks guys, this looks interesting. I'm impressed how short it is, if it actually works for all cases.

I see you include the tagColors.css in xul windows where needed and also call loadTagsIntoCSS().
May I suggest that we use ExtensionSupport.registerWindowListener() which triggers on all windows and then it detects if tagColors.css is in the sheets of current window and only then calls loadTagsIntoCSS(). This would cover all future cases when we add tagColors.css to new windows, or addons use it. Somebody somewhere could forget to call loadTagsIntoCSS().

As to the appearance, I noticed some places have changed.
E.g. with the base "Personal" tag, I got green background and white text in the small 'Tags' bar in the message header view, in trunk. In TB60, there is black text.
Also, the green does seem to be different in that message header and in the background of the message when it is selected in the thread pane.

::: mail/base/content/SearchDialog.js
@@ +14,5 @@
>  
>  var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
>  var {MailUtils} = ChromeUtils.import("resource:///modules/MailUtils.jsm");
>  var {PluralForm} = ChromeUtils.import("resource://gre/modules/PluralForm.jsm");
> +var {TagServices} = ChromeUtils.import("resource:///modules/TagServices.jsm");

This would be unneeded.

@@ +219,5 @@
>  };
>  
>  
>  function searchOnLoad() {
> +  TagServices.loadTagsIntoCSS(document);

This would be unneeded.

::: mail/base/content/mailWindowOverlay.js
@@ +933,5 @@
>  }
>  
>  function AddTagCallback(name, color) {
>    MailServices.tags.addTag(name, color, "");
> +  TagServices.addTagToDocumentSheet(document, MailServices.tags.getKeyForTag(name), color);

I understand it could be hard to implement TagServices.addTagToDocumentSheet() inside the C++ implementation of MailServices.tags.addTag().
May you put both of these commands into a helper, e.g. inside the TagServices.jsm?
So that again, somebody does not forget to call the other one when adding new tags.

Also, adding the new tag may need to be done on all documents, not just the current one.

::: mail/base/content/msgMail3PaneWindow.js
@@ +35,5 @@
>  var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
>  var {MailUtils} = ChromeUtils.import("resource:///modules/MailUtils.jsm");
>  var {AppConstants} = ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
>  var {Color} = ChromeUtils.import("resource://gre/modules/Color.jsm");
> +var {TagServices} = ChromeUtils.import("resource:///modules/TagServices.jsm");

This would be unneeded.

@@ +424,5 @@
>  /**
>   * Called on startup to initialize various parts of the main window
>   */
>  function OnLoadMessenger() {
> +  TagServices.loadTagsIntoCSS(document);

This would be unneeded.

::: mail/base/modules/TagServices.jsm
@@ +17,5 @@
> +  for (let tag of tagArray) {
> +    // tag.key is the internal key, like "$label1" for "Important".
> +    // For user defined keys with non-ASCII characters, key is
> +    // the MUTF-7 encoded name.
> +    addTagToSheet(tag.key, tag.color, tagSheet);

OK, using tag.key instead of the "tag1, ..." may be OK. It still needs to be encoded, but at least we always have that key at hand instead of having to find the index of the tag.

@@ +58,5 @@
> +      break;
> +    }
> +  }
> +  if (!tagSheet) {
> +    dump("== findTagColorSheet: Oops, tagColors.css not found\n");

This wouldn't be an error if we just ran this on all documents.

::: mail/components/preferences/display.js
@@ +306,5 @@
>    MailServices.tags.addTag(aName, aColor, "");
>  
> +  // Add to style sheet.
> +  TagServices.addTagToDocumentSheet(Services.wm.getMostRecentWindow("mail:3pane").document,
> +                                    MailServices.tags.getKeyForTag(aName), aColor);

Does this need to be run for all windows/documents so that it is picked up? You can have multiple "mail:3pane" windows open and the search window too, while changing tags.

::: mail/themes/shared/mail/tagColors.css
@@ +6,5 @@
> +
> +treechildren::-moz-tree-cell-text(lc-white, selected, focus) {
> +  color: #fff !important;
> +}
> +treechildren::-moz-tree-cell-text(lc-black, selected, focus) {

Do we need to keep the "lc-" prefix now? Are the dynamically generated properties also containing it?
I guess 'LC' means 'label color'?

::: mailnews/base/src/nsMsgDBView.cpp
@@ +276,5 @@
>          properties.AppendLiteral(" lc-black");
>        else
>          properties.AppendLiteral(" lc-white");
>      }
> +    properties.Append(' ');

Would AppendLiteral() be better? I think there were some anomalies in this area.

@@ +287,5 @@
> +    size_t len = strlen(base64);
> +    while (base64[len-1] == '=') {
> +      base64[len-1] = 0;
> +      len--;
> +    }

Can there be multiple = signs?

@@ +290,5 @@
> +      len--;
> +    }
> +    nsAutoCString topKey64;
> +    topKey64.Adopt(base64);
> +    properties.Append(NS_ConvertUTF8toUTF16(topKey64));

So many conversions.
This is a hot path and runs at each scroll or redraw of a message row in the thread pane.

Would just replacing the & and - by something be faster?
Also where does the MUTF-7 come from? Can we change the backend to not use it? Or is it already stored in the key, in users' prefs.js files so we can't switch away from it easily?
Attachment #9048050 - Attachment is obsolete: false
Attachment #9048050 - Attachment is obsolete: true

(In reply to :aceman from comment #93)

May I suggest that we use ExtensionSupport.registerWindowListener() which
triggers on all windows and then it detects if tagColors.css is in the
sheets of current window and only then calls loadTagsIntoCSS(). This would
cover all future cases when we add tagColors.css to new windows, or addons
use it. Somebody somewhere could forget to call loadTagsIntoCSS().

Well, I think that's overkill. If someone adds a new window type that shows a message list/tree, then they have to make sure the CSS is loaded. Alta88 suggested to used a user agent sheet in comment #56 and I think that's overkill as well.

As to the appearance, I noticed some places have changed.
E.g. with the base "Personal" tag, I got green background and white text in
the small 'Tags' bar in the message header view, in trunk. In TB60, there is
black text.

Yes, there is a different contrast calculation.

Also, the green does seem to be different in that message header and in the
background of the message when it is selected in the thread pane.

Yes, there seems to be some blending going on.

May you put both of these commands into a helper, e.g. inside the
TagServices.jsm?
So that again, somebody does not forget to call the other one when adding
new tags.

Matter of taste. You want to make programming fool-proof?

Also, adding the new tag may need to be done on all documents, not just the
current one.

Done already in (5b).

OK, using tag.key instead of the "tag1, ..." may be OK. It still needs to be
encoded, but at least we always have that key at hand instead of having to
find the index of the tag.

I don't understand the comment. The keys are already MUTF-7 encoded, so I whack base64 on top.

Does this need to be run for all windows/documents so that it is picked up?
You can have multiple "mail:3pane" windows open and the search window too,
while changing tags.

Yes, done already in (5b).

Do we need to keep the "lc-" prefix now? Are the dynamically generated
properties also containing it?

Richard can answer that. They are not dynamic but created in nsMsgDBView.cpp.

Would AppendLiteral() be better? I think there were some anomalies in this
area.

No. Append for a single character. We fixed all this in autumn 2017, remember?

Can there be multiple = signs?

Yes, at least two, maybe three.

So many conversions.
This is a hot path and runs at each scroll or redraw of a message row in the
thread pane.

I can't change that. I already use Adopt() which doesn't copy the string buffer.

Would just replacing the & and - by something be faster?
Also where does the MUTF-7 come from? Can we change the backend to not use
it? Or is it already stored in the key, in users' prefs.js files so we can't
switch away from it easily?

The comment says where it comes from:
nsMsgTagService::AddTag()
https://searchfox.org/comm-central/rev/1c3790982365d9762765ed64ffbdde0bed6cc070/mailnews/base/src/nsMsgTagService.cpp#244
Looks like this goes to the prefs and the IMAP server.

So what do you want me to change? I answered all question and don't see the need to change anything now.

Comment on attachment 9048069 [details] [diff] [review]
1497041-dynamic-css.patch (v5b)

Maybe Geoff has an opinion here.
Attachment #9048069 - Flags: review?(geoff)
Attachment #9048069 - Flags: review?(acelists)
Attachment #9048069 - Flags: feedback?(acelists)
Attachment #9048070 - Flags: review?(geoff)
Attachment #9048070 - Flags: review?(geoff) → review+
Comment on attachment 9048069 [details] [diff] [review]
1497041-dynamic-css.patch (v5b)

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

In principle I think this alright, but there's a few problems.

::: mail/base/modules/TagServices.jsm
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const {MailServices} = ChromeUtils.import("resource:///modules/MailServices.jsm");
> +
> +var EXPORTED_SYMBOLS = ["TagServices"];

Is TagUtils a better name?

@@ +47,5 @@
> +  // Add rule to sheet.
> +  // Sadly the key is using MUTF-7 which contains & and - which are not valid in selectors,
> +  // hence we add base64 on top.
> +  // More sadly, the potential = at the end of base64 encoding doesn't work in CSS selectors,
> +  // so we need to strip it.

I was about to say that this was clever because all possible characters from btoa (except the =) can be used as a CSS selector, but that's not true. Neither / nor + can be used.

Also the comment isn't correct because - is valid in CSS selectors.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +272,4 @@
>    {
>      if (addSelectedTextProperty)
>      {
>        if (color.EqualsLiteral(LABEL_COLOR_WHITE_STRING))

This is ineffective as it only covers #FFFFFF. The colour picker returns lowercase values now. Also there's the same bad result for colours close to white but not exactly white.

I think the lc-black/lc-white properties might be unnecessary now that you can use stylesheet rules to replace them.
Attachment #9048069 - Flags: review?(geoff) → review-

You could take the UTF-7 string and replace all characters not in A-Za-z_ with the character code in hex, since we know if it's UTF-7 encoded that all the characters are single-byte:

aKey.replace(/\W/g, c => "-" + ("00" + c.charCodeAt(0).toString(16)).substr(-2) + "-")

This leaves a vaguely readable string too. You would need to add a letter or letters to the front.

Comment on attachment 9048069 [details] [diff] [review]
1497041-dynamic-css.patch (v5b)

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

::: mail/base/modules/TagServices.jsm
@@ +26,5 @@
> +  addTagToSheet(aKey, aColor, findTagColorSheet(aDocument));
> +}
> +
> +function addTagToAllDocumentSheets(aKey, aColor) {
> +  let windowList = Services.wm.getEnumerator("mail:3pane", true);

You haven't imported Services.

(In reply to :aceman from comment #93)

Comment on attachment 9048050 [details] [diff] [review]
1497041-dynamic-css.patch (v5)

Review of attachment 9048050 [details] [diff] [review]:

::: mail/themes/shared/mail/tagColors.css
@@ +6,5 @@

+treechildren::-moz-tree-cell-text(lc-white, selected, focus) {

  • color: #fff !important;
    +}
    +treechildren::-moz-tree-cell-text(lc-black, selected, focus) {

Do we need to keep the "lc-" prefix now? Are the dynamically generated
properties also containing it?
I guess 'LC' means 'label color'?

This is what noted in comment 84 to be not dynamic. When this would be implemented, this two rules can go away together with the properties.AppendLiteral(" lc-* nsMsgDBView.cpp.

We'd need a third rule that sets black or white depending on the contrast:

let ruleString3 = "treechildren::-moz-tree-cell-text(" + selector + ", selected, focus) { color: " + BLACK/WHITE + "; }";

Geoff, thank for the comments. I'll rename to TagUtils and add the missing import. The color returned from GetColorForKey() is lowercase, so I'll fix the comparison. I don't intend to address the long-standing issue of "white on light" as noted before.

The most severe issue here is the encoding, I'll have to look into it in more detail.

Actually, no, it returns any case.

Attached patch 1497041-dynamic-css.patch (v5c) (obsolete) — — Splinter Review

This fixes the easy stuff, I still need to think about the encoding.

Attachment #9048069 - Attachment is obsolete: true
Attachment #9048069 - Flags: review?(acelists)

Rebased.

Attachment #9048070 - Attachment is obsolete: true
Attachment #9048070 - Flags: review?(acelists)
Attachment #9048134 - Flags: review+

(v1d) had a |ReferenceError: Color is not defined| error and no tag was shown in the messageheader. Fixed this.

Additionally I added the dynamic text colour setting and removed the lc-*.

Attachment #9048134 - Attachment is obsolete: true
Attachment #9048218 - Flags: review?(jorgk)
Attached patch 1497041-cleanup-win8-stuff.patch (obsolete) — — Splinter Review

Sorry for the spam, missed to do the last qrefresh with a line length change.

Attachment #9048218 - Attachment is obsolete: true
Attachment #9048218 - Flags: review?(jorgk)
Attachment #9048221 - Flags: review?(jorgk)
Comment on attachment 9048221 [details] [diff] [review]
1497041-cleanup-win8-stuff.patch

Thanks, great minds thinks alike. I'm reworking it quite significantly and I already added rule3. I'll pick up your relevant hunks and let you test is.
Attachment #9048221 - Attachment is obsolete: true
Attachment #9048221 - Flags: review?(jorgk)
Attached patch 1497041-dynamic-css.patch (v6) (obsolete) — — Splinter Review

In this patch I ...

  • merged the two patches together so I had access to the contrast calculation,
  • fixed the encoding, still using base64 but replacing unwanted characters,
  • introduced nsIMsgTagService.getSelectorForKey() to concentrate the calculation in one spot,
  • reduced the amount of copying around/conversion of data on the hot code path to make Aceman happy,
  • fixed the contrast issue "white on light" as per Richard's patch.

There appears to be something funny with the contract calculation. Define a tag as pure green (#00ff00) and you'll get white writing on green although black should be OK, no?

Attachment #9048133 - Attachment is obsolete: true
Attachment #9048250 - Flags: feedback?(richard.marti)
Comment on attachment 9048250 [details] [diff] [review]
1497041-dynamic-css.patch (v6)

Looks good. Tested also under Windows 8.1.
Attachment #9048250 - Flags: feedback?(richard.marti) → feedback+
Attached patch 1497041-dynamic-css.patch (v6b) (obsolete) — — Splinter Review

OK, base64 is out, here is Geoff's suggestion from comment #97. This will be faster than encoding to base64 and gives more readable tags. For plain ASCII tags we get the tag with a leading T to avoid collision with "selected" or "focus". I hope there is no inbuilt selector starting with "T".

Attachment #9048407 - Flags: review?(geoff)
Attached patch 1497041-dynamic-css.patch (v6c) (obsolete) — — Splinter Review

Oops, now without spelling mistake and debug print.

Attachment #9048407 - Attachment is obsolete: true
Attachment #9048407 - Flags: review?(geoff)
Attachment #9048408 - Flags: review?(geoff)
Comment on attachment 9048408 [details] [diff] [review]
1497041-dynamic-css.patch (v6c)

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

::: mail/base/content/multimessageview.js
@@ +362,5 @@
>      for (let tag of sortedTags) {
>        let tagNode = document.createElement("span");
>        let color = MailServices.tags.getColorForKey(tag.key);
>        let textColor = "black";
> +      if (!TagUtils.isColorContrastEnough(color)) {

So if the contrast to black is not enough, we use white. Is that always true that contrast to white is enough?

::: mail/base/modules/TagUtils.jsm
@@ +48,5 @@
> +  let selector = MailServices.tags.getSelectorForKey(aKey);
> +  let ruleString1 = "treechildren::-moz-tree-row(" + selector +
> +                    ", selected, focus) { background-color: " + aColor + " !important; }";
> +  let ruleString2 = "treechildren::-moz-tree-cell-text(" + selector +
> +                    ") { color: " + aColor + "; }";

Could you try if using the `..${selector}..` syntax would make these more readable and fit on a line?

@@ +84,5 @@
> +
> +// Here comes some stuff that was originally in Windows8WindowFrameColor.jsm.
> +
> +/* Checks if black writing on 'aColor' background has enough contrast */
> +function isColorContrastEnough(aColor) {

Interesting that this is hardcoded to compare to black. But if this is what m-c has then it probably is enough.

::: mail/themes/shared/mail/tagColors.css
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* ::::: thread labels decoration ::::: */
> +
> +/* This style sheet is empty and will be dynamically populated */

So if you do not want my "run tag rule injecting automatically in all documents loading tagColors.css" proposal, can you at least add the instructions into this file?
Saying that whoever includes this in his document, must also call TagUtils.loadTagsIntoCSS(document) from his document's onload() handler.

::: mailnews/base/src/nsMsgDBView.cpp
@@ -268,5 @@
> -  nsCString color;
> -  rv = mTagService->GetColorForKey(topKey, color);
> -  if (NS_SUCCEEDED(rv) && !color.IsEmpty())
> -  {
> -    if (addSelectedTextProperty)

What was this doing and why can it be dropped?

@@ +269,5 @@
> +  if (NS_SUCCEEDED(rv))
> +  {
> +    properties.Append(' ');
> +    properties.Append(selector);
> +  }

Nice.

@@ +1439,4 @@
>      properties.AppendLiteral(" untagged");
> +  } else {
> +    AppendKeywordProperties(keywordProperty, properties);
> +    properties.AppendLiteral(" tagged");

So this is some new feature and you then bind some transparency in css on it?

::: mailnews/base/src/nsMsgTagService.cpp
@@ +286,5 @@
>  
> +/* long getSelectorForKey (in ACString key, out AString selector); */
> +NS_IMETHODIMP nsMsgTagService::GetSelectorForKey(const nsACString &key, nsAString &_retval)
> +{
> +  // Our keys are the result of MUFT-7 encoding. For CSS selectors we need

MUTF-7 typo?

@@ +301,5 @@
> +        ('a' <= *in && *in <= 'z') ||
> +        *in == '_' || *in == '-') {
> +      _retval.Append(*in);
> +    } else {
> +      _retval.AppendPrintf("%02x", *in);

Will this produce unique selectors? I'd think you need to add some special escaping marker, otherwise XY<charcode255>Z will produce XYFFZ, which is also the same as if the tag name would be XYFFZ.

(In reply to :aceman from comment #111)

Comment on attachment 9048408 [details] [diff] [review]
1497041-dynamic-css.patch (v6c)

Review of attachment 9048408 [details] [diff] [review]:

@@ +1439,4 @@

 properties.AppendLiteral(" untagged");
  • } else {
  • AppendKeywordProperties(keywordProperty, properties);
  • properties.AppendLiteral(" tagged");

So this is some new feature and you then bind some transparency in css on it?

Yes, on Windows with default theme we set a light blue gradient and with this we can override it with a transparency gradient to show the tag background-color behind. The "tagged" is needed because on the trees a :not(untagged) doesn't work.

(In reply to :aceman from comment #111)

So if the contrast to black is not enough, we use white. Is that always true
that contrast to white is enough?

Well, we're not going to try a million colours until one fits.

Could you try if using the ..${selector}.. syntax would make these more
readable and fit on a line?

Please look at it, it will still be longer that 80 chars.

So if you do not want my "run tag rule injecting automatically in all
documents loading tagColors.css" proposal, can you at least add the
instructions into this file?
Saying that whoever includes this in his document, must also call
TagUtils.loadTagsIntoCSS(document) from his document's onload() handler.

OK. Will do.

What was this doing and why can it be dropped?

The reviewer needs to study the code. In short, that lc-white/lc-black handling was only required when setting the selectors for cells and not rows. That is obsolete now.

Nice.

Yes, just for you ;-)

So this is some new feature and you then bind some transparency in css on it?

Richard wanted that and he's already answered.

MUTF-7 typo?

Yes.

Will this produce unique selectors? I'd think you need to add some special
escaping marker, otherwise XY<charcode255>Z will produce XYFFZ, which is
also the same as if the tag name would be XYFFZ.

OK, I'll revisit that.

This should address Aceman's comments. I'm confused who wants to the the final reviewer, so first reviewer wins ;-) - And everyone can enjoy the noise.

Attachment #9048250 - Attachment is obsolete: true
Attachment #9048408 - Attachment is obsolete: true
Attachment #9048408 - Flags: review?(geoff)
Attachment #9048436 - Flags: review?(geoff)
Attachment #9048436 - Flags: review?(acelists)
Comment on attachment 9048436 [details] [diff] [review]
1497041-dynamic-css.patch (v6d)

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

::: mail/themes/shared/mail/tagColors.css
@@ +6,5 @@
> +
> +/*
> + * This style sheet is empty and will be dynamically populated.
> + * To populate it, typically TagUtils.loadTagsIntoCSS(document)
> + * is called from the document's onload() handler.

OK.

@@ +8,5 @@
> + * This style sheet is empty and will be dynamically populated.
> + * To populate it, typically TagUtils.loadTagsIntoCSS(document)
> + * is called from the document's onload() handler.
> + * There is also TagUtils.addTagToAllDocumentSheets() which adds
> + * CSS to all open messenger and search windows.

If you add this here, specify the "CSS" so that nobody thinks this CSS (tagColors.css) is to be added to all messenger windows.
addTagToAllDocumentSheets is only needed if you modify tags. Some addons could do that. That's why I wanted to merge addTag() and addTagToAllDocumentSheets() into a helper in TagUtils.jsm so that e.g. addons do not even need to not forget this detail.

::: mailnews/base/src/nsMsgTagService.cpp
@@ +291,5 @@
> +  // to reduce this to 0-9A-Za-z_ with a leading alpha character.
> +  // We encode non-alphanumeric characters using _ as an escape character
> +  // and start with a leading T in all cases. This way users defining tags
> +  // "selected" or "focus" don't collide with inbuilt "selected" or "focus".
> +  

Trailing whitespace added?

@@ +316,5 @@
> +        ('A' <= *in && *in <= 'Z') ||
> +        ('a' <= *in && *in <= 'z')) {
> +      _retval.Append(*in);
> +    } else {
> +      _retval.AppendPrintf("_%02x", *in);

Why the double loop? The original was OK, you could just change to the .AppendPrintf("_%02x") there.

(In reply to Jorg K (GMT+1) from comment #113)

(In reply to :aceman from comment #111)

So if the contrast to black is not enough, we use white. Is that always true
that contrast to white is enough?

Well, we're not going to try a million colours until one fits.

Sure, I asked if there may be some mathematical certainty.
E.g. if your 'color distance' from black is X and X < 78 (the minimum enough contrast), then the 'color distance' to white is (255 - X), which will be > 78. Or something like that.

Axel, do I remember correctly that you do some stuff with tags in your addons (quickFolders or so)? Can you see if the changes in this patch cover your needs? E.g. if you create tags on the fly, you may need to call some stuff now. Also it seems in the current patch, addon-defined windows will not receive tag color changes (only the 3-pane and message search windows are visited).

Flags: needinfo?(axelg)

(In reply to :aceman from comment #117)

Axel, do I remember correctly that you do some stuff with tags in your addons (quickFolders or so)? Can you see if the changes in this patch cover your needs? E.g. if you create tags on the fly, you may need to call some stuff now. Also it seems in the current patch, addon-defined windows will not receive tag color changes (only the 3-pane and message search windows are visited).

I actually do nothing with tag colors - but my add-on quickFilters will listen for tag changes by wrapping ToggleMessageTag - is that affected?
My function will call the original and then start its filter assistant based on the selected message in order to create a filter that tags messages according to it.

Flags: needinfo?(axelg)

I'm not going to upload yet another 135KB patch to remove some trailing space :-( - Yes, one feature of Notepad++ is that it auto-indents and that leads to trailing white-space if you're not careful. Usually I check before uploading a patch, here I forgot.

(In reply to :aceman from comment #115)

If you add this here, specify the "CSS" so that nobody thinks this CSS
(tagColors.css) is to be added to all messenger windows.
addTagToAllDocumentSheets is only needed if you modify tags. Some addons
could do that. That's why I wanted to merge addTag() and
addTagToAllDocumentSheets() into a helper in TagUtils.jsm so that e.g.
addons do not even need to not forget this detail.

Can you paste the exact wording you want so we avoid more iterations only for a comment. How about:

 * There is also TagUtils.addTagToAllDocumentSheets() which inserts
 * CSS into this sheet on all open messenger and search windows.

Trailing whitespace added?

Fixed locally.

Why the double loop? The original was OK, you could just change to the
.AppendPrintf("_%02x") there.

There was always a double loop hidden in strlen(). You wanted it efficient, so to avoid allocating more and more memory during the append calls, I calculate the expected length exactly up front. Before we had a worst case of "2x input + 1". Now that would be "3x input + 1", so I prefer to run a long the string twice which happened anyway.

Also, can we please limit the scope here. One day we need a solution that works in TB. We can revisit that if it turns out that add-ons have their own document types that feature trees. We're already at comment #117.

(In reply to Jorg K (GMT+1) from comment #119)

Can you paste the exact wording you want so we avoid more iterations only for a comment. How about:

 * There is also TagUtils.addTagToAllDocumentSheets() which inserts
 * CSS into this sheet on all open messenger and search windows.

Thanks, looks better.

There was always a double loop hidden in strlen(). You wanted it efficient, so to avoid allocating more and more memory during the append calls, I calculate the expected length exactly up front. Before we had a worst case of "2x input + 1". Now that would be "3x input + 1", so I prefer to run a long the string twice which happened anyway.

The string (tag key) shouldn't be long, so the code is quite verbose, but OK, it should be fast enough (uses just plain pointer++ and a counter).

Also, can we please limit the scope here. One day we need a solution that works in TB. We can revisit that if it turns out that add-ons have their own document types that feature trees. We're already at comment #117.

We have scope limited here, we just discuss how much features we can lose, as the patch is not restoring full previous functionality.

Attachment #9048436 - Flags: review?(acelists) → review+
Keywords: leave-open

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/92aaaaf8c66c
Add dynamic CSS for tags to stylesheet. r=darktrojan,aceman

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

Landed with the comment and white-space changes. Yay!

Target Milestone: --- → Thunderbird 67.0
Comment on attachment 9047215 [details] [diff] [review]
1497041-tag-colors.patch [landed in comment #50]

Beta uplift for all patches.
Attachment #9047215 - Flags: approval-comm-beta+
Comment on attachment 9048436 [details] [diff] [review]
1497041-dynamic-css.patch (v6d)

Nothing worth complaining about here.
Attachment #9048436 - Flags: review?(geoff) → review+

Since someone asked, the default labels Important, Work, etc. which are internally $label1, $label2 produce this dynamic CSS:

=== treechildren::-moz-tree-cell-text(T_24label1) { color: #ff0000; }
=== treechildren::-moz-tree-cell-text(T_24label1, selected, focus) { color: black; }
=== treechildren::-moz-tree-row(T_24label2, selected, focus) { background-color: #FF9900 !important; }
=== treechildren::-moz-tree-cell-text(T_24label2) { color: #FF9900; }
=== treechildren::-moz-tree-cell-text(T_24label2, selected, focus) { color: black; }
=== treechildren::-moz-tree-row(T_24label3, selected, focus) { background-color: #009900 !important; }

_24 is the encoding for $, basically its HEX value with a _

Query - jorg can you advise ?
userChrome.css
This works for any default tag that has a $label
eg:
mailnews.tags.$label1.tag;Important
can use:
treechildren::-moz-tree-cell-text(T_24label1, unread) {
color: #FF0000 !important;
font-weight: bold !important;
}

But what about tags created by user ?
eg:
mailnews.tags.done.tag;Done

T_24 not required as there is no $.
Unfortunately this does not work. I read previous comments in this bug report and it seems 'done' should be useable.
Have I misunderstood something?

treechildren::-moz-tree-cell-text(done, unread) {
color: #009900 !important;
font-weight: bold !important;
}

The _24 is for the $ in $label1. I think it would just be Tdone since the T is always prepended (no warranty on that).

Thanks Jorg, that worked perfectly.
I had someone using dark theme who did not like the new blue text and wanted white text again. This was easy to fix in userchrome.css, but then another person joined support conversaton and said they wanted white text, but never marked anything as read, so the fix I had supplied meant the tag colour did not work on unread mail. So they needed ability to tag and see the tag colours.

I had some fun trying to use the old method and remembered some recent work on tags had been done, so located this bug report.
It shows how valuable bug report information can be to those not necessarilly helping with fixing or supplying helpful information for the bug itself.
Your help is much appreciated.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: