77806 - Extremely long subject cause that body panel doesn't appear - sanity check subject length

Status: VERIFIED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[ji]

****Observed with 04/26 trunk build*****

Extremely long JPN subject shows as blank on the threadpane. 
Testcase will be attached later. Since the user seldom generates such a long 
subject, this is a very low priority bug.

Comment 1

[ji]

Attachment: Testcase 1: Message with JPN subject longer than 5000 chars

Comment 2

[Thomas]

Does this only applies to a JPN build?

Comment 3

[timeless]

[e-mail]: please read comment 0, it clearly says "04/26 trunk build".

you should be able to save the attachment to disk as a sibling of a mail folder
with a name like "testfolder", run mail and select that folder (which should
contain the single message). give it a try.

Comment 4

[Makoto Kato [:m_kato]]

This is strength result on Thunderbird 3.1.

- If View - Headers - Normal is turned on, body text isn't appeared.
- If View - Headers - All is turned on, it works fine.

So I change summary and assigned to default.

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Screenshot 1: Testcase 1 on TB 3.1

Comment 6

[Wayne Mery (:wsmwk)]

isn't just an intl issue

Comment 8

[Matěj Cepl]

Attachment: Testcase 2: Message with long subject (US-ASCII)

I wanted just to add "me too" (email from the Fedora update system) with clearly US-ASCII only email.

Comment 9

[Matěj Cepl]

Attachment: Screenshot 2: Testcase 2 in "Vertical View" Layout of TB

Comment 10

[Thomas D. (:thomas8)]

OT: attachment 558748 [details] also shows the waste of space on the right of the subject, due to the bad design of the header pane, where we reserve a full column-width for nothing except the date and a lost 'other actions' button somewhere far down.

Comment 11

[Thomas D. (:thomas8)]

We need an UI for overlong subjects, just truncating isn't sufficient.
Perhaps this:

1a) truncate overlong subjects after more than x lines (where x = 5?)
1b) alternatively, but more difficult to implement, something along bug 511408 to auto-resize the header pane up to a certain percentage/user-defined limit
2) show (more of) full subject in tooltip (how do tooltips behave for insanely long strings? truncate tooltip if required)
3) have a "..." link at the end of truncated subject, clicking which will act like the "more..." button for to/cc: expand full subject, show scroll bars on header pane (and fix Bug 511408)

Comments?

Comment 12

[Rimas Kudelis [use needinfo to ping]]

Attachment: Screenshot 3: Message 3 with long subject on TB 18 nightly, Linux

Comment 13

[Rimas Kudelis [use needinfo to ping]]

Attachment: Screenshot 3b: Tooltip of long subject, message 3 (Tb18, Linux)

Just wanted to add another "me too". I got this message with a ridiculously long subject today, and here's what it looks like.

Thomas D. asked about tooltips. Well, here you go... Not so useable either.

I would suggest to simply truncate the value to some number for now, if there's no time to implement a better solution. Subjects like that aren't normal anyway, and even if anyone (a human, not a computer) uses them like that, then they should know better.

Comment 14

[Rimas Kudelis [use needinfo to ping]]

I'd say 200 characters should be enough for everyone. :)

Comment 15

[Thomas D. (:thomas8)]

Ben, Jim, can you comment why long subjects don't respect header pane maximum height and do not cause a scrollbar within header pane, as seen on Screenshot 1 of attachment 437195 [details] and Screenshot 2 of attachment 558748 [details]?
I'd expect a similar behaviour as when clicking "3 more" button on long list of recipients (limited expanding of header pane, with scrollbar to see the remainder).

Comment 16

[Ben Bucksch (:BenB)]

comment 15:
> I'd expect a similar behaviour as when clicking "3 more" button on long list of recipients

That was extremely complex to implement, and has performance implications, and
subjects are expected to be no more than one line.

comment 14:
> I'd say 200 characters should be enough for everyone. :)

