1720540 - Compact dialog (window) size too small to fit

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Wayne Mery (:wsmwk)]

Attachment: image.png

I'm seeing this both on Windows and Mac with 91.0b1 candidate

On Mac it is consistent. (at Mac default text size)
On Windows it is ... random?

Comment 1

[Wayne Mery (:wsmwk)]

Attachment: Mac

Comment 2

[Alessandro Castellani [:aleca]]

Uff, I think this is that damn Shadow DOM loading the buttons too late after the dialog as been sized based on it's content.
https://searchfox.org/comm-central/rev/c1c11fbd23611624428278985a4e84c4db2c960c/mail/base/content/compactFoldersDialog.js#49-53

I can fix this by increasing the delay, but as usual that's kind of arbitrary and not fail proof.
We should really figure out how to wait for the shadow DOM to be fully loaded.

Comment 3

[Thomas D. (:thomas8)]

Attachment: 1720540_compactDialogFixSize.diff

Here's a simple fix which works for me, calling window.sizeToContent() on the window's load event when we definitely know that everything has loaded.

Looking at the official description of DOMContentLoaded event, it looks unlikely that resizing at that time could work reliably, e.g. because stylesheets may not yet have loaded. And we don't want to use an arbitrary timeout as Alex said, so the traditional load event looks just right to me.

https://developer.mozilla.org/en-US/docs/Web/API/Document/DOMContentLoaded_event

The DOMContentLoaded event fires when the initial HTML document has been completely loaded and parsed, without waiting for stylesheets, images, and subframes to finish loading.

https://stackoverflow.com/questions/46989755/are-styles-applied-after-domcontentloaded-is-triggered/46994100

Comment 4

[Eckard Berberich]

Attachment: compact.png

Comment 5

[Eckard Berberich]

I can't reproduce this issue running macOS 10.14.6

Comment 6

[Alessandro Castellani [:aleca]]

