Closed Bug 1590503 Opened 6 months ago Closed 5 months ago

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

Categories

(MailNews Core :: Account Manager, defect)

defect
Not set

Tracking

(thunderbird71 fixed, thunderbird72 fixed)

RESOLVED FIXED
Thunderbird 72.0
Tracking Status
thunderbird71 --- fixed
thunderbird72 --- fixed

People

(Reporter: jorgk-bmo, Assigned: aleca)

References

Details

Attachments

(4 files, 2 obsolete files)

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?

Flags: needinfo?(richard.marti)
Flags: needinfo?(alessandro)

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.

Flags: needinfo?(richard.marti)

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.

Flags: needinfo?(alessandro)
Assignee: nobody → alessandro
Status: NEW → ASSIGNED
Attached image 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.

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.

Attachment #9105039 - Flags: feedback?(jorgk)
Attachment #9105039 - Flags: feedback?(alessandro)
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
Attachment #9105039 - Flags: feedback?(alessandro) → feedback+
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.
Attachment #9105039 - Flags: feedback?(jorgk)
Attachment #9105039 - Flags: feedback?(alessandro)
Attachment #9105039 - Flags: feedback+
Comment on attachment 9105039 [details] [diff] [review]
1590503-emailWizard-subtitle-wrap.patch

Sorry, mid-air collision, there was f+ from Alex.
Attachment #9105039 - Flags: feedback?(alessandro)
Attached patch 1590503-email-wizard.patch (obsolete) — Splinter Review

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.

Attachment #9105069 - Flags: review?(jorgk)
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 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 :-(
Attachment #9105069 - Flags: review?(jorgk)

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.

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.

Attachment #9105069 - Attachment is obsolete: true
Attachment #9105093 - Flags: review?(jorgk)
Attachment #9105093 - Flags: feedback?(richard.marti)
Attachment #9105093 - Flags: feedback?(mkmelin+mozilla)

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 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.
Attachment #9105093 - Flags: review?(jorgk) → review+
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?
Attachment #9105093 - Flags: feedback?(mkmelin+mozilla) → feedback+

(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 on attachment 9105093 [details] [diff] [review]
1590503-email-wizard.patch

Landing this now, I hope Richard likes it.
Attachment #9105093 - Flags: approval-comm-beta+

I tested it as well ;-)

Target Milestone: --- → Thunderbird 72.0

Pushed by mozilla@jorgk.com:
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

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED

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

Flags: needinfo?(alessandro)

On it!

Flags: needinfo?(alessandro)
Comment on attachment 9105093 [details] [diff] [review]
1590503-email-wizard.patch

Looks good for me. Thanks
Attachment #9105093 - Flags: feedback?(richard.marti) → feedback+

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.

Attached patch 1590503-follow-up.patch (obsolete) — Splinter Review

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

Attachment #9105371 - Flags: review?(jorgk)
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.
Attachment #9105371 - Flags: review?(jorgk)

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

Attachment #9105371 - Attachment is obsolete: true
Attachment #9105390 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/11e4fa28d82f
Follow up: fix failing test. r=jorgk
See Also: → 1601811
Regressions: 1604409
You need to log in before you can comment on or make changes to this bug.