1697575 - Open Email Wizard dialog in a Tab

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, task, P1)

Description

[Alessandro Castellani [:aleca]]

This is the first step to move the entire Account Creation workflow inside a Tab.
This initial patch will only deal with the following changes:

• Open the Email Wizard in a Tab.

The following aspects will not be touched and will be tackled in follow up bugs:

• UI of the email wizard.
• Other creation dialogs.
• New email account provisioner dialog.
• String changes.

This is to keep the patches as small as possible and follow an incremental implementation path.

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: bug1697575-email-wizard-tab.diff

WIP patch to gather some feedback and get some help in an area I'm currently stuck.

As I wrote in the original description of this bug, I'm not doing much in terms of strings or UI as I'm focusing on making the email wizard dialog work in a tab.

I'm currently having an issue with the verifyConfig() method: https://searchfox.org/comm-central/rev/991a6842fdbe755a625e08e399442de3970ce243/mail/components/accountcreation/content/verifyConfig.js#41

This method requires a msgWindow.notificationCallbacks but I don't really understand what that is, and if it's possible to avoid passing a msgWindow altogether.

The msgWindow we are currently passing to the emailWizard dialog is the MailServices.mailSession.topmostMsgWindow: https://searchfox.org/comm-central/rev/991a6842fdbe755a625e08e399442de3970ce243/mailnews/base/prefs/content/accountUtils.js#235

I'm kind of confused by the whole workflow here.
I assume here we should verify that the configuration is valid, and then continue by calling the success callback.
Am I wrong?

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9211133 [details] [diff] [review]
bug1697575-email-wizard-tab.diff

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

notificationCallbacks is used to get cert errors propagated back to the caller. It looks like that is still needed.

But it looks like what you use now would be the same window as before, so the callbacks would not change? Why doesn't it have them now?

Comment 3

[Alessandro Castellani [:aleca]]

I think I found the issue.
That method expects an nsIMsgWindow, so I'm passing the mail3Pane.msgWindow to the tab opening the new account hub.
I'm not sure why that msgWindow is not readily available, tho.

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: bug1697575-email-wizard-tab.diff

Another WIP progress to gather some feedback.

I'm currently dealing with 2 problems:

Privacy Tab stealing focus
When the Privacy tab appears it steals the focus, creating various problems. The most common are:

• The focus moves away from the wizard fields even if the user is typing.
• If the privacy tab appears when the wizard tab closes after a successful setup, the primary UI doesn't properly load.

Other than that, the email wizard works normally in a tab (tested only with a simple IMAP account).

Please ignore the account provisioner dialog
Right now it only opens, but it's not properly updated so it fails on pretty much anything. What should we do with it?
Should we close the Account Hub Tab when it opens?
It will nonetheless be moved to it's own tab in a follow up bug.

Right now I'm creating a couple of new startup methods since those we're using in the AccountUtils.js are shared with SeaMonkey, and managing those was getting very complicated.

I'm trying to remove as many callback functions as possible and have a more linear approach.

What do you think?
Do you see any red flags or wrong approaches?

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: bug1697575-email-wizard-tab.diff

Forgot to refresh the bookmark.
Sorry for the noise.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9212359 [details] [diff] [review]
bug1697575-email-wizard-tab.diff

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

Looks like a good start. The tab title should be something shorter: Account Hub or Account Setup

::: mail/base/content/msgMail3PaneWindow.js
@@ +819,5 @@
> +    // returns true if it had to migrate, which we will use to mean this is a
> +    // just migrated or new profile.
> +    let newProfile = migrateGlobalQuotingPrefs(
> +      MailServices.accounts.allIdentities
> +    );

I don't know what that was doing there. migrateGlobalQuotingPrefs looks like anchient junk...

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: Bug 1697575 - Open Email Wizard dialog in a Tab. r=mkmelin

Comment 8

[Alessandro Castellani [:aleca]]

