Folder Pane is not enabled after a successful account setup on a clean profile
Categories
(Thunderbird :: General, defect, P1)
Tracking
(thunderbird_esr78 unaffected, thunderbird86 affected)
Tracking | Status | |
---|---|---|
thunderbird_esr78 | --- | unaffected |
thunderbird86 | --- | affected |
People
(Reporter: aleca, Assigned: aleca)
References
(Regression)
Details
(Keywords: regression)
Attachments
(1 file, 9 obsolete files)
23.36 KB,
patch
|
aleca
:
review+
|
Details | Diff | Splinter Review |
After concluding a successful first configuration, the email wizard dialog closes but the window remains on the Account Central tab.
The Folder Pane remains collapsed and the UI doesn't updated until the next restart.
This is the error message when completing the "Get new email address" setup:
JavaScript error: chrome://messenger/content/accountUtils.js,
line 422: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE)
[nsIMsgMailSession.topmostMsgWindow]
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
More info:
- The issue seems to happen only after the creation of a new email.
- When using an existing email, the UI updates but the selection of the Inbox doesn't trigger the correct loading of the remote folders.
Assignee | ||
Comment 2•4 years ago
|
||
I think I found the problems, but I'm not sure if the possible solutions are correct.
UI not loading when an existing account is created on first run
The issue was that the Inbox
folder was getting selected before the proper eventListener were set in place, so the selection wasn't triggering any loading of the remove folders or messages.
By removing those setTimeout(..., 0);
the startup sequence works as expected.
Are those timeouts necessary?
UI not loading when creating a new email account
The problem is that, after the Account Wizard closes, this condition will always return false (https://searchfox.org/comm-central/rev/27183de94cdae57d6b19f02ca615726d6601c911/mail/base/content/msgMail3PaneWindow.js#757) since we're not passing through the okCallback()
method expected from the emailWizard
What should we do here?
Comment 3•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 4•4 years ago
•
|
||
This patch fixes the problem with UI not loading after an existing email account is configured on first run.
I also fixed the issue when a new email is created from the Account Provisioner, but there were some caveats.
- With Andrei, we created https://github.com/thundernest/static-test, so we can update the pref to point to
https://static-test.thunderbird.net/providerList
to simulate a new email creation process locally. - When reaching this part: https://searchfox.org/comm-central/rev/4eb14a03690b8d6bbe53853a33b6e09d330a05ea/mail/components/newmailaccount/content/uriListener.js#189-196, the listener fails returning this error:
client config xml = {"parsererror":{"sourcetext":"\u001f\b","$sourcet
- If I manually write the config.xml file like we do in the tests, the whole process completes successfully.
I suspect that we might need to update something on that new test site we created to ensure a good response, but I might be wrong and an underlying issue might be present.
Do we know of anyone actually using Ghandi and completing the process?
Since the Account Provisioner overrides the okCallback()
of the emailWizard, we don't pass the verifyAccount()
condition that would trigger the initialization of the UI. To fix that temporarily, I'm checking if the account provisioner is opened with the args.success
attribute, meaning a new email has been created, and if the user has any account currently configured but the folders pane is still collapsed, I trigger the LoadPostAccountWizard()
account.
This works only if bug 1689079 is applied.
Comment 5•4 years ago
|
||
Assignee | ||
Comment 6•4 years ago
|
||
I'm stumbling upon a problem where, after a successful account setup, the Inbox folder is selected by the messages and the other folders are not loaded. Clicking on another folder and then selecting again Inbox triggers the loading from the server.
Setting a timeout before letting the code select the Inbox folder solves this problem, but of course that's not good.
That means we probably have a race issue where something is getting configured and we select the Inbox folder too soon.
After trying to trace all the startup calls, I found out that the code stops here on first run: https://searchfox.org/comm-central/rev/19d26c50df73a50d1d26fc568f75f614dec9ddf5/mail/base/content/mailWindowOverlay.js#1940
Any suggestion?
Assignee | ||
Comment 7•4 years ago
|
||
I'm a bit out of my depth here, and it seems that the startup code handling the triggering of the email wizard is shared with seamonkey, so I don't know how much I can change.
I'm starting to think that, since this whole thing with dialogs will be dropped with the account wizard in a tab, maybe we should temporarily ignore this problem, pushing for the new wizard before the next esr.
What do you think?
Comment 8•4 years ago
|
||
If it works for this case to put MsgGetMessagesForAllServers on a timeout, that seems ok. Also add a comment that it's for a clean account so that case doesn't get missed in the future.
Assignee | ||
Comment 9•4 years ago
|
||
All right, this works properly for both Account setup and New Email setup on first run.
I solved the issues of the system integration dialog popping on top of the emailWizard dialog, and the fodler selection happening too early.
Now the UI loads correctly for every scenario, I think.
This whole timeout based process will be dropped entirely after we implement the Account Creation in a Tab, so this is mostly temporary just to not ship glitchy betas when setting up a new account.
Here's a try run to see if I destroyed something: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=e73c35d8650cc1d9512bacc4f0fe68770b6be7bf
Assignee | ||
Comment 10•4 years ago
|
||
As I expected, lots of test failures.
Assignee | ||
Comment 11•4 years ago
|
||
I fixed all the tests failing, at least locally.
Let's see: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=a11707574fde499391748cb0458edb4bf2ee7f64
Comment 12•4 years ago
|
||
Assignee | ||
Comment 13•4 years ago
|
||
So, apparently the application needs at least a loadStartFolder(null)
on startup in order to function, that's why those tests were failing.
I was trying to be smart and interrupt the selection of the start folder in case we didn't pass some conditions, but without that, no proper initial focus/selection is triggered.
Patch updated to keep the same condition structure to select the initial folder.
Another try run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=659d5e9774151d77b5e93ec9b89d328a156fb127
Comment 14•4 years ago
|
||
Comment on attachment 9202482 [details] [diff] [review]
1689724-account-setup-after.diff
browser_newmailaccount.js is still failing
Comment 15•4 years ago
|
||
Just to note that the test case added in bug 1693883 had a TODO item to test this bug (at the TODO item, the folder pane should again start showing)
Updated•4 years ago
|
Assignee | ||
Comment 16•4 years ago
|
||
I'm a bit lost with this.
I can't understand why that browser_newmailaccount.js
test fails with:
Found an unexpected alert:alert at the end of test run
I'm not able to make the test fail locally, and it passes on macOS in the try-server.
I also run the test with --verify
and that had a bunch of different failures, which I fixed, except for a failure that involves the missing Services.logins
on a second run.
Removing the Services.logins.removeAllLogins();
method added in bug 1693883 makes the test succeed with --verify
but I'm not sure if we should remove it.
I'm not sure where to look and what to try here.
Assignee | ||
Comment 17•4 years ago
|
||
I think I found the possible issue.
I just noticed that my Notification panel in Ubuntu showed multiple notifications with Daily failed to connect to server imap-provisioned.example.com
.
I suspect this is what that alert:alert
reported in the test failure is about, as I think on Linux and Windows those are recognized as system alerts, while on macOS are simply ignored as notifications.
How do we solve this?
Assignee | ||
Comment 18•4 years ago
|
||
I was able to fix the failing tests with the alert:alert
issue by implementing a MockAlertService, bu now I'm stumbling up on an old test failure fixed in bug 1663209.
Upon investigating, I found out that the changes in that bug are what caused this regression.
Without the setTimeout()
set to 0, the folder is selected too soon after a successful account setup and the initial download of the inbox is not properly triggered.
Assignee | ||
Comment 19•4 years ago
|
||
Hey Geoff, I'm pinging you to see if you have insights on this.
As I wrote before, removing the setTimeout wrapper for the selectFirstFolder()
causes the app to select the first folder without triggering the loading of the messages or the server folders on first run.
With this patch, the startup process after a successful setup completes correctly, but of course it causes an issue in this test that you previously fixed: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=133a6cc889b62408e66caccd1eb942f8f55a37e8&selectedTaskRun=VapsGTWvSMGGjEVy3JDgaA.0
I'm a bit stuck with this patch and I'm not sure what to do here as it seems that the FolderPaneSelectionChange()
method doesn't do its job if the Inbox folder is selected too soon on a fresh install.
Comment 20•4 years ago
|
||
Replace:
setTimeout(selectFirstFolder, 0);
with:
Services.tm.dispatchToMainThread(() => selectFirstFolder());
That seems to do the trick. I've had a fight with setTimeout
here before (bug 1663209) but I've never really understood the problem.
Assignee | ||
Comment 21•4 years ago
|
||
I tried that, sorry, I should have wrote that in my previous message.
Using Services.tm.dispatchToMainThread(() => selectFirstFolder());
doesn't properly trigger the load of the server at the end a new account setup.
Comment 22•4 years ago
|
||
Found it! There are two things calling getNewMessages
and the back end is getting confused.
Of the two, I'd remove the first because it shouldn't be up to the wizard to fetch new mail.
Comment 23•4 years ago
|
||
Assignee | ||
Comment 24•4 years ago
|
||
(In reply to Geoff Lankow (:darktrojan) from comment #22)
Found it! There are two things calling
getNewMessages
and the back end is getting confused.Of the two, I'd remove the first because it shouldn't be up to the wizard to fetch new mail.
Ah! Here's the culprit.
The email wizard one doesn't really do anything as it doesn't actually trigger the loading of new folders and messages.
We need to keep that as it's necessary for 2 scenarios:
- On first startup when no accounts are configured, the email wizard needs to trigger the fetching after a successful setup because selecting the newly created Inbox folder doesn't load anything without a timeout.
- If the user already has an account and creates a new one, we won't go through the
loadStartFolder
.
Assignee | ||
Comment 25•4 years ago
|
||
Assignee | ||
Comment 26•4 years ago
|
||
A tiny fix to properly load folders and messages of newly created accounts.
Tested this by manually creating 3 accounts in a row and everything seems to work like a charm.
Comment 27•4 years ago
|
||
Assignee | ||
Comment 28•4 years ago
|
||
Thanks for the review.
Assignee | ||
Updated•4 years ago
|
Comment 29•4 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/853c656dce05
Fix UI not updating after a new account setup. r=mkmelin
Description
•