Closed Bug 1498908 Opened 11 months ago Closed 11 months ago

Folders with unread messages no longer distinguishable/bold in folder pane after upgrading to Thunderbird 63.0b1

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set

Tracking

(thunderbird63 wontfix, thunderbird64 fixed)

RESOLVED FIXED
Thunderbird 64.0
Tracking Status
thunderbird63 --- wontfix
thunderbird64 --- fixed

People

(Reporter: Gijs, Assigned: Paenglab)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

Running 63.0b1 (64-bit) on macOS 10.13. The font in the folder pane has changed as part of the upgrade. Perhaps related to this, perhaps not, the folders with unread messages are no longer bold, and as a result it isn't possible for me to see at a glance which folders have unread messages and which don't.

This seems related to bug 1498751 if this has to do with e.g. the right atoms being listed for the row in the treeview, which seems likely.
Hm, though bug 1498751 is about the messages list instead of the folder list, so they're probably not directly the same bug.
See Also: 1498751
Hm, for a folder with unread messages I still see:

"folderNameCol serverType-imap specialFolder-none biffState-NoMail isSecure-true hasUnreadMessages-true noSelect-false imapShared-false"

when asking for getRowProperties(<row index that has unread messages>).
If I change:

treechildren::-moz-tree-cell-text(hasUnreadMessages-true) {
  font-weight: bold;
}

to

treechildren::-moz-tree-cell-text(hasUnreadMessages-true) {
  font-weight: bold !important;
}

in folderPane.css then the correct behavior returns (though the font is still odd). Richard, do you know why this may have regressed and/or why we need !important now?

It's not easy to figure out with the inspector because they're all pseudo-elements...
+ni for comment #3.
Flags: needinfo?(richard.marti)
Yes, the font change was in bug 1461026 and surely the cause of this issue.

Gijs, would you be able to review a patch? We are very limited with Mac developers.
Flags: needinfo?(richard.marti)
(In reply to Richard Marti (:Paenglab) from comment #5)
> Yes, the font change was in bug 1461026 and surely the cause of this issue.
> 
> Gijs, would you be able to review a patch? We are very limited with Mac
> developers.

Sure - who's writing it? :-)
Blocks: 1461026
(In reply to :Gijs (he/him) from comment #6)
> 
> Sure - who's writing it? :-)

Naturally me. :-) I'm building now.
Attached patch 1498908-unreadSelector.patch (obsolete) — Splinter Review
I increased the specificity to beat the font-weight rule for the Yosemite+ folderPane.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9017011 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 9017011 [details] [diff] [review]
1498908-unreadSelector.patch

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

So this isn't enough to fix the changes from the regressing bug. In particular, the server headers are also no longer blue-text. There might be other issues, I'd have to doublecheck the other rules in folderPane more carefully.

I don't really understand why these rules live in mailWindow1.css instead of in the folderPane css code, and why they have the #id selector portion. Can we move them to the folderPane code and reduce their specificity, instead of copying rules from folderPane to mailWindow1.css and increasing their specificity? Sorry if this is a dumb question - I'm not familiar with Thunderbird's architecture.
Attachment #9017011 - Flags: review?(gijskruitbosch+bugs) → review-
Attached patch themeTrees.patch (obsolete) — Splinter Review
Hmm, here it works. Do you probably use the dark theme? This patch should fix this with adding !important because we need now to overrule the dark theme rules too. This is needed also on Linux and Windows.

folderPane.css is not only loaded in the folderPane but also in other locations like the message search window or the menulist with type="folder".

I only moved the (imapdeleted) to folderPane.css which also makes sense in the message search window.
Attachment #9017011 - Attachment is obsolete: true
Attachment #9017032 - Flags: review?(gijskruitbosch+bugs)
Attached patch 1498908-unreadSelector.patch (obsolete) — Splinter Review
Oops, sorry the wrong patch.
Attachment #9017032 - Attachment is obsolete: true
Attachment #9017032 - Flags: review?(gijskruitbosch+bugs)
Attachment #9017033 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 9017033 [details] [diff] [review]
1498908-unreadSelector.patch

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

I'll believe that this fixes the issue, but it's hard for me to verify it doesn't introduce new, other issues - !important means we now end up overriding the color + font-weight everywhere, even if there were other rules in place for those that really *should* override the ones in folderPane.css (unlike the one in mailWindow1.css, which *shouldn't*). So f+ from me on fixing the problem, but I think you should get an r+ from a mail peer who understands the codebase better and knows for sure if there are other places that this would affect.

