758618 - Account provionner: disable search button if no providers are listed

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jb Piacentino]

In languages where there are no default search providers, users can press 'Search' and get an error. 

The 'Search' button should be disabled if 
- no search providers are selected 
- or none are displayed

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

This patch disables the search button if there are no providers listed that support the users language.

The search button is re-enabled once a provider is checked.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

I should point out that this patch relies on the patches in bug 757823, bug 758237, and bug 758707 in that order.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

I've applied all of those, but this one is giving me an error patching mail/test/mozmill/newmailaccount/test-newmailaccount.js

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Ah, yeah, I see that. Hang on...

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Ok, I had updated the patch for bug 758707 to include the test for making sure that the fields were re-enabled. Make sure you're using that patch, and not v1.  Should apply fine on top of that.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 627341 [details] [diff] [review]
Patch v1

Looks good.  ui-r=me.

>+++ b/mail/components/newmailaccount/content/accountProvisioner.js
>@@ -607,16 +611,18 @@ var EmailAccountProvisioner = {
>       if (supportsSomeUserLang) {
>+        hasSupportForUserLang = true;
>@@ -629,16 +635,22 @@ var EmailAccountProvisioner = {
>+    // If we didn't load any providers that support the user's language,
>+    // disable the search button. We'll only re-enable once the user has
>+    // selected one of the other providers.
>+    if (!hasSupportForUserLang)
>+      EmailAccountProvisioner.searchButtonEnabled(false);

Instead of hoisting this variable up out of the loop, what about having EmailAccountProvisioner.searchButtonEnabled set to false by default, and then setting it to true if we get into the supportsSomeUserLang case?

Other than that, I like it.  r=me with that fixed.

Thanks,
Blake.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #6)
> Comment on attachment 627341 [details] [diff] [review]
> Patch v1
> 
> Looks good.  ui-r=me.
> 

Thanks!

> >+++ b/mail/components/newmailaccount/content/accountProvisioner.js
> >@@ -607,16 +611,18 @@ var EmailAccountProvisioner = {
> >       if (supportsSomeUserLang) {
> >+        hasSupportForUserLang = true;
> >@@ -629,16 +635,22 @@ var EmailAccountProvisioner = {
> >+    // If we didn't load any providers that support the user's language,
> >+    // disable the search button. We'll only re-enable once the user has
> >+    // selected one of the other providers.
> >+    if (!hasSupportForUserLang)
> >+      EmailAccountProvisioner.searchButtonEnabled(false);
> 
> Instead of hoisting this variable up out of the loop, what about having
> EmailAccountProvisioner.searchButtonEnabled set to false by default, and
> then setting it to true if we get into the supportsSomeUserLang case?
> 

So the call EmailAccountProvisioner.beOnline() that's fired just before the function exits calls EmailAccountProvisioner.searchEnabled(true), which enables all of the search fields - so setting EmailAccountProvisioner.searchButtonEnabled(false) will be overridden when the function exits, regardless of whether or not a supported language exists.

I considered refactoring the dialog's search enabling and disabling logic to be more granular for this sort of thing, but there are quite a few cases to consider, and I think the potential for it to expose us to new bugs is higher than this patch is.

Instead, I've had the provider-loading function stash a boolean into EmailAccountProvisioner for whether or not at least one provider supports the user's language. That's potentially a step in the right direction for that refactoring, which we should perhaps do in a separate bug (and not target for beta).

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 627700 [details] [diff] [review]
Patch v2

Okay, that seems reasonable.

Thanks,
Blake.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

comm-central: https://hg.mozilla.org/comm-central/rev/d8e6f2b61e01

Comment 10

[Mike Conley (:mconley) (:⚙️)]

comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/6ee7d7b3c428

Comment 11

[Mike Conley (:mconley) (:⚙️)]

comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/ff96b286f877