1569129 - mail notification oddly formatted with very long From: address

Status: VERIFIED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jorg K (CEST = GMT+2)]

Attachment: bad-notification.png

+++ This bug was initially created as a clone of Bug #1568109 +++

See attachment. Richard, can you see where this goes wrong?

Comment 1

[Richard Marti (:Paenglab)]

It seems, that it doesn't recognize that this is a vbox and use a hbox instead.
Jörg, is this reproducible? Always from same sender?

Comment 2

[Jorg K (CEST = GMT+2)]

This is a little puzzling. I've just started TB and the notification for a message from a different sender was fine. So thinking about it, it could be that it only affects notifications for messages from that Yahoo mailing list. Where do you see a vbox/hbox? Surely the formatting doesn't depend on the content that's formatted??

Comment 3

[Richard Marti (:Paenglab)]

It should be this vbox: https://searchfox.org/comm-central/source/mailnews/base/content/newmailalert.xul#27.

Yes, content should not be able to format this.

Comment 4

[Jorg K (CEST = GMT+2)]

Yes, but why is that vbox filled badly? That happens somewhere in newmailalert.js, no? Basically the entire thing is a vbox, with some hbox(?) children, each one containing information about one message.

Comment 5

[Richard Marti (:Paenglab)]

This is JS area, so better Magnus takes a look.

Comment 6

[Jorg K (CEST = GMT+2)]

Well, the action happens here:
https://searchfox.org/comm-central/search?q=_createFolderSummaryMessage()&case=true&regexp=false&path=

So somehow the boxes aren't glued together/layed out correctly. No special JS skills needed. As you can see, we create another vbox with hbox children and some other XUL stuff.

Comment 7

[Wayne Mery (:wsmwk)]

(Notifications historically go in MW FE)

Comment 8

[Jorg K (CEST = GMT+2)]

I've observed it for a while. Only the notifications from the Yahoo mailing list are badly formatted.

Comment 9

[Magnus Melin [:mkmelin]]

Actually, what is your expectation on how it should look? This "From" is unusually long which makes it look slightly odd, but other than that...

Comment 10

[Jorg K (CEST = GMT+2)]

Actually, you are right ;-)

The from on that list is, for example (redacted):
Mariana Sol Cubertina [e-mail] [Barcelona-Freecycle] <[e-mail]>

But the funnily formatted notification never occurred to me in TB 60. Hard to check now without going back to TB 60 on my production profile. Maybe the From: was truncated before? Or an ellipses added? CSS can do that. Did that change?

Comment 11

[Magnus Melin [:mkmelin]]

It can (text-overflow: ellipsis;) but then the boxes need to have given widths. I don't think we used to do that.

I tested with 60 now - you can easily test it by sending yourself a mail with a very long custom from. It does indeed behave differently: only one line for this info.

Comment 12

[Jorg K (CEST = GMT+2)]

Glad I convinced you. When upgrading to a new version, some things just cause a "huh?" effect without being able to pin down exactly what has changed.

Comment 13

[Magnus Melin [:mkmelin]]

Hmm, figured it out, but... 60 doesn't actually handle long from very nicely either: it can push the subject totally out of view and that looks bad. Also, comparing the behaviours I find myself missing the wrapped long subjects, typically all bugmail for instance.

Comment 14

[Magnus Melin [:mkmelin]]

Attachment: bug1569129_notification_formatting.patch

The crux here is that by label value="something" doesn't wrap, but label with text content does wrap.

I've made the sender use label value, so that won't wrap anymore, and cropping with ellipsis will take place if the from is too long. The subject is kept as textContent, so if it's long then it does wrap.

We can't use a max-width in combination with flex="1" - that makes the notification the wrong size, so I've set fixed sizes for subject and sender.

The platform specific css files were almost identical, so moved to a shared file.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9081887 [details] [diff] [review]
bug1569129_notification_formatting.patch

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

::: mail/base/content/foldersummary.js
@@ +138,5 @@
>          if (this.showSender) {
>            let addrs = MailServices.headerParser.parseEncodedHeader(
>              msgHdr.author, msgHdr.effectiveCharset, false);
>            let folderSummarySender = msgBox.querySelector(".folderSummary-sender");
> +          folderSummarySender.value =

What's the difference between setting textContent and value?

Comment 16

[Magnus Melin [:mkmelin]]

Per comment 14, when you set value the content is non-line-wrapping (i.e. strictly one line).

Comment 17

[Magnus Melin [:mkmelin]]

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/label#Wrapping

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 2019-07-31 10_52_44-.png

Something totally wrong now. While you fix it, please add a comment why we use value in one case and textContent in the other.

Comment 19

[Magnus Melin [:mkmelin]]

Did you rebuild? I'd imagine that's what it looks like without picking up the shared css.

Comment 20

[Magnus Melin [:mkmelin]]

Ah, I see the jar change didn't make it into the patch

Comment 21

[Jorg K (CEST = GMT+2)]

Re. comment 19: I work here since 2015 ;-)
Re. comment 20: That's what I was going to say.

Comment 22

[Magnus Melin [:mkmelin]]

Attachment: bug1569129_notification_formatting.patch

Will deal with the preview text in bug 1568109

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: 2019-07-31 11_30_34-Window.png

Looks good now.

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 9081897 [details] [diff] [review]
bug1569129_notification_formatting.patch

Thanks, and sorry for not reading the comment about value vs. textContent properly.

I'll set the uplift flags for you.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/627a555b3165
make mail notification properly formatted for long senders, and make sure long subjects also are properly shown. r=jorgk DONTBUILD

Comment 26

[Richard Marti (:Paenglab)]

(In reply to Magnus Melin [:mkmelin] from comment #14)

The platform specific css files were almost identical, so moved to a shared file.

An improvement could be to move the line in jar.inc.mn below the #ifndef XP_MACOSX because Mac doesn't use this notification.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/de1e979e4910
Follow-up: Don't include shared/newmailalert.css on Mac. r=jorgk DONTBUILD

Comment 28

[Jorg K (CEST = GMT+2)]

TB 69 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/b557db605509cf9a0166a264144268ac021864dc (incl. follow-up)

Comment 29

[Jorg K (CEST = GMT+2)]

TB 68.0 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/fff337e3341874a6cf123b1b20fb424e2cc930f4

Comment 30

[Jorg K (CEST = GMT+2)]

Finally I could see the effect in real life with the mailing list in question. Looks good now.