1564443 - Conversation textbox always almost not visible because too small

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Richard Marti (:Paenglab)]

I can't say since when this happens because chat made TB crash before. But since it works again (20190706) the conversation textbox, there you enter the text for the chats, has almost no height. Only through the paddings in the textbox is the box visible.

Make it taller with the mouse works and it stays until restart.

Comment 1

[Richard Marti (:Paenglab)]

I checked in Inspector and see that the "conv-bottom" has "NaN" as values for the minheight and height. Like this that can't work. The last change at this position was in bug 1554633.

Comment 2

[Richard Marti (:Paenglab)]

Attachment: conv-bottom.png

What's shown in Inspector.

Comment 3

[Alessandro Castellani [:aleca]]

I noticed this issue yesterday for the first time.
I know it was working before as I did the de-xbl and used the chat almost everyday (before the crash) to tackle the various OTR bugs.
I'll take a look at this regression and see why those attributes are not properly set.

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: 1564443-conversation-textbox.patch

The issue is related to the fact that we fetch the line-height of the textarea element to calculate a default height value.
Probably something changed recently as the value returned is the string "normal" which it can't be converted to an integer.
Setting a default value to 1 and implementing a condition should temporarily take care of this.

I'll file a dedicate bug against FF to see if we can find which patch cause this regression, and if it might affect something else wince we're moving away from textbox in favour of html: fields.

Comment 5

[Jorg K (CEST = GMT+2)]

No need to file a bug, it was an intentional change: bug 1536871.

Comment 6

[Richard Marti (:Paenglab)]

Comment on attachment 9077558 [details] [diff] [review]
1564443-conversation-textbox.patch

You're in the hope getPropertyValue("line-height") will work again in the future?

With lineHeight = 1; the textbox is too small. I propose something like 20. With 20 the box has here almost the same height as before.

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1564443-conversation-textbox.patch

Setting lineHeight to 20 to maintain the height we always had, and removing the condition since with the new themes in 68 modifying the default textarea line-height attributes it's not possible.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Sorry for the bustage here :(

Comment 9

[Alessandro Castellani [:aleca]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)

Sorry for the bustage here :(

No worries, I do that all the time :D

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ad5905771f43
Fix height of conversation textbox to make it visible again. r=Paenglab

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 9077797 [details] [diff] [review]
1564443-conversation-textbox.patch

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

::: mail/components/im/content/chat-conversation.js
@@ +506,5 @@
>      }
>  
>      calculateTextboxDefaultHeight() {
>        let totalSpace = parseInt(window.getComputedStyle(this).getPropertyValue("height"));
> +      let lineHeight = 20;

- including an arbitrary constant in the code like this doesn't look great. When doing it, there should be a comment above explaining what the value represents, and why we can't compute it instead.
- I don't believe this value is actually a constant. If the font size changes for some reason (actually are you sure the default font size is the same on all platforms?), it'll likely need to be different.

Comment 13

[Patrick Cloke [:clokep]]

Comment on attachment 9077797 [details] [diff] [review]
1564443-conversation-textbox.patch

[Approval Request Comment]
Regression caused by (bug #): bug 1536871
User impact if declined: The input box is not visible for chat
Testing completed (on c-c, etc.): c-c has this fix
Risk to taking this patch (and alternatives if risky): I don't see much risk in taking this patch. We likely also want to take the patch for bug 1568121 if that gets one.

There's been at least one duplicate bug filed about this: bug 1569604.

Comment 14

[Jorg K (CEST = GMT+2)]

Grrrr, this affects 69 and the developer/reviewer didn't request uplift :-( - Now we apparently have a beta where chat doesn't work. Hard to believe that slipped through testing.

Comment 15

[Jorg K (CEST = GMT+2)]

Please don't change the tracking flags.

Comment 17

[Jorg K (CEST = GMT+2)]

TB 69 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/efa306ce07b572592f8891e894da0726cfebb345