Closed Bug 1720540 Opened 3 years ago Closed 3 years ago

Compact dialog (window) size too small to fit

Categories

(MailNews Core :: Backend, defect)

Thunderbird 91
Unspecified
All
defect

Tracking

(thunderbird91+ fixed)

RESOLVED FIXED
92 Branch
Tracking Status
thunderbird91 + fixed

People

(Reporter: wsmwk, Assigned: aleca)

References

(Regression)

Details

(Keywords: regression)

Attachments

(5 files, 3 obsolete files)

Attached image 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?

Flags: needinfo?(alessandro)
Attached image Mac

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.

Flags: needinfo?(alessandro)
Assignee: nobody → alessandro

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

Attachment #9231225 - Flags: review?(alessandro)
Status: NEW → ASSIGNED
Attached image compact.png

I can't reproduce this issue running macOS 10.14.6

(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 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.
Attachment #9231225 - Flags: review?(alessandro) → review-

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

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

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.

Attachment #9231225 - Attachment is obsolete: true
Attachment #9231245 - Attachment description: Bug 1720540 - Call sizeToContent on folders compact dialog window load, and increase timeout to prevent edge cases. r=darktrojan → Bug 1720540 - Only update fluent args and call sizeToContent on folders compact dialog window load. r=darktrojan

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

(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!

Attachment #9231245 - Attachment description: Bug 1720540 - Only update fluent args and call sizeToContent on folders compact dialog window load. r=darktrojan → Bug 1720540 - Only update fluent args and call sizeToContent on folders compact dialog window load. r=darktrojan,ThomasD
Target Milestone: --- → 92 Branch

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

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED

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.

Attachment #9231245 - Flags: approval-comm-beta?

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

Attachment #9231245 - Flags: approval-comm-beta? → approval-comm-beta+

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.

Attachment #9231865 - Flags: review?(geoff)
Attachment #9231865 - Flags: review?(geoff) → review+
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/11c645550e71
Follow-up: fix load event handler callback. r=darktrojan

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

Attachment #9231865 - Flags: approval-comm-beta?

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

[Triage Comment]
Approved for beta

Attachment #9231865 - Flags: approval-comm-beta? → approval-comm-beta+

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

Flags: needinfo?(alessandro)

(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

Flags: needinfo?(alessandro)

(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?

(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.

Attachment #9231418 - Attachment is obsolete: true
Attachment #9231418 - Attachment mime type: image/png → text/none
Attachment #9231418 - Attachment filename: Screen Shot 2021-07-12 at 2.54.56 PM.png → Screen Shot 2021-07-12 at 2.54.56 PM
Attachment #9231418 - Attachment description: Mac dialog about OpenPGP cuts off on right.png → Mac dialog about OpenPGP cuts off on right
Attachment #9231419 - Attachment description: Mac dialog about OpenPGP cuts off on right part 2.png → Mac dialog about OpenPGP cuts off on right part 2
Attachment #9231419 - Attachment filename: Screen Shot 2021-07-12 at 2.53.24 PM.png → Screen Shot 2021-07-12 at 2.53.24 PM
Attachment #9231419 - Attachment mime type: image/png → text/none
Attachment #9231419 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: