1529295 - Unable to scroll when modifying an instant messaging account properties

Status: VERIFIED FIXED

(Thunderbird :: Instant Messaging, defect, P3)

Description

[Patrick Cloke [:clokep]]

Not sure when this broke, but when creating an instant messaging account in the account wizard when you get to the "advanced" step you can no longer scroll, so some options are cut-off at the bottom.

Looks like the last bug to touch this was bug 1520643?

Comment 1

[Patrick Cloke [:clokep]]

OK, so this seems to work in the account wizard for me now, but when going back to modify an account properties you are unable to scroll within the account properties.

Comment 2

[Patrick Cloke [:clokep]]

Was testing this UI due to bug 1562447 and I'm able to scroll now. Looks like this got fixed at some point.

Comment 3

[Patrick Cloke [:clokep]]

I seem to have confused myself with my previous comment.

This is still broken, but only when modifying an existing account. Creating a new account works fine.

Steps to reproduce:

1. Create (or have) an instant messaging account
2. Go in the account properties (on the "Chat" tab click "Show Accounts", then select your account, then click the "Properties" button).
3. Observe that the "General" properties should be able to be scrolled and can't.

Note that I do have OTR enabled, but I believe I originally observed this before any of that code landed.

Alex, I think this would be something quick to fix and I think we messed with this page recently for the OTR stuff, could that have broken it?

Comment 4

[Alessandro Castellani [:aleca]]

It seems like this section is missing a <vbox flex="1" style="overflow: auto;"> wrapping all the other elements, which other sections have.
I'm not sure if this was regressed by the implementation of the OTR tab, anyway, it's an easy fix indeed.
Do you want me to take care of it?

Comment 5

[Patrick Cloke [:clokep]]

(In reply to Alessandro Castellani (:aleca) from comment #4)

Do you want me to take care of it?

If you have time for it, that'd be great! I'm slammed at work for the next couple of weeks.

Comment 6

[Jorg K (CEST = GMT+2)]

Hmm, when modifying a chat account, there's no scroll bar, but when creating a new chat account, there is.

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1529295-chat-account-properties.patch

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: 1529295-chat-account-properties.patch

Sorry for the double request, I fixed some messed up indentation.

Comment 9

[Alessandro Castellani [:aleca]]

Jorg, I think Patrick is super busy.
Since you know the issue, would you be able to review this patch?

Comment 10

[Jorg K (CEST = GMT+2)]

I'm not really a XUL guy. Let's try Richard.

Comment 11

[Alessandro Castellani [:aleca]]

Comment on attachment 9085148 [details] [diff] [review]
1529295-chat-account-properties.patch

Let's go ahead and ask for a full review.

Comment 12

[Richard Marti (:Paenglab)]

Comment on attachment 9085148 [details] [diff] [review]
1529295-chat-account-properties.patch

Yes, adding this vbox fixes the issue.

Comment 13

[Alessandro Castellani [:aleca]]

Attachment: 1529295-chat-account-properties.patch

Thanks for the review.
I updated the comment to reflect the right reviewer.
I started a try push here, but all our previous builds seem to be busted, so, let's wait: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ac7198b958fb5e4530fff56ef482102074d0145e

Comment 14

[Jorg K (CEST = GMT+2)]

I'll check the try before landing it, the shippable build will work. BTW, I can change the reviewer when checking stuff in ;-)

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9085881 [details] [diff] [review]
1529295-chat-account-properties.patch

And you want that fixed everywhere, right?

Comment 16

[Alessandro Castellani [:aleca]]

(In reply to Jorg K (GMT+2) from comment #14)

I'll check the try before landing it, the shippable build will work. BTW, I can change the reviewer when checking stuff in ;-)

Thank you so much. I didn't want to give you extra work if it's a simple thing I can change and remember to do :D

And you want that fixed everywhere, right?

That'd be awesome.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: no-ws-patch.patch

Looks a whole less threatening when you ignore white-space ;-)

Comment 18

[Jorg K (CEST = GMT+2)]

Hmm, why the change to readonly="readonly"? I can't see any examples for this in the tree. Should you submit a new patch, please change the commit message to: Add scrollbar to instant messaging account properties panel. The bug summary is rarely a good commit message.
See: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
"The text of the message should be what you did to fix the bug, not a description of what the bug was."
(I'm surprised, since usually I don't need to edit your commit messages :-) )

Comment 19

[Richard Marti (:Paenglab)]

Oh, I missed this one. :-(
It's a XUL textbox, so it should be readonly="true".

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c25118b0881d
Add scrollbar to instant messaging account properties panel. r=Paenglab DONTBUILD

Comment 21

[Jorg K (CEST = GMT+2)]

Landed with the readonly fixed. Of course we uplift the landed patch, not the attached one.

Comment 22

[Patrick Cloke [:clokep]]

Alex, Richard, Jorg -- thanks for taking care of this! You guys rock!

Comment 23

[Jorg K (CEST = GMT+2)]

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

Adding a vbox won't blow up ESR68 ;-)

Comment 24

[Jorg K (CEST = GMT+2)]

Scrollbar present in TB 68 ESR.

Comment 25

[Jorg K (CEST = GMT+2)]

TB 69 beta 4:
https://hg.mozilla.org/releases/comm-beta/rev/69f507411a5a5ad1e79d83672be90e2f23cbc2fd