Still fighting with a failure on macOS due to the way the platform handles focus.
The changes in this try-run pass locally in all platforms.
Let's see if I'm lucky: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=f721e103ce0a0cff0893471e565798b0a5ec30ed

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/563485d4bac2
Open Email Wizard dialog in a Tab. r=mkmelin

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: Bug 1697575 - Fix dark mode and Proton style for the account hub. r=Paenglab

Comment 11

[Alessandro Castellani [:aleca]]

These new bct5 failures for the browser_newmailaccount.js were most likely caused by this patch.
https://treeherder.mozilla.org/jobs?repo=comm-central&selectedTaskRun=DPU9_VpXQMWUiBWA5Bqe3g.0

I'll try to deal with them in a follow up, but nonetheless that type of "flip_flop" test will go away once we move the account provisioner in a tab.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/43c5915a13eb
Fix dark mode and Proton style for the account hub. r=Paenglab

Comment 13

[Alessandro Castellani [:aleca]]

Attachment: Bug 1697575 - Rename all emailWizard related files to accountSetup. r=mkmelin

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1702c4e774ac
Rename all emailWizard related files to accountSetup. r=mkmelin

Comment 15

[Alessandro Castellani [:aleca]]

For the various warnings and alerts we currently have in the email wizard, I was trying to replace them with a native HTML <dialog> as it works quite nicely as you suggested.

I noticed that it's only supported in Firefox Nightly via a pref.
Do you have any insight if the support will be available in the next ESR?

Otherwise, I think we should use the SubDialogs.

Comment 16

[Magnus Melin [:mkmelin]]

I've asked in bug 840640.

Comment 17

[Alessandro Castellani [:aleca]]

I'm trying to figure out if these elements are still used anywhere in our code:
https://searchfox.org/comm-central/rev/faba8b4037ce7434416ee2f5fb2d00497675795b/mail/components/accountcreation/content/accountSetup.xhtml#50-108

It doesn't seem like it, as the general "insecure" warning seems to be handled in one single place when showing the #warningbox element.
Just wondering if I'm missing something obvious.

Comment 18

[Alessandro Castellani [:aleca]]

Attachment: Bug 1697575 - Implement new UI and strings for the Account Setup. r=mkmelin,ui-r=Paenglab

Comment 19

[Magnus Melin [:mkmelin]]

Probably unused yes.

Comment 20

[Alessandro Castellani [:aleca]]

I missed a couple of fluent linting fixes.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cfcd9ece70e1
Implement new UI and strings for the Account Setup. r=mkmelin,ui-r=Paenglab

Comment 22

[Alessandro Castellani [:aleca]]

Another couple of patches for this meta bug before closing it.
Here's what's coming:

• XUL removal and UI polish
  Still a few XUL elements are present in the page, as well as some partially updated UI regarding warning dialogs and manual configuration area.
  The upcoming patch will take care of removing those and completing the UI work in order to get this whole tab to a pure HTML state.

• Final step/Guided introduction page
  This is the final page after a successful setup process, with links to guided configurations and CarDav/CalDav integrated autodiscovery.

Comment 23

[Magnus Melin [:mkmelin]]

Attachment: Bug 1697575 - make adding certificate exceptions on account setup work again. r=aleca

Also cleaned up some logging, and remove the showCertOverrideDialog function which can't have done anything since before 78: a cert must be passed in already for it to work.

Comment 24

[alta88]

I'd like to make a suggestion. The new email wizard (and almost every other account type setup) should not be in its own tab but rather integrated in the Account Settings tab UI.

• A visually prominent New Account item should replace the awkward Account Actions button.
• The Set as Default and Remove Account menuitems could be buttons on an account item, visible when hovered.
• New Account item selection will show the same nicely integrated Set Up Another Account/Account Hub? (as shown already in Account Central) in the Account Settings right panel.
• Selecting the account type then proceeds with the setup wizard.
• The current hodgepodge of behaviors (mostly dialogs) for different account types would be inlined in the panel, like the email wizard.

There are a lot of other UX enhancements that can be done to make things look unified and not jarring: for example, a new Filelink looks like it goes to a dead end; selecting New Account in menus should take the user to the Account Setting tab with the correct new account type setup selected, etc.

