Account Provisioner search button should be disabled if the search input is empty

RESOLVED FIXED in Thunderbird 15.0

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks 1 bug)

unspecified
Thunderbird 15.0
x86
All
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird13+ fixed, thunderbird14+ fixed)

Details

Attachments

(1 attachment)

STR:

1)  Create a new profile, and wait for the Account Provisioner to open

What happens?

The search button is enabled, even though the search input is empty.

What's expected?

If the search input is empty, searching should not be possible.
Posted patch Patch v1Splinter Review
So it turns out that this bug is a regression caused by bug 758618 (oh boy).

This patch undoes bug 758618, and does things a bit better by using a built-in function that we should have been using instead.

I've added a test, and modified our older tests to make sure that they're typing manually into the text input, as opposed to poking the value in with jQuery.
Assignee: nobody → mconley
Attachment #628028 - Flags: review?(bwinton)
Assignee

Updated

7 years ago
Blocks: 758618
Keywords: regression
Comment on attachment 628028 [details] [diff] [review]
Patch v1

>+++ b/mail/components/newmailaccount/content/accountProvisioner.js
>@@ -632,17 +629,17 @@ var EmailAccountProvisioner = {
>-    EmailAccountProvisioner.searchButtonEnabled(EmailAccountProvisioner._hasSupportedLang);
>+    EmailAccountProvisioner.onSearchInputOrProvidersChanged();

That seems better…

>+++ b/mail/test/mozmill/shared-modules/test-newmailaccount-helpers.js
>@@ -86,19 +87,19 @@ function wait_for_provider_list_loaded(a
> function wait_for_search_ready(aController) {
>   mc.waitFor(function() {
>     mc.sleep(0);
>-    return aController.window.$("#searchSubmit").is(":enabled");
>+    return aController.window.$("#name").is(":enabled");

Hmmm…  Shouldn't those always be the same?  And/or should we check both?

Other than that, it seems reasonable.

r=me.
Attachment #628028 - Flags: review?(bwinton) → review+
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #2)
> Comment on attachment 628028 [details] [diff] [review]
> Patch v1
> 
> >+++ b/mail/components/newmailaccount/content/accountProvisioner.js
> >@@ -632,17 +629,17 @@ var EmailAccountProvisioner = {
> >-    EmailAccountProvisioner.searchButtonEnabled(EmailAccountProvisioner._hasSupportedLang);
> >+    EmailAccountProvisioner.onSearchInputOrProvidersChanged();
> 
> That seems better…
> 
> >+++ b/mail/test/mozmill/shared-modules/test-newmailaccount-helpers.js
> >@@ -86,19 +87,19 @@ function wait_for_provider_list_loaded(a
> > function wait_for_search_ready(aController) {
> >   mc.waitFor(function() {
> >     mc.sleep(0);
> >-    return aController.window.$("#searchSubmit").is(":enabled");
> >+    return aController.window.$("#name").is(":enabled");
> 
> Hmmm…  Shouldn't those always be the same?  And/or should we check both?

They aren't always the same, no. What wait_for_search_ready is for, is to cause us to spin until we're at the point where we can enter a search. Since we now do not enable the search button until there's text in the input, we need to key off of the input as opposed to the button.

Which now makes me realize that bug 758618 really didn't cause this - I think this behaviour always existed, otherwise wait_for_search_ready would have always failed. Still, the new approach is better.

> 
> Other than that, it seems reasonable.
> 
> r=me.

Thanks!
Assignee

Updated

7 years ago
No longer blocks: 758618
Keywords: regression
Comment on attachment 628028 [details] [diff] [review]
Patch v1

This one is not risky at all.
Attachment #628028 - Flags: approval-comm-beta?
Attachment #628028 - Flags: approval-comm-aurora?
Attachment #628028 - Flags: approval-comm-beta?
Attachment #628028 - Flags: approval-comm-beta+
Attachment #628028 - Flags: approval-comm-aurora?
Attachment #628028 - Flags: approval-comm-aurora+
Assignee

Updated

7 years ago
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.