1596582 - [autoconfig] Account creation dialog window is too small when addons are offered [TB 68 only]

Status: VERIFIED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Ben Bucksch (:BenB)]

See screenshot https://bugzilla.mozilla.org/attachment.cgi?id=9104934 . This screenshot is for TB 68 on Windows. Bug may be Windows only. TB trunk has a different dialog layout, but double-check there as well.

Not sure why that happens, because we make window.sizeToContent() calls. Maybe there's one missing after an async operation?

Comment 1

[Ben Bucksch (:BenB)]

I asked Neil to look into this.

Comment 2

[[e-mail]]

Attachment: Proposed patch for ESR

This appears to be caused by an interaction with the ordering of the CSS @-rules. In particular, the Windows version of accountCreation.css wants to declare the html namespace so that it can change the placeholder font style. However, with the @-rules in the order given, the entire accountCreation.css import fails to apply for some reason, however I don't know CSS well enough to know why.

I since noticed that bug 1527003 already switched the two @-rules, thus fixing the bug on trunk. (It also added parallel @-rules to the other platforms, although they seem unnecessary.) This suggests that this is indeed the correct fix. (My Windows PC doesn't want to build for some reason, but I was able to verify that switching the @-rules on the Linux build exhibits the same problem.)

Comment 3

[[e-mail]]

Obviously we want the shared accountCreation.css on all platforms anyway, but there's also the underlying reason of why the window doesn't resize correctly. What happens here is that without the shared CSS, all of the parts of the extension display want to take up as much width as they can. In the case of the icon though it hasn't started loading yet, so it doesn't know its width. Shortly after the window is resized, the icon loads, and realises that it needs 32px of width, so it unceremoniously shoves everything else out of the way.

With the shared CSS properly included, the parts of the extension display are limited in the width that they can take up, which means that there's room for the icon once it's loaded.

An alternative approach to using max-width on the <column> elements would have been to use flex="1" width="1" which prevents the column from affecting the sizeToContent width instead requiring it to size itself to the remaining width after deducting size for the icon and button (which themselves would always acquire as much width as they need).

Comment 4

[Ben Bucksch (:BenB)]

Really hideous bug: Namespace declaration clashes and sort order of imports - pfft! Good analysis and patch! Thanks.

I'll leave the review to aleca, because I'm sure he'll find this bug and patch interesting as well.

Comment 5

[[e-mail]]

With the shared CSS properly included, the parts of the extension display are limited in the width that they can take up, which means that there's room for the icon once it's loaded.

In addition, the shared CSS also defines the icon size, which means that the correct amount of space is calculated in the first place anyway.

Comment 6

[Ben Bucksch (:BenB)]

See #result_addon_install image.icon in https://searchfox.org/comm-central/source/mail/themes/shared/mail/accountCreation.css#282
This patch makes these CSS rules actually apply.

Comment 7

[Alessandro Castellani [:aleca]]

Comment on attachment 9112844 [details] [diff] [review]
Proposed patch for ESR

This is not the right solution, I think.
The problem is most likely due to a wrong max-width in the addon description.
I'm uploading a patch which fixed the issue on Linux.

For future reference, be sure to upload the file with a `.patch` extension and write the commit message with the current format:
`Bug XXXXXXX - Message. r=reviewer`

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: 1596582-account-creation.patch

Comment 9

[Ben Bucksch (:BenB)]

@aleca: I think you're talking about a different issue. This bug is about a Windows-specific problem. We don't see it on Linux, only on Windows. Neil found that the shared CSS file doesn't apply correctly on Windows. Did you test this on Windows, with and without the patch?

Comment 10

[Ben Bucksch (:BenB)]

Comment on attachment 9112844 [details] [diff] [review]
Proposed patch for ESR

Could you please re-test on Windows?
FWIW, .diff is the correct file extension for patches.

Comment 11

[Alessandro Castellani [:aleca]]

Comment on attachment 9112844 [details] [diff] [review]
Proposed patch for ESR

This issue happens also on Linux, and I still think that reordering CSS hierarchy is not the right solution.
Asking Richard to review this.

Comment 12

[Richard Marti (:Paenglab)]

Comment on attachment 9112844 [details] [diff] [review]
Proposed patch for ESR

This is for ESR only, true? C-C has this already.

Yes, the @import has to be before the @namespace.

Comment 13

[Alessandro Castellani [:aleca]]

Thanks Richard, if the first patch fixes the issue you can disregard mine.

Comment 14

[Richard Marti (:Paenglab)]

Comment on attachment 9114677 [details] [diff] [review]
1596582-account-creation.patch

This wouldn't work without Neil's patch and is no more needed.

Comment 15

[Ben Bucksch (:BenB)]

Comment on attachment 9112844 [details] [diff] [review]
Proposed patch for ESR

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Comment 16

[Ben Bucksch (:BenB)]

This is for ESR only, true? C-C has this already

Indeed (which shows that the fix was correct):
https://searchfox.org/comm-central/source/mail/themes/windows/mail/accountCreation.css

Comment 17

[Ben Bucksch (:BenB)]

Needs landing on ESR

Comment 18

[Alessandro Castellani [:aleca]]

The commit message is incorrect, it should be updated to something like: Bug 1596582 - Fix incorrect CSS import hierarchy in accountCreation.css. r=Paenglab

Comment 19

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2019-12-12 09-45-43.png

This issue is still present on Linux.
I'm gonna submit a patch for it.

Comment 20

[Alessandro Castellani [:aleca]]

Richard, how is this looking on macos?

Comment 21

[Alessandro Castellani [:aleca]]

Attachment: 1596582-account-creation_Linux-follow-up.patch

This fixes the problem for Linux.

Comment 22

[Ben Bucksch (:BenB)]

FWIW, it works fine here on Linux for me. I'm a little puzzled that you still have the problem after Neil's patch.

Would be nice for Richard to review this. Maybe he has a hunch what's going on.

Comment 23

[Alessandro Castellani [:aleca]]

It's pretty weird indeed. I asked a review to Magnus because I know he's on an Ubuntu system as well, and if I'm not wrong Richard is on Windows and macos.
Let's see what they both report.

Comment 24

[Richard Marti (:Paenglab)]

Tried on Mac and Linux Mint too - no cut off text or buttons. Could this be a scaling issue as you use a HiDPI screen?

Comment 25

[Jorg K (CEST = GMT+2)]

You're going to put this into TB 68.4 ESR? Pretty bad UI if we don't, IMHO.

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 9115543 [details] [diff] [review]
1596582-account-creation_Linux-follow-up.patch

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

I don't see this problem either. But if it fixes it for you, shouldn't hurt

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 9115543 [details] [diff] [review]
1596582-account-creation_Linux-follow-up.patch

> I don't see this problem either. But if it fixes it for you, shouldn't hurt

Did you build or patch 68? Both are 68-only patches?

Comment 28

[Magnus Melin [:mkmelin]]

I did build 68 yes. Both are 68-only afaik.

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 9115543 [details] [diff] [review]
1596582-account-creation_Linux-follow-up.patch

OK, then let's stick the flag on here as well. BTW, I've never built 68, it won't work since you need other rust, etc. I do try builds or unpack omni.ja which works nicely.

Comment 30

[Magnus Melin [:mkmelin]]

At least on linux I can build both just by updating to the right m-c and c-c revisions.

Comment 31

[Jorg K (CEST = GMT+2)]

Lucky you :-) - I have all the necessary repositories, but building 68 just fails miserably, mostly due to wrong rust version, etc.

Comment 32

[Jorg K (CEST = GMT+2)]

Geoff, you missed landing this bug and hence the panel still looks bad. This is part of the overall improvement package for Exchange detection.

Comment 33

[Geoff Lankow (:darktrojan)]

Perhaps you should not set the milestone until the bug is fixed.

68.4:
https://hg.mozilla.org/releases/comm-esr68/rev/5c342f84503eee5f3267a0b2984e7ac0097f9afe
https://hg.mozilla.org/releases/comm-esr68/rev/a1bd5116cd3ebdb3aa112971f80c2a76bbc5df39

Comment 34

[Jorg K (CEST = GMT+2)]

Perhaps you should not set the milestone until the bug is fixed.

Umm, how would the BMO query work then? You want to query all bugs with the approval bit set? The query I used to use qualified on the target milestone and the flags. Bugs with no target milestone set were missed for sure. Feel free to organise it differently in the future.

Comment 35

[Jorg K (CEST = GMT+2)]

Looks good in TB 68.4 on Windows.