Comment 25

[Alessandro Castellani [:aleca]]

Should we uplift patch D115138 for beta?

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d71c00626b9c
make adding certificate exceptions on account setup work again. r=aleca

Comment 27

[Magnus Melin [:mkmelin]]

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

Should we uplift patch D115138 for beta?

Idk, nobody complained...

Comment 28

[Magnus Melin [:mkmelin]]

(In reply to alta88 from comment #24)

I'd like to make a suggestion. The new email wizard (and almost every other account type setup) should not be in its own tab but rather integrated in the Account Settings tab UI.

The plan is to integrate the other account setup parts also into the account setup tab, so that you would after email easily and naturally move on to connecting your address book, calendar, filelink, im, etc.

Comment 29

[Alessandro Castellani [:aleca]]

Attachment: bug1697575-account-hub-html.diff

Meanwhile, I'm still working on this behemoth of a follow up patch, which includes:

• Removing editable menulists.
• Converting warning box to native HTML dialog.
• Clean up strings and move to fluent.
• Updating the manual configuration UI.
• Implement better transitions between different states.

70% is done, with the majority of the work left needed on the warning dialog and the manual config UI.
I just wanted to upload a WIP patch to show the progress.
Apologies for the extra time is taking me.

Comment 30

[Alessandro Castellani [:aleca]]

Attachment: Bug 1697575 - Remove XUL and update strings and UI of Account Setup tab. r=mkmelin,ui-r=Paenglab

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/76d9e4c16c04
Remove XUL and update strings and UI of Account Setup tab. r=mkmelin,ui-r=Paenglab

Comment 33

[Alessandro Castellani [:aleca]]

Attachment: bug1697575-account-hub-final-WIP.diff

I'm uploading a WIP patch for you to review the UI with fake data.
If you open the Account Setup tab you should see the final view directly.

Only the dictionaries and closing buttons work as the rest will be hooked to the newly created account.
A couple of questions as I'm working on implementing the autodiscovery.

1. Should the calendar and address book autodiscovery happen whiel the account is being created (after the user clicks done) or we should defer it for when this view gets visible?

We could have the "Synchronize..." section hidden and show a loader while we try to fetch that info.

2. I'd like to show a dedicated view for the Calendar and Address Book configuration step, instead of having everything showed directly in this view.

This is to avoid to make the content of this page too tall in case the user has multiple calendars and address books to connect to.

3. What level of configuration control we want to add to this step?

Should we allow users to set the calendar color and other options directly here during the configuration step? Same thing for the address book.
I'm thinking we should maybe defer those options to the preferences dialog and keep the setup process as simple as possible, unless there are important reason to allow users controlling all the configuration options also during this step.

Pinging Richard for an initial round of feedback for UI and icons.

Comment 34

[Richard Marti (:Paenglab)]

Comment on attachment 9226959 [details] [diff] [review]
bug1697575-account-hub-final-WIP.diff

Looks good.

What happens when there are Address Books and Calendars and the user wants both? Does he need to add first one and then this page is shown again with the remaining option? This could end in three loops.

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

A couple of questions as I'm working on implementing the autodiscovery.

1. Should the calendar and address book autodiscovery happen whiel the account is being created (after the user clicks done) or we should defer it for when this view gets visible?

Maybe when this page is visible and show a spinner and prompt like Checking for linked Address Books and Calendars.... Then showing them like in your proposal.

2. I'd like to show a dedicated view for the Calendar and Address Book configuration step, instead of having everything showed directly in this view.

Sounds okay. But see my question on top.

3. What level of configuration control we want to add to this step?

Maybe we should show the same things we have in the properties dialogs. The user can leave the defaults but if he wants he can directly customize what he wants.

Comment 35

[Alessandro Castellani [:aleca]]

What happens when there are Address Books and Calendars and the user wants both? Does he need to add first one and then this page is shown again with the remaining option? This could end in three loops.

Good point.
Maybe we could handle these configurations inside subdialogs.
This approach will allow us to keep the code better separated and avoid turning the accountSetup.js into a massive file.

Visually, users will be able to close the subdialog and click on the other config, avoiding the potential multiple back and forth.

Comment 36

[Alessandro Castellani [:aleca]]

Attachment: bug1697575-account-hub-final-WIP.diff

Another WIP patch for the final step of the new account setup.
I'm asking a feedback specifically for the detection of calendars and address books.

The dialogs to set those up are not done yet.
I'm considering using what we already have but I don't know how doable it is.
I'll need more time to test that approach.

Do you see anything sketchy?

Comment 37

[Geoff Lankow (:darktrojan)]

Comment on attachment 9227323 [details] [diff] [review]
bug1697575-account-hub-final-WIP.diff

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

Looking good!

::: mail/components/accountcreation/content/accountSetup.js
@@ +2113,5 @@
> +
> +  // TODO...
> +  openAddressBooksSetup() {
> +    console.log("Opening address book subdialog");
> +  },

I think it would feel better if this was handled on the same screen (ie. without a subdialog). I imagined (when Configure is clicked) the Address Books box expanding downwards to show a check list of available books, and I suppose either a "Connect" (some other verb?) button or some wording that says it will happen when you click Finish.

Same with calendars except each calendar could also have its Properties button like it does in the existing new calendar dialog. Or not. Personally I think that's a distraction at this point.

@@ +2206,5 @@
> +    document.getElementById("linkedAddressBooks").hidden = !addressBooks.length;
> +
> +    // Look for calendars.
> +    let calendars = this.fetchCalendars();
> +    document.getElementById("linkedCalendars").hidden = !calendars.size;

You haven't `await`ed fetchCalendars. I guess you also mean to put the address book code in fetchAddressBooks.

@@ +2221,5 @@
> +
> +  /**
> +   * Fetch any available CalDav calendars.
> +   *
> +   * @returns {calendars[]} - An array of found calendars.

That's not what cal.provider.detection.detect returns, but I think you'll want to handle its return value inside this function and return an array.

::: mail/components/accountcreation/content/accountSetup.xhtml
@@ +614,3 @@
>  
> +    <aside id="successView" class="column first-column" hidden="hidden">
> +      <section class="account-success-block">

This block's wider than everything else in the same view.

@@ +649,5 @@
> +      </section>
> +
> +      <section id="linkedServices" hidden="hidden">
> +        <h3 data-l10n-id="accoutn-setup-linked-services-title"></h3>
> +        <p data-l10n-id="accoutn-setup-linked-services-description"

Typo, twice.

Comment 38

[Magnus Melin [:mkmelin]]

Comment on attachment 9227323 [details] [diff] [review]
bug1697575-account-hub-final-WIP.diff

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

Note that we do need to allow configuring the name and such for calendars we set up. 
Take a look at https://www.calconnect.org/events/calendars

... then try to subscribe e.g. to webcal://p48-caldav.icloud.com/published/2/MjA5OTc4NTc4MjA5OTc4NYPAre1eU33EoiR3Bg4yHV4icYsjNN3Uzsw9x9WG0lzQ5Muq52Nshz7YO1tZsS7DYvkEUxqMImgzP44a02GGyaY 
It's not the greatest experience ;)

::: mail/components/accountcreation/content/accountSetup.js
@@ +2247,5 @@
> +   * to close the wizard.
> +   */
> +  onFinish() {
> +    // Send the message to the mail tab in case the UI didn't load during the
> +    // previou setup callback.

typo

::: mail/components/accountcreation/content/accountSetup.xhtml
@@ +658,5 @@
> +                  class="account-success-block config-button"
> +                  hidden="hidden"
> +                  onclick="gAccountSetup.openAddressBooksSetup();">
> +            <img src="chrome://messenger/skin/icons/new-addressbook.svg"
> +                class="linked-service-image" alt="" />

nit: misaligned

@@ +735,5 @@
> +        <img src="chrome://messenger/skin/illustrations/accounts.svg"
> +             data-l10n-id="account-setup-step5-image"
> +             alt="" />
> +        <p data-l10n-id="account-setup-success-help"></p>
> +        <a href="https://support.mozilla.org/en-US/products/thunderbird/learn-basics-get-started"

shouldn't hard code en-US. Leave out, and I think it will find the right article for the locale

::: mail/locales/en-US/messenger/accountcreation/accountSetup.ftl
@@ +12,5 @@
>      { -brand-product-name } will automatically search for a working and recommended server configuration.
>  
> +account-setup-success-title = Account successfully created
> +
> +account-setup-success-description = Great work! You can now start using { -brand-product-name }, synchronize your Calendar, Address Book or set up another account.

This is a bit focused on the first setup, but many users set up more than one account.

Maybe skip this text where it is. Then under the created email account, have "Next" to click through setting up other services? That could give us more room for marketing our features (perhaps showing example screenshots or similar) in the space we gain there.

The address books and calendars do not yet work, but I think they will still need some user input in many cases, and allowing detail configurations similar to the mail setup. So the current UI is too simplistic.

Also keep in mind that the user should be able to start off with setting up an address book or calendar, e.g. if coming back later to set those up for the account...

Comment 39

[Magnus Melin [:mkmelin]]

Small bug in related to this: "Daily will attempt to auto-detect fields that are left blank." shown before we have any results

Comment 40

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Manual account setp: The tooltip for "Port:" mentions one can set it to "0" for autodetection but the input field doesn't allow values less than 1.

Comment 41

[Magnus Melin [:mkmelin]]

I think it should be blank, not 0 for autodetection now.

Comment 42

[Alessandro Castellani [:aleca]]

Thanks for the feedback.

Maybe skip this text where it is. Then under the created email account, have "Next" to click through setting up other services? That could give us more room for marketing our features (perhaps showing example screenshots or similar) in the space we gain there.

I don't think that's a good idea.
The goal here is to guide the user with a simple and fast setup process. Immediately returning available connected services is better than having a generic "Next" button.
For marketing, we can use the area to the right if we need to.

The address books and calendars do not yet work, but I think they will still need some user input in many cases, and allowing detail configurations similar to the mail setup. So the current UI is too simplistic.

I wrote it in the comment below the uploaded patch that I'm still working on those buttons.
Indeed, when the user clicks on one of those buttons a new UI will be revealed to allow setting things up properly.

Also keep in mind that the user should be able to start off with setting up an address book or calendar, e.g. if coming back later to set those up for the account...

Isn't this a bit out of scope for this bug?
We have the objective of removing all those separated dialogs to create calendars, address books, etc, but that's for future iterations.
Right now we should focus on a pure and optimal email setup, and then later slowly iterate to implement the other account types.

Small bug in related to this: "Daily will attempt to auto-detect fields that are left blank." shown before we have any results

Yeah, I saw that and I fixed it in this WIP patch.

Manual account setp: The tooltip for "Port:" mentions one can set it to "0" for autodetection but the input field doesn't allow values less than 1.

Good catch, thanks.

Comment 43

[Magnus Melin [:mkmelin]]

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

Isn't this a bit out of scope for this bug?

Well, if you don't do that, we'll have to keep all the other pesky dialogs, and it would be good not to have extra duplication where not required.

That's also why I'm suggesting above to make the "step" selectable so you can start off, or re-start from the service you want. (Thus the "next").

Immediately returning available connected services is better than having a generic "Next" button.

Perhaps, but there are many cases you can't autodetect. You may easily want to set up all the services but they cannot be autodetected because the users wants a combination: you're using your isp for mail but google for your calendar, nextcloud for you addressbook etc... Not to mention how to add other services to the setup flow: e.g. you can't autodetect feeds even if people may want to use them (but didn't know we had them).

Comment 44

[Alessandro Castellani [:aleca]]

Attachment: Bug 1697575 - Implement the final page in the Account Setup. r=mkmelin,ui-r=Paenglab

Comment 45

[Alessandro Castellani [:aleca]]

I'm answering here to the comments on Phabricator as I just found out that it hides "old" comments.

Note: DNS SRV are not trustable. If it's not the same domain, we need to make sure we don't send creds before the user confirms. I didn't check if we do this or not now...

I'm passing the hostname for calendar and address book detection, alongside the password if the user wrote it, otherwise the dialog with a password prompt will appear. I'm not relying on DNS SRV.

You can now use this account in Daily!
For a better experience, you will want to connect related services as well.

Sounds a bit strange, but I like the direction. What about this?
You can now use this account with { -brand-short-name }.
Connect related services and configure account options for a better experience.

Create address book: let's not add this. In general people should not create more local address books unless they really know what they are doing.
Create calendar: should probably read Create New Calendar. But... I think it would be better if we have that last in the Calendars section. Similarly there should be a way to manually add address books even if they weren't found. This could also be last in the Address book sections. This should allow adding an LDAP address book as well.

All right, I'll need to completely change the UI of this last section as I wasn't showing it entirely if no AB or Cal were found.
I will show this section regardless, and update the UI to only reflect if we found connected services.
Buttons to create an AB or Cal will be available underneath the related sections.

In general, I'm not sure we want to use the word synchronize here or in the header. It could be better to say "connect". Synchronize souds a bit much like it's a one time thing, which makes it way less appealing.
This could also make the wording better for the header, for the case where we don't find anything. "Synchronize your linked services" could read "Connect other services"

Sounds good.

The calendar "synchronize" buttons do not work. (js error in console)

Yes, I wrote in the original comment in Phab about that feature not being done yet.

Anyway, perhaps it's better to have them be pre-checked checkboxes and if you click through you're automatically set up with them all?

I'm not a fan of checkboxes as it's not immediately clear how to use them.
The user might assume that simply checking them and clicking "Finish" will set up everything, but I'd like to avoid that.
The "Finish" button should only close the tab and post a message to the main window, without doing any configuration.
I'd like to keep the setup of calendars and address book within the page via dedicated buttons, or with the "Connect all" button if the user wants.
Doing this allows us to show a "Done" feedback when something is properly connected. By using the checkboxes we would lose that and the user wouldn't know if the setup was successful or not unless they go around the app looking for those services.

Skip the "set up another account" - I don't think that's a very common, and if you do it atm you're totally lost and can't go back.

Are you sure? That link simply resets the tab to restart the account setup.
It might be an edge case, but it's convenient if the user needs to add 2 or 3 email accounts all at once without needing to close the tab and reopen it.
I don't have a strong opinion on this, so if you think that link is redundant I'm okay with removing it.

"Finish" right aligned?

I think it's better to keep it centered.
We're using right aligned buttons for all the steps of the account setup, so having a final centered button is more indicative of a final and ultimate action, rather than an expectation of a potential next step.

Comment 46

[Alessandro Castellani [:aleca]]

Attachment: Account [e-mail]

I'm experimenting with the UI of this final section as is changing from the initial mock-ups and I need to better visually prototype this.

What do you thin about this?
A collapsible area with the available address books and calendars, and a button below to create a new one.

Comment 47

[Alessandro Castellani [:aleca]]

Attachment: Account [e-mail]

This is another iteration.

Other than showing how it might look before a container is opened, this is a bit more clean without the number and a visible description for both areas.

Comment 48

[Richard Marti (:Paenglab)]

Comment on attachment 9228851 [details]
Account [e-mail]

I like the other mock-up more as the number isn't clear for what it is. In your mock-up I thought first it's step 2 and step 3. :-)