(In reply to Eckard Berberich from comment #5)

I can't reproduce this issue running macOS 10.14.6

Unfortunately expected as this issue is totally random and depends on many factors.

Comment 7

[Alessandro Castellani [:aleca]]

Comment on attachment 9231225 [details] [diff] [review]
1720540_compactDialogFixSize.diff

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

Good intention, but unfortunately also this is not 100% safe.
Also with this, once every 4 or 5 times, the dialog is not resized properly.
Maybe we could do this, and also add a 100ms timeout. That should be pretty safe...I think.

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: Bug 1720540 - Only update fluent args and call sizeToContent on folders compact dialog window load. r=darktrojan,ThomasD

Comment 9

[Alessandro Castellani [:aleca]]

I tried a mix of the two solutions and it seems to work consistently.

Comment 10

[Wayne Mery (:wsmwk)]

I can reliably reproduce on my Mac when the dialog appears immediately on startup. I'm happy to test.

Comment 11

[Gus Andrews]

Attachment: Mac dialog about OpenPGP cuts off on right

Is this maybe the same issue? I'm seeing it on my Mac (Big Sur 11.4) in Daily 92.0a1 (2021-07-13) (64-bit)

Comment 12

[Gus Andrews]

Attachment: Mac dialog about OpenPGP cuts off on right part 2

Same issue, different dialog

Comment 13

[Thomas D. (:thomas8)]

(In reply to Gus Andrews from comment #12)

Created attachment 9231419 [details]
Mac dialog about OpenPGP cuts off on right part 2.png
Same issue, different dialog

This is interesting, and imo suggests that it might be Fluent causing this. Here it really looks as if some long strings have pushed things to the side.
Similarly, in the compact dialog, it might be the main text string which pushes things down.

Comment 14

[Alessandro Castellani [:aleca]]

This is unrelated to this bug as what you're using is a SubDialog from the Account Settings tab, so not a native OS dialog.

It looks like the issue is caused by the text with your email + name underneath the imported key not wrapping properly.
Please, open a dedicated bug and CC me on that and I can fix it.

Comment 15

[Alessandro Castellani [:aleca]]

I think I found a good solution that doesn't feel too hacky and it should be reliable in every OS.
I added the string fluent directly to the description so when the DOM is loaded we already have the proper amount of space occupied by the text.
The only thing that gets updated is the data attribute.
This, alongside the suggested fix from Thomas, looks consistently and makes the dialog resize properly.

I tested this also by manually adding an insane amount like 3485723098572340985723894GB and the dialog resized properly.

I suspect the issue is a mix of shadow DOM elements not physically occupying the space, alongside a tiny race issue with the large fluent string applying asynchronously, therefore randomly not being complete in the DOM when the "onload" event kicks in.
These are just speculations.

Comment 16

[Alessandro Castellani [:aleca]]

Here's a try run with the patch applied.
If it doesn't fail you can test try builds: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=9116dc49dd0ab6c1a13e65f536a70c93392b9139

Comment 17

[Wayne Mery (:wsmwk)]

(In reply to Alessandro Castellani [:aleca] from comment #16)

Here's a try run with the patch applied.

Tested three starts, all worked perfectly. Magnifique!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0a31d91ef4d6
Only update fluent args and call sizeToContent on folders compact dialog window load. r=darktrojan,ThomasD

Comment 19

[Thomas D. (:thomas8)]

Comment on attachment 9231245 [details]
Bug 1720540 - Only update fluent args and call sizeToContent on folders compact dialog window load. r=darktrojan,ThomasD

[Approval Request Comment]
Regression caused by (bug #): bug 1678856
User impact if declined: Our shining new compact prompt may appear with buttons cut off at the bottom.
Developer impact if declined: Devs might go wild if they receive another round of complaints about the compact prompt!
Testing completed (on c-c, etc.): Sure.
Risk to taking this patch (and alternatives if risky): Near zero. We're not doing anything indecent here.

Comment 20

[Wayne Mery (:wsmwk)]

Comment on attachment 9231245 [details]
Bug 1720540 - Only update fluent args and call sizeToContent on folders compact dialog window load. r=darktrojan,ThomasD

[Triage Comment]
Approved for beta

Comment 21

[Thomas D. (:thomas8)]

Attachment: 1720540_compactDialogSizeToContentFollowup.diff

Bug 1720540 - Follow-up: fix load event handler callback. r=darktrojan

Good catch, Geoff! This fixes the little flaw.

I should have seen that, too! Now I'm surprised that people have tested it and it seemed to work anyway, so perhaps sizeToContent in window.onload is not even needed any more after Alex made the Fluent to load earlier/sync. Anyway, it's not harmful.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/11c645550e71
Follow-up: fix load event handler callback. r=darktrojan

Comment 23

[Rob Lemley [:rjl]]

Thunderbird 91.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/8890239b3b11
Thunderbird 91.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/af2519663a70

Comment 24

[Alessandro Castellani [:aleca]]

Comment on attachment 9231865 [details] [diff] [review]
1720540_compactDialogSizeToContentFollowup.diff

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

Comment 25

[Wayne Mery (:wsmwk)]

Comment on attachment 9231865 [details] [diff] [review]
1720540_compactDialogSizeToContentFollowup.diff

[Triage Comment]
Approved for beta

Comment 26

[Rob Lemley [:rjl]]

Both the original patch and the follow-up are already on 91beta. Am I missing something?

Comment 27

[Thomas D. (:thomas8)]

(In reply to Rob Lemley [:rjl] from comment #26)

Both the original patch and the follow-up are already on 91beta. Am I missing something?

That's right, both patches are already on 91beta. All good here.

https://hg.mozilla.org/releases/comm-beta/log?rev=compactFoldersDialog.js

Comment 28

[Gus Andrews]

(In reply to Alessandro Castellani [:aleca] from comment #14)

This is unrelated to this bug as what you're using is a SubDialog from the Account Settings tab, so not a native OS dialog.

It looks like the issue is caused by the text with your email + name underneath the imported key not wrapping properly.
Please, open a dedicated bug and CC me on that and I can fix it.

Did a dedicated bug get opened? :thomas8?

Comment 29

[Thomas D. (:thomas8)]

(In reply to Gus Andrews from comment #28)

Did a dedicated bug get opened? :thomas8?

I guess that was an invitation to you Gus to file this ;-)
Anyway, I've filed bug 1733086 for that.