1590503 - "Set Up You Existing Email Address" dialogue resizes itself when you click/type into the e-mail field.

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Jorg K (CEST = GMT+2)]

I was looking at bug 1590397 and wanted to create a new account. The panel came up narrow so I didn't see the buttons. So I made it wider, but as soon as I typed in the e-mail address, it became smaller again. That was on TB 70 beta 4, but it's the same on trunk.

This is a little annoying. Richard, do you see this, too?

Comment 1

[Richard Marti (:Paenglab)]

The dialog opens in a normal size and I see all buttons initially. But I can confirm that the window resizes when typing the e-mail address when the dialog was made bigger before. Tested under Windows 10 and Linux with trunk.

Comment 2

[Alessandro Castellani [:aleca]]

Mhh, the dialog should only resize if prompt messages or the printed content is bigger than the dialog window, and it shouldn't resize while just typing.
I worked on this a while back, and I thought I accounted for the scenario described above, but apparently not.
I'll take a look at it and upload a patch for it.

P.S. The dialog appears properly sized on elementary OS (Ubuntu GTK based desktop environment) and macOS mojave.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: too small.png

Comes up too small in a Spanish localisation. Works OK in English. Try Italian, language packs for 71 beta are here:
http://ftp.mozilla.org/pub/thunderbird/releases/71.0b1/linux-x86_64/xpi/
and for Daily here:
http://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-central-l10n/linux-x86_64/xpi/

EDIT: Italian doesn't look good either.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: 1590503-emailWizard-subtitle-wrap.patch

How about this? This lets the title wrap. The only issue I see, is, that the window doesn't grow enough in the height in the following steps (showing the result and manual setting). Especially when going to the manual setting.

Alessandro, this needs again your magic sizing calculations.

Comment 5

[Alessandro Castellani [:aleca]]

Comment on attachment 9105039 [details] [diff] [review]
1590503-emailWizard-subtitle-wrap.patch

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

Letting the text wrap is definitely a good first step.
I'll integrate these changes in the patch I'm working on to fix the resizing issues.
Cheers

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 9105039 [details] [diff] [review]
1590503-emailWizard-subtitle-wrap.patch

That looks OK in Spanish, but it still resizes when you start typing in the e-mail address field.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 9105039 [details] [diff] [review]
1590503-emailWizard-subtitle-wrap.patch

Sorry, mid-air collision, there was f+ from Alex.

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: 1590503-email-wizard.patch

This should take care of all the various edge cases.
The dialog doesn't resize anymore on typing, and it resizes properly when dealing with l10n strings.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 9105069 [details] [diff] [review]
1590503-email-wizard.patch

Why do you constantly have to call
-        window.sizeToContent();
+        this.resizeDialog();
some resizing method? Won't that constantly change the size?

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 9105069 [details] [diff] [review]
1590503-email-wizard.patch

I've tried it briefly.

Say the user has a really long name, like in German:
Hottentottenstottertrottelmutterattentäterlattengitterwetterkotterbeutelrattenfangprämie
Or:
Princess Supercalifragilisticexpialidocious

OK, they make the window wider to enter that name. Then the press continue and you make the window narrow again. Madness :-(

Comment 11

[Alessandro Castellani [:aleca]]

I'm calling the resizing method every time there's a change of panel.
This is necessary to account for the different content size of the various steps of the setup wizard, and to prevent scrollbars or cutoff elements.

So, what you're saying is, if the user resizes the window, we shouldn't resize it again.
I'll do that.

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: 1590503-email-wizard.patch

This patch accounts for a manually resized dialog.
If the content doesn't require extra space, the dialog is not resize automatically in order to maintain the user's preference.

This introduces an issue in case the user access the manual config step and then deletes the email to start again. The dialog doesn't shrink back to the original size, remaining tall with lots of blank space.

I'm asking feedback from Magnus and Richard for this.

Comment 13

[Jorg K (CEST = GMT+2)]

Thanks, this is what I suggested, yes, don't make it smaller. Do you know any software which would automatically shrink windows for you?

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 9105093 [details] [diff] [review]
1590503-email-wizard.patch

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

I haven't tried it again since you have two pending feedback requests, but this looks right to me.

::: mail/components/accountcreation/content/emailWizard.js
@@ +285,5 @@
> +   * Resize the window based on the content height and width.
> +   * Since the sizeToContent() method doesn't account for the height of
> +   * wrapped text, we're checking if the width and height of the "mastervbox"
> +   * is taller than the window width and height. This is necessary to account
> +   * for l10n strings or the user manually resizing the window. Bug 1590503.

I'd lose the bug number here.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9105093 [details] [diff] [review]
1590503-email-wizard.patch

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

I guess, though it's unfortunate to have to ever resize at all (after the initial setup). That jumping around is not nice at all. Is it really necessary to do it more than the once?

Comment 16

[Alessandro Castellani [:aleca]]

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

That jumping around is not nice at all. Is it really
necessary to do it more than the once?

Unfortunately, it is.
The various steps of the email wizard can drastically change in size based on the amount of content, selected options, and l10n.
We can't safely assume that one default size of the window dialog will fit them all, and it's always better to resize it accordingly.

With this code, the window is resized only if the content is wider or taller than the window. So, for the most part, no resizing will ever happen, but nonetheless we need this method as a fail safe for those cases.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 9105093 [details] [diff] [review]
1590503-email-wizard.patch

Landing this now, I hope Richard likes it.

Comment 18

[Jorg K (CEST = GMT+2)]

I tested it as well ;-)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7c267fcfa482
Only resize mail account setup dialog to make it bigger; let subtitle wrap if needed. r=jorgk

Comment 20

[Jorg K (CEST = GMT+2)]

Looks like this test failure is from here:
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1572456094/build/tests/mozmill/account/test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_bad_password_uses_old_settings

Comment 21

[Alessandro Castellani [:aleca]]

On it!

Comment 22

[Richard Marti (:Paenglab)]

Comment on attachment 9105093 [details] [diff] [review]
1590503-email-wizard.patch

Looks good for me. Thanks

Comment 23

[Jorg K (CEST = GMT+2)]

Since we're in follow-up mode, can you please remove the bug number here:
https://hg.mozilla.org/comm-central/rev/7c267fcfa482#l1.34
I forgot to do it. And also, let's use a local variable here:
https://hg.mozilla.org/comm-central/rev/7c267fcfa482#l1.37
so we don't get the element twice.

Comment 24

[Alessandro Castellani [:aleca]]

Attachment: 1590503-follow-up.patch

Here's the patch that fixes the failing test, alongside adding those changes you requested.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 9105371 [details] [diff] [review]
1590503-follow-up.patch

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

::: mail/components/accountcreation/content/emailWizard.js
@@ +290,4 @@
>     */
>    resizeDialog() {
> +    var contentHeight = document.getElementById("mastervbox").clientHeight;
> +    var contentWidth = document.getElementById("mastervbox").clientWidth;

Umm, why are they var's now?
I suggested:
let mastervbox = getElementById("mastervbox");
let contentHeight = mastervbox.clientHeight;

etc.

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: 1590503-follow-up.patch

OK, this is the follow-up the way I wanted it ;-)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/11e4fa28d82f
Follow up: fix failing test. r=jorgk

Comment 28

[Jorg K (CEST = GMT+2)]

TB 71 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/59f2278c050d8c7ad0d6fc8ac8b76e6cbaaca5ad
https://hg.mozilla.org/releases/comm-beta/rev/9f4ffa0f05f869a5914f632abd69cf4a3d6acebc