Comment 49

[Alessandro Castellani [:aleca]]

I updated the patch on Phabricator with the new UI.
The Calendar setup is not completed yet, that should be the last step.

Comment 50

[Magnus Melin [:mkmelin]]

Comment on attachment 9228852 [details]
Account [e-mail]

I guess this is slightly preferable.
I wouldn't mind having the "step 2", "step 3" or somehing, but I guess the numbers wasn't about that.

Comment 51

[Alessandro Castellani [:aleca]]

Try-run to see how many things I've broken with the last patch: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=89d0f7e5cb6faa77e9c86fca156638a98408c9a9

Comment 52

[Magnus Melin [:mkmelin]]

Small bug report: enter a gmail address, let it find the settings. Under manual config, blank the ports. Hit Re-test. The Looking up configuration: Probing server..." remained, with the "The following settings were found by probing the given server:" beneath it. (The probing server... should go away once found)

Comment 53

[Alessandro Castellani [:aleca]]

Fixed pretty much everything.
Now I'm battling with implementing tests to cover the entire final section.

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/09c0477c15e9
Implement the final page in the Account Setup. r=mkmelin,ui-r=Paenglab

Comment 55

[Alessandro Castellani [:aleca]]

Last step for this is making the test work and covering the entire final page.

Comment 56