Agreed.

We should add a max-height: 2em; or so and an overflow-y: auto, but IIRC I tried that in another bug and it didn't work. Would be worth trying it again, in fact you can do it yourself in the live app using DOM Inspector.

Comment 17

[Thomas D. (:thomas8)]

(In reply to Ben Bucksch (:BenB) from comment #16)
> comment 15:
> > I'd expect a similar behaviour as when clicking "3 more" button on long list of recipients
> 
> That was extremely complex to implement, and has performance implications,

Sure. I'm not requesting the complexity of that feature in detail...

> subjects are expected to be no more than one line.

I think you yourself implemented Bug 489609 (subjectwrap) because many real-life subjects have more than one line.

> comment 14:
> > I'd say 200 characters should be enough for everyone. :)
> Agreed.

BMO bugs can have subjects with up to 255 characters, and I've seen a number of those in real life. Well, as long as we make the rest of the subject accessible, I guess 256 is a reasonable number; which amounts to something between 3 and 5 subject lines depending on screen size etc.

> We should add a max-height: 2em; or so and an overflow-y: auto,

The direction of that idea sounds good, perhaps a bit more or more flexible height.

> but IIRC I
> tried that in another bug and it didn't work.

That's probably bug 489609.

> Would be worth trying it
> again, in fact you can do it yourself in the live app using DOM Inspector.

https://addons.mozilla.org/de/thunderbird/addon/dom-inspector-6622/

Comment 18

[Thomas D. (:thomas8)]

A somewhat similar problem was fixed with a few lines in Bug 739311.

Comment 19

[Ben Bucksch (:BenB)]

> We should add a max-height: 2em; or so and an overflow-y: auto,
> but IIRC I tried that in another bug and it didn't work.

Ah, indeed, due to bug 492645. Adding as dep.

(This bug does not block 489609, though.)

Comment 20

[Ben Bucksch (:BenB)]

See bug 489609 comment 72 for explanation.

Comment 21

[Thomas D. (:thomas8)]

(In reply to Ben Bucksch (:BenB) from comment #19)
> (This bug does not block 489609, though.)

Ben, pls stop removing my changes to a bug without giving reason or asking for explanation. If you don't understand why other people are setting certain flags or dependencies, please ask and provide reasons that suggest removal, instead of briskly removing them without reason. It's not the first time, unfortunately.

Explanation how bug 489609 "depends on" this bug

- Short version:

There is a clear and obvious relationship of code, UI/behaviour, and discussion of problems between bug 489609 and this bug.

- Long version:

For a start, it is common practice on BMO to link followup bugs (this bug) using the "Depends on" field of the main bug (bug 489609), regardless whether a strict technical dependency is present or not. In bug 489609, Ben helpfully introduced subject wrapping. Subject wrapping breaks for long subject (this bug). So this bug is an edge case of bug 489609 which isn't solved yet, hence the dependency. So if we want to fix this bug, it's certainly useful and necessary to look into exactly the same code that bug 489609 was touching. It doesn't matter if bug 489609 is technically responsible for this problem or not, and this is not about playing blame games. It's about providing helpful links to related code, technical information and discussion of bug 489609 that can help solve this bug. It's not helpful to remove such links/dependencies, because that will make it harder for everyone to find them somewhere in a long row of comments.

And fwiw, using "See also" field would not be a good alternative either because it comes with a lot of informative and behaviour shortcomings (no tooltips, non-reciprocity of link, no friendly links, etc.) which makes it very inferior compared to a proper dependency link.

> > We should add a max-height: 2em; or so and an overflow-y: auto,
> > but IIRC I tried that in another bug and it didn't work.
> Ah, indeed, due to bug 492645. Adding as dep.

Bug 492645 is *one* possible explanation why bug 489609 was not able to address edge cases of long subjects. There might be other explanations and/or solutions.

Comment 22

[Ben Bucksch (:BenB)]

> Ben, pls stop removing my changes to a bug without giving reason or asking for explanation.
> If you don't understand why other people are setting certain flags or dependencies,
> please ask and provide reasons that suggest removal, instead of briskly removing them
> without reason. It's not the first time, unfortunately.

Thomas, while we appreciate your help and effort, devs know more about the code, and make the decisions. I have all the rights to override you.

A blocker is something that prevents a fix, something that needs to be done before it can be fixed. That this bug doesn't block bug 489609 is obvious, because bug 489609 is already fixed.

> Bug 492645 is *one* possible explanation why bug 489609 was not able to address edge cases
> of long subjects. There might be other explanations and/or solutions.

No, it's *the* reason. I tried to fix this bug here as part of bug 489609, but couldn't due to bug 492645. I have actually tried it, I diagnosed it, and found the reason, and recorded it, as you can read there.

I am getting very tired about you *constantly* contradicting me as a dev. Every bug ends up being a fight. Thus, this shall be my last comment about this matter here.

Comment 23

[Thomas D. (:thomas8)]

(In reply to Ben Bucksch (:BenB) from comment #22)
> > Ben, pls stop removing my changes to a bug without giving reason or asking for explanation.
> > If you don't understand why other people are setting certain flags or dependencies,
> > please ask and provide reasons that suggest removal, instead of briskly removing them
> > without reason. It's not the first time, unfortunately.
> 
> Thomas, while we appreciate your help and effort, devs know more about the
> code, and make the decisions. I have all the rights to override you.

This isn't only or primarily about code, and I never interfered with anything code-wise here. I have not questioned your coding skills. This is about bug management, workflows, QA and providing/linking of information which is helpful and required for solving a bug. I don't think you have any more rights in that respect than I do, after more than 5 years of me contributing on BMO. Afasik, you're a volunteer contributor just like me. Even if you had "all the rights to override me" - to have rights and to use them wisely and cooperatively are two different stories...

> A blocker is something that prevents a fix, something that needs to be done
> before it can be fixed. That this bug doesn't block bug 489609 is obvious,
> because bug 489609 is already fixed.

No, it's not obvious at all. Between black and white, there's always shades of gray. We have plenty of bugs that are fixed and still depend on "blockers" which are still open, like Bug 545955 (quickfilterbar) which currently has 5 open blocking bugs listed in "depends on".

You are right, a blocker is something that prevents a (full) fix, something that needs to be done before it can be (fully) fixed. In that sense, Bug 489609 isn't *fully* fixed, because subject wrapping clearly fails for the edge case of this bug. Or, in your own words from comment 22: 

> I tried to fix this bug here as part of bug 489609
> but couldn't due to bug 492645.

So this bug apparently *should* have been fixed as *part* of bug 489609, but according to you, that wasn't possible because of Bug 492645. Unfixed parts of a bug surely qualify as a "blocker", if only to provide a direct and obvious link to such unfixed parts (this bug) from the main bug (bug 489609).

As mentioned in my comment 21, most of which you either didn't read or deliberately ignored (nor answered appropriately), it is common practice on BMO to list related problems in the "depends on" field of their parent bugs, to create visible links between bugs, which does not always require a strictly technical relationship. Unfortunately, we only have a limited set of bug relationships in Bugzilla, so we need to use them with some flexibility.

> > Bug 492645 is *one* possible explanation why bug 489609 was not able to address edge cases
> > of long subjects. There might be other explanations and/or solutions.

> No, it's *the* reason. I tried to fix this bug here as part of bug 489609,
> but couldn't due to bug 492645. I have actually tried it, I diagnosed it,
> and found the reason, and recorded it, as you can read there.

I don't doubt that you've tried and done all that well. I do doubt that you never make mistakes. Perhaps the approach you tried to get this working is not the only approach that can be taken. E.g., you mention in Bug 492645 Comment 12 that another approach proven successful for similar problems might be to use the XUL "flex" attribute as a workaround to bug 492645. I don't know enough about the code yet to judge those approaches, but by setting the dependency, I am trying to open the door for others to look at the code in question (your patch in bug 489609) and perhaps come up with new ideas how to solve this problem.
For that reason alone (and not to fight you personally), I'll re-add the dependency.

By removing the dependency, you are actively and deliberately preventing progress on this bug. You're also adding to the impression that you don't want "your" bugs to come under scrutiny.

> I am getting very tired about you *constantly* contradicting me as a dev.
> Every bug ends up being a fight. Thus, this shall be my last comment about
> this matter here.

What, because you're a dev, it's not allowed to differ in opinion?
That's a strange sense of cooperation, especially now that Mozilla withdraws most professional manpower from the TB project and we have to organize ourselves as a community of volunteers.

Comment 24

[Thomas D. (:thomas8)]

Re-adding dependency for ease of tracking, as explained in comment 23.

Comment 25

[rsx11m]

Well, at least the constant change of the bug relationships brought some attention to this bug by anybody watching the other bug given the load of e-mail notifications (and that would be the only kind of dependency which is justified as this bug technically isn't a regression of bug 489609, though the issue was revived with that fix).

Ben is certainly correct that bug 492645 is the underlying cause that anything saying "no more than n lines for a wrapped box" fails on the CSS level. As I recall we had figured that XUL limitation out already during the bug 456596/bug 550487 work, which resulted in an explicit element-length determination and length counting to end up with an n-line address list and the subsequent "x more" button.

I don't think that a similar approach is advised here, thus applying a maximum subject length (with the full subject shown in the View > Headers > All setting) as a constant or a function of the current message-viewer/window size might be a good compromise for the time being. In the end, fixing bug 492645 (and then bug 511408) would give a more thorough solution here much better options.

Comment 26

[Ben Bucksch (:BenB)]

> anything saying "no more than n lines for a wrapped box" fails on the CSS level

Even more so, "no more than n px for a wrapped XUL box" fails, too.

Comment 27

[Jared Richo]

Thunderbird 15.0.1 is affected. I routinely get mails with very long subjects due to scripted output. Not only does the body not show in the index / pane view, but double clicking the mail to show it in a new tab still does not give the body, or a scroll bar to see it.

Since this is effectively makes it impossible to read some mails, I consider this pretty serious.

Comment 28

[Thomas D. (:thomas8)]

Related bug for *sending* overlong subjects: Bug 192562

Comment 29

[[e-mail]]

I just ran into this problem on thunderbird 17.0.5 (fedora 18)

Comment 30

[Stefan Neufeind]

same problem, Thunderbird 24 (Fedora 19).

Comment 31

[Stefan Neufeind]

Tried using the forward-button to get the data in a message-compose-view to at least somehow be able to read it. That just caused the CPU to spike though - Thunderbird not usable anymore. *wtf*

Comment 33

[Toralf Förster]

still an issue in 38.1.0 - /me wonders if an adversary could abuse this issue to bother TB users ?

Comment 35

[qqwy]

This is still an issue right now, in version 52.1.1.

Since it makes it impossible to read such an email, I think this bug should be fixed soon.

Comment 36

[Magnus Melin [:mkmelin]]

Looks like this would need css line-clamp (bug 866102)

Comment 37

[Magnus Melin [:mkmelin]]

Oh, which is implemented!

Comment 38

[Magnus Melin [:mkmelin]]

Attachment: bug77806_subject_height.patch

Limit subject to max 3 lines, after which you get scrollbars instead.
Use https://bugzilla.mozilla.org/attachment.cgi?id=32356 to easily reproduce.

Comment 39

[Wayne Mery (:wsmwk)]

Attachment: hover message list

Affects hovering over message list, but I'm not certain there is anything here to fix.

Comment 40

[Richard Marti (:Paenglab)]

Comment on attachment 9194399 [details] [diff] [review]
bug77806_subject_height.patch

Looks a bit weird using -webkit- but there are no alternatives yet. Thanks.

Comment 41

[Ben Bucksch (:BenB)]

Comment on attachment 9194399 [details] [diff] [review]
bug77806_subject_height.patch

Good approach and idea to use CSS only

I concur with Richard. At least the "box" should have a standard or -moz alternative, leaving only -webkit-line-clamp with webkit prefix. Can you please try whether these work together, and if so, use them?

Comment 42

[Richard Marti (:Paenglab)]

(In reply to Ben Bucksch (:BenB) from comment #41)

Comment on attachment 9194399 [details] [diff] [review]
bug77806_subject_height.patch

Good approach and idea to use CSS only

I concur with Richard. At least the "box" should have a standard or -moz alternative, leaving only -webkit-line-clamp with webkit prefix. Can you please try whether these work together, and if so, use them?

I tried it, the -moz alternative doesn't work.

Comment 43

[Ben Bucksch (:BenB)]

Thanks for testing this, to both of you.

Comment 44

[Magnus Melin [:mkmelin]]

Yes unfortunately standards line-clamp is not yet implemented, only -webkit-line-clamp, for compatibility purposes. And that requires display: -webkit-box;

Comment 45

[Thomas D. (:thomas8)]

(In reply to Wayne Mery (:wsmwk) from comment #39)

Created attachment 9194403 [details]
hover message list

Affects hovering over message list, but I'm not certain there is anything here to fix.

Imho the tooltip should also be cropped after a couple of lines.

Comment 46

[Ben Bucksch (:BenB)]

That should be trivial, by limiting the tooltip to 500 characters:

   el.tooltip = msg.subject.substr(0, 500);

(You cannot limit the tooltip to a number of lines, because it's native code, and CSS is not applicable. You don't know how large the screen is. A limit to a number of characters is a good enough approximation. Just make sure that it's long enough so any remotely close to normal subjects are not cut, only insane ones like Wayne showed. The tooltip is useful to show subjects that are too long for a line - that's the whole reason why we have the tooltip in the first place: to show overly long subjects.)

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/260b9ca429e3
limit the subject to max 3 lines, for more than that use scrollbars. r=Paenglab

Comment 48

[Magnus Melin [:mkmelin]]

This is the tooltip for a tree cell. I don't see exactly where we set it atm. Feel free to send a patch to fix it.

Comment 49

[Magnus Melin [:mkmelin]]

Comment on attachment 9194399 [details] [diff] [review]
bug77806_subject_height.patch

[Approval Request Comment]
User impact if declined: for extremely long subjects, message body can't be seen
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): css only, not risky

Comment 50

[Wayne Mery (:wsmwk)]

Comment on attachment 9194399 [details] [diff] [review]
bug77806_subject_height.patch

[Triage Comment]
Approved for beta

Comment 51

[Rob Lemley [:rjl]]

Thunderbird 85.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/39842adf700a

Comment 52

[WaltS48 [:walts48]]

Used Testcase #2 in my testing of the 85.0b3 release candidate on Windows 10.

Saved the testcase.
Opened it and the Subject field had a scrollbar.

Opened a composition window and copied the subject and sent it to another account.
Opened the message and the Subject field had a scrollbar.
The tool tip showed the whole subject.

Comment 53

[Wayne Mery (:wsmwk)]

Comment on attachment 9194399 [details] [diff] [review]
bug77806_subject_height.patch

[Triage Comment]
Approved for esr78

Comment 54

[Rob Lemley [:rjl]]

Thunderbird 78.6.1:
https://hg.mozilla.org/releases/comm-esr78/rev/9ae34847f12e

Comment 55

[WaltS48 [:walts48]]

Verifying in the 78.6.1 rc on Windows 10.