Account provionner: disable search button if no providers are listed

RESOLVED FIXED in Thunderbird 15.0

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jb, Assigned: mconley)

Tracking

(Blocks 1 bug)

13 Branch
Thunderbird 15.0
x86_64
Windows 7
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird13+ fixed, thunderbird14+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

7 years ago
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
Assignee

Updated

7 years ago
Assignee: nobody → mconley
Posted patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #627341 - Flags: ui-review?(bwinton)
Attachment #627341 - Flags: review?(bwinton)
I should point out that this patch relies on the patches in bug 757823, bug 758237, and bug 758707 in that order.
I've applied all of those, but this one is giving me an error patching mail/test/mozmill/newmailaccount/test-newmailaccount.js
Ah, yeah, I see that. Hang on...
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 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.
Attachment #627341 - Flags: ui-review?(bwinton)
Attachment #627341 - Flags: ui-review+
Attachment #627341 - Flags: review?(bwinton)
Attachment #627341 - Flags: review+
Posted patch Patch v2Splinter Review
(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).
Attachment #627341 - Attachment is obsolete: true
Attachment #627700 - Flags: review?(bwinton)
Comment on attachment 627700 [details] [diff] [review]
Patch v2

Okay, that seems reasonable.

Thanks,
Blake.
Attachment #627700 - Flags: review?(bwinton) → review+
Assignee

Updated

7 years ago
Attachment #627700 - Flags: approval-comm-beta?
Attachment #627700 - Flags: approval-comm-aurora?
comm-central: https://hg.mozilla.org/comm-central/rev/d8e6f2b61e01
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
Attachment #627700 - Flags: approval-comm-beta?
Attachment #627700 - Flags: approval-comm-beta+
Attachment #627700 - Flags: approval-comm-aurora?
Attachment #627700 - Flags: approval-comm-aurora+
Assignee

Updated

7 years ago
Depends on: 759327
Assignee

Updated

7 years ago
No longer depends on: 759327
You need to log in before you can comment on or make changes to this bug.