[Alessandro Castellani [:aleca]]

Gathered some feedback for potential string changes that we might consider for this week:

Privacy policy in the footer

Your credentials will be used according to our <a>privacy policy</a>
and will only be stored locally on your computer.

• The Privacy Policy text at the bottom of the page feels a bit too corporate and very similar to what companies like Google or Amazon do, and might scare the users. Maybe we could change it with something along the line of:

Your credential will not be shared with anyone and will only be stored locally on your computer.
<a>Learn more</a> about what type of data is collected by Thunderbird.

Looking up config notification

• The loading messages during autoconfig now are not showed stacked one after another, so it's not really useful having those strings replacing the loading notification as they're not readable by the user.

Looking up configuration…
Looking up configuration: Trying common server names…
Looking up configuration: Probing server…
Looking up configuration: { -brand-short-name } installation…
Looking up configuration: Email provider…
Looking up configuration: Mozilla ISP database…
Looking up configuration: Incoming mail domain…
Looking up configuration: Exchange server…

Most of the times the system is so fast these messages are not readable, and when the system takes a bit more time, the message settles on the last string, which is Looking up configuration: Exchange server…. Not very useful.

We should leave the generic Looking up configuration… in the notification UI, and use the other strings only for the logs.

Exchange
Microsoft is heavily pushing towards Office365/Microsoft365, so our Exchange option, even if accurate, looks a bit outdated and is not helpful at first glance for users that don't know or never used an Exchange server but they're using Office365.
We should rename those strings for the option name and description to be more discoverable and also write something along the lines of what we use for IMAP and POP3.
Eg. Connect to the Microsoft Exchange/Office365 server, or something like that.