In particular, I worry about the effect this will have on highlighted/selected rows / text, also in menulist dropdowns.

More generally, the !important makes me sad, but based on what you said I'm not sure there are easy, better alternatives.

As an alternative suggestion: could we use a different selector for the folder pane in mailWindow1.css that wouldn't increase the specificity so much, and/or are there non-ID (maybe class?) selectors we could use in folderPane.css to increase specificity that would apply to all the different usage locations? Or are there none really available?
Attachment #9017033 - Flags: review?(gijskruitbosch+bugs) → feedback+
(In reply to Richard Marti (:Paenglab) from comment #10)
> Created attachment 9017032 [details] [diff] [review]
> themeTrees.patch
> 
> Hmm, here it works.

To be clear, I didn't test the patch (I don't have a setup for that). I was going by code inspection, and the fact that the colour of the server rows is gone in my "production" beta copy that I use for my day-to-day email. Your original patch "only" added a rule for font-weight, but the patch in bug 1461026 increased specificity for the rules governing text color (ie css color: property), too, so I would expect the same issues with the server row and this rule: https://dxr.mozilla.org/comm-central/source/comm/mail/themes/osx/mail/folderPane.css#338 . Am I missing something?

> Do you probably use the dark theme?

Nope.
To the previous patch I also tidied the new message colours to use always the same colour also when the row is actually selected. I could also use #folderTree instead of the !important but then the highlighting etc. in for example the folder selector in the smart filter dialog would no more woark as it does now. I tested this and saw no problems with the !important.
Attachment #9017033 - Attachment is obsolete: true
Attachment #9017093 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9017093 [details] [diff] [review]
1498908-unreadSelector.patch

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

Seems to work. r=mkmelin
Attachment #9017093 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
Comment on attachment 9017093 [details] [diff] [review]
1498908-unreadSelector.patch

I guess you want to fix TB 63b1. I haven't tried whether the patch applies.
Attachment #9017093 - Flags: approval-comm-beta+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3e5b48477fb4
Increase the specificity of the unread messages treechildren selectors. r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
(In reply to Jorg K (GMT+2) from comment #17)
> TB 63.0b2:
> https://hg.mozilla.org/releases/comm-beta/rev/e9f0f4d5d705
> 
> Trunk will land in a minute. Gijs, can you pick a build from this push,
> please:
> https://treeherder.mozilla.org/#/jobs?repo=comm-
> beta&revision=e9f0f4d5d7053a5e0cad397e73b1989199965c90

LGTM, thank you!
Is there a timescale for the release of a beta with the fix? I looked at https://wiki.mozilla.org/Thunderbird:Home#Releases but that didn't really help me...
(In reply to :Gijs (he/him) from comment #20)
> Is there a timescale for the release of a beta with the fix? I looked at
> https://wiki.mozilla.org/Thunderbird:Home#Releases but that didn't really
> help me...

If all goes well, next week. If does not go well, which is frequent, then not possible to predict.
Duplicate of this bug: 1499431
I reported 1499431. It's not just lack of bold, the new folder tree font weight is off relative to native UI. Needs to be slightly heavier overall. See screenshot in referenced bug.
Gijs, thanks for verifying, but we're not shipping a TB 63 beta 2.
You need to log in before you can comment on or make changes to this bug.