Error message
When the credentials don't work for an open protocol, but we have Exchange as a viable option, the error message is very long and we don't suggest/recommend the Exchange option as a viable option.
We should maybe explore the idea of having a smaller error message with some visual indicators inside those protocol options to better guide the user.

Comment 57

[Alessandro Castellani [:aleca]]

Geoff, not sure if you have the time to take a look at the test that landed in the last patch (with a skip() for now).
I'm having issues in making the last click on the finish button succeed and I can't figure out why.
Any hint?

Comment 58

[Wayne Mery (:wsmwk)]

Would it help to have a support article mentioning exchange/o365?

Comment 59

[Geoff Lankow (:darktrojan)]

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

Geoff, not sure if you have the time to take a look at the test that landed in the last patch (with a skip() for now).
I'm having issues in making the last click on the finish button succeed and I can't figure out why.
Any hint?

You're waiting for chrome://pippki/content/exceptionDialog.xhtml to open and it never does. Stop doing that and everything else works.

Comment 60

[Alessandro Castellani [:aleca]]

(In reply to Wayne Mery (:wsmwk) from comment #58)

Would it help to have a support article mentioning exchange/o365?

Not sure, probably it wouldn't hurt. Pinging Ben as he's the expert on this.
Ben, how would you improve the Exchange wording?

Exchange
Microsoft is heavily pushing towards Office365/Microsoft365, so our Exchange option, even if accurate, looks a bit outdated and is not helpful at first glance for users that don't know or never used an Exchange server but they're using Office365.
We should rename those strings for the option name and description to be more discoverable and also write something along the lines of what we use for IMAP and POP3.

What about "Connect to the Microsoft Exchange/Office365 server"?

Comment 61

[Alessandro Castellani [:aleca]]

Attachment: Bug 1697575 - Update strings and fix broken test in new account setup. r=darktrojan

Comment 62

[Ben Bucksch (:BenB)]

Hey Alec, thanks for the ping. I've commented in Phab.

Comment 63

[Ben Bucksch (:BenB)]

In reply to Wayne Mery (:wsmwk) from comment #58:

Would it help to have a support article mentioning exchange/o365?

@Wayne:
It sure would! Can you ping me per email on this, what you have in mind and how we can help?

Comment 64

[Ben Bucksch (:BenB)]

Attachment: [autoconfig] Fix Exchange addon install

Exchange addon install is broken. Just a missing import.

Regression

Comment 65

[Ben Bucksch (:BenB)]

Attachment: [autoconfig] Add Office365 as the official brand name

When offering the Exchange option, the label currently only says:
"
( ) Exchange
Microsoft Exchange server
"

1. For most users, this is non-descriptive and very cryptic. They might not know what "Exchange" is. For IMAP and POP3, we explain better:
   "Keep your folders and emails synced on your server" and "Keep your folders and emails on your computer"
   We should explain the Exchange option better as well.

2. Users who use Office365 consider that to be the brand name. Microsoft uses only Office365 (and nowadays Microcrosft365) as brand name. "Exchange" does not appear anywhere towards the end user in their daily routine, but "Office365" is everywhere and that's what they recognize. We should use the description that users recognize.

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f3895481be18
Update strings and fix broken test in new account setup. r=darktrojan

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f5b6dfe3acf9
[autoconfig] Add Office365 as the official brand name. r=aleca,mkmelin

Comment 68

[Alessandro Castellani [:aleca]]

Closing this as we landed a lot of good stuff for 91 and we kinda met the objective of this bug.
Let's move forward with bug 1700487.

Comment 69

[[e-mail]]

Attachment: 1697575.diff

While playing with account creation I did run into a few issues. I've attached some changes that work for me but I'm not 100% convinced that they're correct.

• The confirmExchange function looks to have a number of bugs. It attaches two event listeners, but only the listener that fires (if any) is actually removed. Furthermore, it looks as if the Abortable ends up closing the tab instead of the dialog, as it uses close(); instead of dialog.close();.
• If you try to cancel the Exchange autodiscover insecure redirection confirmation prompt, it actually confirms the prompt. This is because the buttons have the wrong ids. Once fixed, cancelling still doesn't work, partly due to bug 1720195, and partly because the UI doesn't get completely reset. Removing the self._abortable = null; seems to fix this last issue.
• Errors during discovery don't appear because the UI is reset after the error is displayed. Switching the order of the two operations seems to fix this issue.

Comment 70

[Ben Bucksch (:BenB)]

@Neil: Thanks for the fixes. Given that Alec just closed this bug, can you file a new bugzilla bug about the bugs and attach the patches there, please?

Comment 71

[Alessandro Castellani [:aleca]]

Comment on attachment 9230808 [details] [diff] [review]
1697575.diff

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

Hey, thank you so much for the proposed fixes.
This looks good and apologies for the issues.

> If you try to cancel the Exchange autodiscover insecure redirection confirmation prompt, it actually confirms the prompt. This is because the buttons have the wrong ids. Once fixed, cancelling still doesn't work, partly due to bug 1720195, and partly because the UI doesn't get completely reset. Removing the self._abortable = null; seems to fix this last issue.

Can you try again now that bug 1720195 has been confirmed as a dup and the issue should be fixed?

As suggested by Ben, please open a bug as regressions caused by this current bug and upload your updated patch.
You can ask for a full review to me on the new patch so we can land it if it's good.

Cheers,

Comment 72

[Ben Bucksch (:BenB)]

Comment on attachment 9230808 [details] [diff] [review]
1697575.diff

Patch with regression fixes moved to bug 1720307, as discussed above.